Skip to content

Commit

Permalink
Allow amp-experiment to prerender and bypass maximum build limit (#24745
Browse files Browse the repository at this point in the history
)

* allow amp-experiment to prerender

* rename
  • Loading branch information
zhouyx committed Sep 26, 2019
1 parent fb91bb2 commit e451f48
Show file tree
Hide file tree
Showing 8 changed files with 99 additions and 1 deletion.
13 changes: 13 additions & 0 deletions build-system/tasks/presubmit-checks.js
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,19 @@ const forbiddenTerms = {
'src/inabox/amp-inabox.js',
],
},
'isBuildRenderBlocking': {
message:
'This is a protected API. Please only override it the element is ' +
'render blocking',
whitelist: [
'src/service/resources-impl.js',
'src/service/resource.js',
'src/custom-element.js',
'src/base-element.js',
'extensions/amp-experiment/0.1/amp-experiment.js',
'extensions/amp-experiment/1.0/amp-experiment.js',
],
},
'^describe[\\.|\\(|$]': {
message:
'Top-level "describe" blocks in test files have been deprecated. ' +
Expand Down
17 changes: 17 additions & 0 deletions extensions/amp-experiment/0.1/amp-experiment.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,23 @@ export class AmpExperiment extends AMP.BaseElement {
return layout == Layout.NODISPLAY || layout == Layout.CONTAINER;
}

/** @override */
prerenderAllowed() {
/*
* Prerender is allowed because the client_id is only used to calculate
* the variant bucket.
* In the case where a client_id is first generated
* during prerender, the base cid will be stored in the AMP viewer domain.
*/
return true;
}

/** @override */
isBuildRenderBlocking() {
// variantService is render blocking
return true;
}

/** @override */
buildCallback() {
return getServicePromiseForDoc(this.getAmpDoc(), 'variant').then(
Expand Down
17 changes: 17 additions & 0 deletions extensions/amp-experiment/1.0/amp-experiment.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,23 @@ export class AmpExperiment extends AMP.BaseElement {
return layout == Layout.NODISPLAY || layout == Layout.CONTAINER;
}

/** @override */
prerenderAllowed() {
/*
* Prerender is allowed because the client_id is only used to calculate
* the variant bucket.
* In the case where a client_id is first generated
* during prerender, the base cid will be stored in the AMP viewer domain.
*/
return true;
}

/** @override */
isBuildRenderBlocking() {
// variantService is render blocking
return true;
}

/** @override */
buildCallback() {
const buildCallbackPromises = [
Expand Down
12 changes: 12 additions & 0 deletions src/base-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,18 @@ export class BaseElement {
return false;
}

/**
* Subclasses can override this method to indicate that it is has
* render-blocking service.
*
* The return value of this function is used to determine if the element
* built _and_ laid out will be prioritized.
* @return {boolean}
*/
isBuildRenderBlocking() {
return false;
}

/**
* Subclasses can override this method to create a dynamic placeholder
* element and return it to be appended to the element. This will only
Expand Down
9 changes: 9 additions & 0 deletions src/custom-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -1013,6 +1013,15 @@ function createBaseCustomElementClass(win) {
return this.implementation_.prerenderAllowed();
}

/**
* Whether the element has render-blocking service.
* @return {boolean}
* @final
*/
isBuildRenderBlocking() {
return this.implementation_.isBuildRenderBlocking();
}

/**
* Creates a placeholder for the element.
* @return {?Element}
Expand Down
8 changes: 8 additions & 0 deletions src/service/resource.js
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,14 @@ export class Resource {
return this.element.prerenderAllowed();
}

/**
* Whether this element has render-blocking service.
* @return {boolean}
*/
isBuildRenderBlocking() {
return this.element.isBuildRenderBlocking();
}

/**
* @param {number|boolean} viewport derived from renderOutsideViewport.
* @return {!Promise} resolves when underlying element is built and within the
Expand Down
6 changes: 5 additions & 1 deletion src/service/resources-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,11 @@ export class ResourcesImpl {
* @private
*/
buildResourceUnsafe_(resource, schedulePass, force = false) {
if (!this.isUnderBuildQuota_() && !force) {
if (
!this.isUnderBuildQuota_() &&
!force &&
!resource.isBuildRenderBlocking()
) {
return null;
}
const promise = resource.build();
Expand Down
18 changes: 18 additions & 0 deletions test/unit/test-resources.js
Original file line number Diff line number Diff line change
Expand Up @@ -1110,13 +1110,31 @@ describes.realWin('Resources discoverWork', {amp: true}, env => {
resource1.element.isBuilt = () => false;
resource1.element.idleRenderOutsideViewport = () => true;
resource1.prerenderAllowed = () => true;
resource1.isBuildRenderBlocking = () => false;
resource1.state_ = ResourceState.NOT_BUILT;
resource1.build = sandbox.spy();

resources.buildOrScheduleBuildForResource_(resource1);
expect(resource1.build).to.not.be.called;
});

it('should build render blocking resource even if quota is reached', () => {
sandbox.stub(resources.ampdoc, 'hasBeenVisible').callsFake(() => false);
sandbox.stub(resources, 'schedule_');
resources.documentReady_ = true;
resources.buildAttemptsCount_ = 21; // quota is 20

resource1.element.isBuilt = () => false;
resource1.element.idleRenderOutsideViewport = () => true;
resource1.prerenderAllowed = () => true;
resource1.isBuildRenderBlocking = () => true;
resource1.state_ = ResourceState.NOT_BUILT;
resource1.build = sandbox.spy();

resources.buildOrScheduleBuildForResource_(resource1);
expect(resource1.build).to.be.called;
});

it('should layout resource if outside viewport but idle', () => {
const schedulePassStub = sandbox.stub(resources, 'schedulePass');
resources.documentReady_ = true;
Expand Down

0 comments on commit e451f48

Please sign in to comment.