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.

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

Already on GitHub? Sign in to your account

Revert "馃毊 Remove idleRenderOutsideViewport" #28306

Merged
merged 1 commit into from
May 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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