Skip to content

Commit

Permalink
Revert "🚮 Remove idleRenderOutsideViewport (#28043)" (#28306)
Browse files Browse the repository at this point in the history
This reverts commit ab03e7c.

(cherry picked from commit ceef8ee)
  • Loading branch information
William Chou authored and gmajoulet committed May 11, 2020
1 parent 5d859d4 commit a199ce9
Show file tree
Hide file tree
Showing 8 changed files with 297 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,51 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
this.inZIndexHoldBack_ = false;
}

/**
* @return {number|boolean} render on idle configuration with false
* indicating disabled.
* @private
*/
getIdleRenderEnabled_() {
if (this.isIdleRender_) {
return this.isIdleRender_;
}
// Disable if publisher has indicated a non-default loading strategy.
if (this.element.getAttribute('data-loading-strategy')) {
return false;
}
const expVal = this.postAdResponseExperimentFeatures['render-idle-vp'];
const vpRange = parseInt(expVal, 10);
if (expVal && isNaN(vpRange)) {
// holdback branch sends non-numeric value.
return false;
}
return vpRange || 12;
}

/** @override */
idleRenderOutsideViewport() {
const vpRange = this.getIdleRenderEnabled_();
if (vpRange === false) {
return vpRange;
}
const renderOutsideViewport = this.renderOutsideViewport();
// False will occur when throttle in effect.
if (typeof renderOutsideViewport === 'boolean') {
return renderOutsideViewport;
}
this.isIdleRender_ = true;
// NOTE(keithwrightbos): handle race condition where previous
// idleRenderOutsideViewport marked slot as idle render despite never
// being schedule due to being beyond viewport max offset. If slot
// comes within standard outside viewport range, then ensure throttling
// will not be applied.
this.getResource()
.whenWithinViewport(renderOutsideViewport)
.then(() => (this.isIdleRender_ = false));
return vpRange;
}

/** @override */
isLayoutSupported(layout) {
this.isFluidPrimaryRequest_ = layout == Layout.FLUID;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1725,6 +1725,100 @@ describes.realWin(
});
});

describe('#idleRenderOutsideViewport', () => {
beforeEach(() => {
element = createElementWithAttributes(doc, 'amp-ad', {
'width': '200',
'height': '50',
'type': 'doubleclick',
});
impl = new AmpAdNetworkDoubleclickImpl(element);
env.sandbox
.stub(impl, 'getResource')
.returns({whenWithinViewport: () => Promise.resolve()});
});

it('should use experiment value', () => {
impl.postAdResponseExperimentFeatures['render-idle-vp'] = '4';
expect(impl.idleRenderOutsideViewport()).to.equal(4);
expect(impl.isIdleRender_).to.be.true;
});

it('should return false if using loading strategy', () => {
impl.postAdResponseExperimentFeatures['render-idle-vp'] = '4';
impl.element.setAttribute(
'data-loading-strategy',
'prefer-viewability-over-views'
);
expect(impl.idleRenderOutsideViewport()).to.be.false;
expect(impl.isIdleRender_).to.be.false;
});

it('should return false if invalid experiment value', () => {
impl.postAdResponseExperimentFeatures['render-idle-vp'] = 'abc';
expect(impl.idleRenderOutsideViewport()).to.be.false;
});

it('should return 12 if no experiment header', () => {
expect(impl.idleRenderOutsideViewport()).to.equal(12);
});

it('should return renderOutsideViewport boolean', () => {
env.sandbox.stub(impl, 'renderOutsideViewport').returns(false);
expect(impl.idleRenderOutsideViewport()).to.be.false;
});
});

describe('idle renderNonAmpCreative', () => {
beforeEach(() => {
element = createElementWithAttributes(doc, 'amp-ad', {
'width': '200',
'height': '50',
'type': 'doubleclick',
});
impl = new AmpAdNetworkDoubleclickImpl(element);
impl.postAdResponseExperimentFeatures['render-idle-vp'] = '4';
impl.postAdResponseExperimentFeatures['render-idle-throttle'] = 'true';
env.sandbox
.stub(AmpA4A.prototype, 'renderNonAmpCreative')
.returns(Promise.resolve());
});

// TODO(jeffkaufman, #13422): this test was silently failing
it.skip('should throttle if idle render and non-AMP creative', () => {
impl.win['3pla'] = 1;
const startTime = Date.now();
return impl.renderNonAmpCreative().then(() => {
expect(Date.now() - startTime).to.be.at.least(1000);
});
});

it('should NOT throttle if idle experiment not enabled', () => {
impl.win['3pla'] = 1;
delete impl.postAdResponseExperimentFeatures['render-idle-vp'];
const startTime = Date.now();
return impl.renderNonAmpCreative().then(() => {
expect(Date.now() - startTime).to.be.at.most(50);
});
});

it('should NOT throttle if experiment throttle not enabled', () => {
impl.win['3pla'] = 1;
const startTime = Date.now();
return impl.renderNonAmpCreative().then(() => {
expect(Date.now() - startTime).to.be.at.most(50);
});
});

it('should NOT throttle if idle render and no previous', () => {
impl.win['3pla'] = 0;
const startTime = Date.now();
return impl.renderNonAmpCreative().then(() => {
expect(Date.now() - startTime).to.be.at.most(50);
});
});
});

