Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Workspace: InOb-based load scheduler #31591

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Workspace: InOb-based load scheduler #31591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a33b707
InOb-based load scheduler
Dec 7, 2020
e5c63a0
ensure that measure APIs are not needed for loaderV2
Jan 13, 2021
b6bba50
do not recompute media/sizes for loaderV2
Jan 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions builtins/amp-img.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {Layout, isLayoutSizeDefined} from '../src/layout';
import {Services} from '../src/services';
import {dev} from '../src/log';
import {guaranteeSrcForSrcsetUnsupportedBrowsers} from '../src/utils/img';
import {isExperimentOn} from '../src/experiments';
import {listen} from '../src/event-helper';
import {propagateObjectFitStyles, setImportantStyles} from '../src/style';
import {registerElement} from '../src/service/custom-element-registry';
Expand Down Expand Up @@ -261,6 +262,11 @@ export class AmpImg extends BaseElement {
return false;
}

/** @override */
isLoadableV2() {
return isExperimentOn(this.win, 'amp-img-loader-v2');
}

/** @override */
layoutCallback() {
this.initialize_();
Expand Down
49 changes: 49 additions & 0 deletions examples/loader.amp.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<!doctype html>
<html ⚡>
<head>
<meta charset="utf-8">
<title>Lorem Ipsum | PublisherName</title>
<link rel="canonical" href="https://medium.com/p/cb7f223fad86" >
<meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1">
<script async src="https://cdn.ampproject.org/v0.js"></script>

<style amp-custom>
body {
margin: 0;
font-family: 'Georgia', Serif;
background: white;
}

</style>
<link href='https://fonts.googleapis.com/css?family=Georgia|Open+Sans|Roboto' rel='stylesheet' type='text/css'>
<style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
</head>
<body>
<div style="height: 0vh">scroll</div>

<amp-img id="hero-img"
src="img/[email protected]"
srcset="img/[email protected] 1x, img/[email protected] 2x"
layout="responsive" width="360"
height="216">
</amp-img>

<amp-img id="ad-img"
src="img/[email protected]"
layout="responsive" width="360"
height="216"
xpri="2"
noprerender>
</amp-img>

<div style="height: 30vh">scroll</div>

<amp-img id="offscreen-img"
src="img/clock.jpg"
layout="responsive" width="360"
height="216">
</amp-img>

<div style="height: 2000vh">scroll</div>
</body>
</html>
29 changes: 29 additions & 0 deletions src/base-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,11 +415,39 @@ export class BaseElement {
* {@link layoutCallback} calls. Note that this method is not consulted for
* the first layout given that each element must be laid out at least once.
* @return {boolean}
* TODO(#31915): rename to isReloadNeeded
*/
isRelayoutNeeded() {
return false;
}

/**
* Whether this element supports loaderV2 scheduler.
* @return {boolean}
* TODO(#31915): rename to just isLoadable
*/
isLoadableV2() {
return false;
}

/**
* Called when the element should perform loading. At this point the element
* should load/reload resources associated with it. This method is called
* by the runtime and cannot be called manually. Returns promise that will
* complete when loading is considered to be complete.
*
* The first layout call is always called. If the subclass is interested in
* receiving additional callbacks, it has to opt in to do so using
* {@link isRelayoutNeeded} method.
*
* This method is only called if {@link isLoadableV2} returns `true`.
*
* @return {!Promise|undefind}
*/
loadCallback() {
Copy link
Member

@samouri samouri Jan 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that this (and build/layout) should not have callback in their names. I gave the full reasoning here #31046 (comment). The tldr is that we are asking an element to load as opposed to informing it that load has happened.

I understand the desire to match CE's naming scheme, but IMO it doesn't make sense for these methods

return this.layoutCallback();
}

/**
* Called when the element should perform layout. At this point the element
* should load/reload resources associated with it. This method is called
Expand All @@ -431,6 +459,7 @@ export class BaseElement {
* {@link isRelayoutNeeded} method.
*
* @return {!Promise}
* TODO(#31915): deprecate and remove when loaderV2 is fully launched.
*/
layoutCallback() {
return Promise.resolve();
Expand Down
65 changes: 49 additions & 16 deletions src/custom-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ const UpgradeState = {
UPGRADE_IN_PROGRESS: 4,
};

const EMPTY_FUNC = () => {};

/**
* Caches whether the template tag is supported to avoid memory allocations.
* @type {boolean|undefined}
Expand Down Expand Up @@ -340,6 +342,17 @@ function createBaseCustomElementClass(win) {
return this.upgradeDelayMs_;
}

/**
* Whether this element supports loaderV2 scheduler.
*
* @return {boolean}
* @final
* TODO(#31915): rename to just isLoadable
*/
isLoadableV2() {
return this.implementation_.isLoadableV2();
}

/**
* Completes the upgrade of the element with the provided implementation.
* @param {!./base-element.BaseElement} newImpl
Expand Down Expand Up @@ -396,6 +409,15 @@ function createBaseCustomElementClass(win) {
return this.signals_.whenSignal(CommonSignals.BUILT);
}

/**
* Returns the promise that's rewsolved when the element has been loaded.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rewsolved --> resolved

* If the loading fails, the resulting promise is rejected.
* @return {!Promise}
*/
whenLoaded() {
return this.signals_.whenSignal(CommonSignals.LOAD_END).then(EMPTY_FUNC);
}

/**
* Get the priority to load the element.
* @return {number}
Expand Down Expand Up @@ -497,6 +519,14 @@ function createBaseCustomElementClass(win) {
));
}

/**
* Forces immediate load of this element.
* @return {!Promise}
*/
load() {
return this.build().then(() => this.getResources().forceLoad(this));
}

/**
* Called to instruct the element to preconnect to hosts it uses during
* layout.
Expand Down Expand Up @@ -534,12 +564,10 @@ function createBaseCustomElementClass(win) {
/**
* Updates the layout box of the element.
* Should only be called by Resources.
* @param {!./layout-rect.LayoutRectDef} layoutBox
* @param {boolean} sizeChanged
*/
updateLayoutBox(layoutBox, sizeChanged = false) {
updateLayoutBox() {
if (this.isBuilt()) {
this.onMeasure(sizeChanged);
this.onMeasure();
}
}

Expand Down Expand Up @@ -877,6 +905,9 @@ function createBaseCustomElementClass(win) {
/** @private */
connected_() {
if (this.built_) {
if (this.isLoadableV2()) {
Copy link
Member

@samouri samouri Jan 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't one of the big points here that we don't need to branch on isBuilt() anymore? I.e. scheduleLoad can be extracted from the condition since the definition of load() is build().then(() => this.layout()))

this.getResources().scheduleLoad(this);
}
this.implementation_.attachedCallback();
}
}
Expand Down Expand Up @@ -1112,11 +1143,8 @@ function createBaseCustomElementClass(win) {
}

this.dispatchCustomEventForTesting(AmpEvents.LOAD_START);
const isLoadEvent = this.layoutCount_ == 0; // First layout is "load".
this.signals_.reset(CommonSignals.UNLOAD);
if (isLoadEvent) {
this.signals_.signal(CommonSignals.LOAD_START);
}
this.signals_.signal(CommonSignals.LOAD_START);

// Potentially start the loading indicator.
this.toggleLoading(true);
Expand All @@ -1130,9 +1158,10 @@ function createBaseCustomElementClass(win) {
if ((!getMode().test || signal) && signal.aborted) {
throw cancellation();
}
if (isLoadEvent) {
this.signals_.signal(CommonSignals.LOAD_END);
if (this.isLoadableV2()) {
this.getResources().unscheduleLoad(this);
}
this.signals_.signal(CommonSignals.LOAD_END);
this.readyState = 'complete';
this.layoutCount_++;
this.toggleLoading(false);
Expand All @@ -1148,13 +1177,14 @@ function createBaseCustomElementClass(win) {
if ((!getMode().test || signal) && signal.aborted) {
throw cancellation();
}
// add layoutCount_ by 1 despite load fails or not
if (isLoadEvent) {
this.signals_.rejectSignal(
CommonSignals.LOAD_END,
/** @type {!Error} */ (reason)
);
if (this.isLoadableV2()) {
this.getResources().unscheduleLoad(this);
}
// add layoutCount_ by 1 despite load fails or not
this.signals_.rejectSignal(
CommonSignals.LOAD_END,
/** @type {!Error} */ (reason)
);
this.layoutCount_++;
this.toggleLoading(false);
throw reason;
Expand Down Expand Up @@ -1226,6 +1256,9 @@ function createBaseCustomElementClass(win) {
const isReLayoutNeeded = this.implementation_.unlayoutCallback();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: isReLayoutNeeded --> isRelayoutNeeded

if (isReLayoutNeeded) {
this.reset_();
if (this.isLoadableV2()) {
this.getResources().scheduleLoad(this);
}
}
this.dispatchCustomEventForTesting(AmpEvents.UNLOAD);
return isReLayoutNeeded;
Expand Down
8 changes: 7 additions & 1 deletion src/preact/base-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,13 @@ export class PreactBaseElement extends AMP.BaseElement {
}

/** @override */
layoutCallback() {
isLoadableV2() {
const Ctor = this.constructor;
return Ctor['loadable'];
}

/** @override */
loadCallback() {
const Ctor = this.constructor;
if (!Ctor['loadable']) {
return super.layoutCallback();
Expand Down
Loading