describe('#preconnect', () => {
beforeEach(() => {
element = createElementWithAttributes(doc, 'amp-ad', {
Expand Down
11 changes: 11 additions & 0 deletions src/base-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,17 @@ export class BaseElement {
return getMode(this.win).runtime == 'inabox' || 3;
}

/**
* Allows for rendering outside of the constraint set by renderOutsideViewport
* so long task scheduler is idle. Integer values less than those returned
* by renderOutsideViewport have no effect. Subclasses can override (default
* is disabled).
* @return {boolean|number}
*/
idleRenderOutsideViewport() {
return false;
}

/**
* Subclasses can override this method to opt-in into receiving additional
* {@link layoutCallback} calls. Note that this method is not consulted for
Expand Down
10 changes: 10 additions & 0 deletions src/custom-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -1033,6 +1033,16 @@ function createBaseCustomElementClass(win) {
return this.implementation_.renderOutsideViewport();
}

/**
* Whether the element should render outside of renderOutsideViewport when
* the scheduler is idle.
* @return {boolean|number}
* @final
*/
idleRenderOutsideViewport() {
return this.implementation_.idleRenderOutsideViewport();
}

/**
* Returns a previously measured layout box adjusted to the viewport. This
* mainly affects fixed-position elements that are adjusted to be always
Expand Down
9 changes: 9 additions & 0 deletions src/service/resource.js
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,15 @@ export class Resource {
);
}

/**
* Whether this is allowed to render when scheduler is idle but not in
* viewport.
* @return {boolean}
*/
idleRenderOutsideViewport() {
return this.isWithinViewportRatio(this.element.idleRenderOutsideViewport());
}

/**
* Sets the resource's state to LAYOUT_SCHEDULED.
* @param {number} scheduleTime The time at which layout was scheduled.
Expand Down
24 changes: 22 additions & 2 deletions src/service/resources-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -1349,8 +1349,27 @@ export class ResourcesImpl {
this.queue_.getSize() == 0 &&
now > this.exec_.getLastDequeueTime() + 5000
) {
// Phase 5: Idle Render Outside Viewport layout: layout up to 4 items
// with idleRenderOutsideViewport true
let idleScheduledCount = 0;
// Phase 5: Idle layout: layout more if we are otherwise not doing much.
for (
let i = 0;
i < this.resources_.length && idleScheduledCount < 4;
i++
) {
const r = this.resources_[i];
if (
r.getState() == ResourceState.READY_FOR_LAYOUT &&
!r.hasOwner() &&
r.isDisplayed() &&
r.idleRenderOutsideViewport()
) {
dev().fine(TAG_, 'idleRenderOutsideViewport layout:', r.debugid);
this.scheduleLayoutOrPreload(r, /* layout */ false);
idleScheduledCount++;
}
}
// Phase 6: Idle layout: layout more if we are otherwise not doing much.
// TODO(dvoytenko): document/estimate IDLE timeouts and other constants
for (
let i = 0;
Expand Down Expand Up @@ -1628,7 +1647,8 @@ export class ResourcesImpl {
if (
!forceOutsideViewport &&
!resource.isInViewport() &&
!resource.renderOutsideViewport()
!resource.renderOutsideViewport() &&
!resource.idleRenderOutsideViewport()
) {
return false;
}
Expand Down
53 changes: 53 additions & 0 deletions test/unit/test-resource.js
Original file line number Diff line number Diff line change
Expand Up @@ -1010,6 +1010,59 @@ describes.realWin('Resource', {amp: true}, (env) => {
});
});

describe('Resource idleRenderOutsideViewport', () => {
let element;
let resources;
let resource;
let idleRenderOutsideViewport;
let isWithinViewportRatio;

beforeEach(() => {
idleRenderOutsideViewport = window.sandbox.stub();
element = {
idleRenderOutsideViewport,
ownerDocument: {defaultView: window},
tagName: 'AMP-AD',
hasAttribute: () => false,
isBuilt: () => false,
isBuilding: () => false,
isUpgraded: () => false,
prerenderAllowed: () => false,
renderOutsideViewport: () => true,
build: () => false,
getBoundingClientRect: () => null,
updateLayoutBox: () => {},
isRelayoutNeeded: () => false,
layoutCallback: () => {},
applySize: () => {},
unlayoutOnPause: () => false,
unlayoutCallback: () => true,
pauseCallback: () => false,
resumeCallback: () => false,
viewportCallback: () => {},
getLayoutPriority: () => LayoutPriority.CONTENT,
};
resources = new ResourcesImpl(new AmpDocSingle(window));
resource = new Resource(1, element, resources);
isWithinViewportRatio = window.sandbox.stub(
resource,
'isWithinViewportRatio'
);
});

it('should return true if isWithinViewportRatio', () => {
idleRenderOutsideViewport.returns(5);
isWithinViewportRatio.withArgs(5).returns(true);
expect(resource.idleRenderOutsideViewport()).to.equal(true);
});

it('should return false for false element idleRenderOutsideViewport', () => {
idleRenderOutsideViewport.returns(false);
isWithinViewportRatio.withArgs(false).returns(false);
expect(resource.idleRenderOutsideViewport()).to.equal(false);
});
});

describes.realWin('Resource renderOutsideViewport', {amp: true}, (env) => {
let element;
let resources;
Expand Down

0 comments on commit a199ce9

Please sign in to comment.