From eecae1cbb2858202ac3738a19498348166070713 Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Mon, 15 Mar 2021 12:27:26 -0400 Subject: [PATCH 1/3] resources: remove intersect-resources experiment --- .../global-configs/canary-config.json | 1 - build-system/global-configs/prod-config.json | 1 - src/inabox/inabox-resources.js | 5 - src/service/mutator-impl.js | 35 +-- src/service/resource.js | 63 +---- src/service/resources-impl.js | 224 +++--------------- src/service/resources-interface.js | 8 +- test/integration/test-visibility-states.js | 16 +- test/unit/test-resource.js | 35 --- test/unit/test-resources.js | 97 +------- tools/experiments/experiments-config.js | 6 - 11 files changed, 61 insertions(+), 430 deletions(-) diff --git a/build-system/global-configs/canary-config.json b/build-system/global-configs/canary-config.json index dc321b811c6f..653f12d28031 100644 --- a/build-system/global-configs/canary-config.json +++ b/build-system/global-configs/canary-config.json @@ -10,7 +10,6 @@ "dfp-render-on-idle-cwv-exp": 1, "expand-json-targeting": 1, "flexAdSlots": 0.05, - "intersect-resources": 0, "ios-fixed-no-transfer": 1, "visibility-trigger-improvements": 1, "ads-initialIntersection": 1, diff --git a/build-system/global-configs/prod-config.json b/build-system/global-configs/prod-config.json index 14f377f35ebb..32ad5a4004da 100644 --- a/build-system/global-configs/prod-config.json +++ b/build-system/global-configs/prod-config.json @@ -9,7 +9,6 @@ "doubleclickSraReportExcludedBlock": 0.1, "expand-json-targeting": 1, "flexAdSlots": 0.05, - "intersect-resources": 0, "ios-fixed-no-transfer": 0, "visibility-trigger-improvements": 1, "layout-aspect-ratio-css": 0, diff --git a/src/inabox/inabox-resources.js b/src/inabox/inabox-resources.js index e6dc5e3a1ba8..a7ab6ca6683f 100644 --- a/src/inabox/inabox-resources.js +++ b/src/inabox/inabox-resources.js @@ -201,11 +201,6 @@ export class InaboxResources { return this.firstPassDone_.promise; } - /** @override */ - isIntersectionExperimentOn() { - return false; - } - /** * @private */ diff --git a/src/service/mutator-impl.js b/src/service/mutator-impl.js index 8c53d966ef00..fd3637bc6c44 100644 --- a/src/service/mutator-impl.js +++ b/src/service/mutator-impl.js @@ -58,9 +58,6 @@ export class MutatorImpl { this.activeHistory_.onFocus((element) => { this.checkPendingChangeSize_(element); }); - - /** @private @const {boolean} */ - this.intersect_ = this.resources_.isIntersectionExperimentOn(); } /** @override */ @@ -129,16 +126,12 @@ export class MutatorImpl { /** @override */ collapseElement(element) { - // With IntersectionObserver, no need to relayout or remeasure - // due to a single element collapse (similar to "relayout top"). - if (!this.intersect_) { - const box = this.viewport_.getLayoutRect(element); - if (box.width != 0 && box.height != 0) { - if (isExperimentOn(this.win, 'dirty-collapse-element')) { - this.dirtyElement(element); - } else { - this.resources_.setRelayoutTop(box.top); - } + const box = this.viewport_.getLayoutRect(element); + if (box.width != 0 && box.height != 0) { + if (isExperimentOn(this.win, 'dirty-collapse-element')) { + this.dirtyElement(element); + } else { + this.resources_.setRelayoutTop(box.top); } } @@ -147,9 +140,7 @@ export class MutatorImpl { // Unlike completeExpand(), there's no requestMeasure() call here that // requires another pass (with IntersectionObserver). - if (!this.intersect_) { - this.resources_.schedulePass(FOUR_FRAME_DELAY_); - } + this.resources_.schedulePass(FOUR_FRAME_DELAY_); } /** @override */ @@ -217,9 +208,8 @@ export class MutatorImpl { if (measurer) { measurer(); } - // With IntersectionObserver, "relayout top" is no longer needed since - // relative positional changes won't affect correctness. - if (!this.intersect_ && !skipRemeasure) { + + if (!skipRemeasure) { relayoutTop = calcRelayoutTop(); } }, @@ -243,12 +233,6 @@ export class MutatorImpl { } this.resources_.schedulePass(FOUR_FRAME_DELAY_); - // With IntersectionObserver, no need to set relayout top. - if (this.intersect_) { - this.resources_.maybeHeightChanged(); - return; - } - if (relayoutTop != -1) { this.resources_.setRelayoutTop(relayoutTop); } @@ -279,7 +263,6 @@ export class MutatorImpl { * @param {!Element} element */ dirtyElement(element) { - devAssert(!this.intersect_); let relayoutAll = false; const isAmpElement = element.classList.contains('i-amphtml-element'); if (isAmpElement) { diff --git a/src/service/resource.js b/src/service/resource.js index 410656401c81..664aeaac722e 100644 --- a/src/service/resource.js +++ b/src/service/resource.js @@ -222,9 +222,6 @@ export class Resource { /** @private {?Function} */ this.loadPromiseResolve_ = deferred.resolve; - /** @const @private {boolean} */ - this.intersect_ = resources.isIntersectionExperimentOn(); - // TODO(#30620): remove isInViewport_ and whenWithinViewport. /** @const @private {boolean} */ this.isInViewport_ = false; @@ -344,20 +341,7 @@ export class Resource { return this.element.buildInternal().then( () => { this.isBuilding_ = false; - // With IntersectionObserver, measure can happen before build - // so check if we're "ready for layout" (measured and built) here. - if (this.intersect_ && this.hasBeenMeasured()) { - this.state_ = ResourceState.READY_FOR_LAYOUT; - // The InOb premeasurement couldn't account for fixed position since - // implementation wasn't loaded yet. Perform a remeasure now that we know it - // is fixed. - if (this.element.isAlwaysFixed() && !this.isFixed_) { - this.requestMeasure(); - } - this.element.onMeasure(/* sizeChanged */ true); - } else { - this.state_ = ResourceState.NOT_LAID_OUT; - } + this.state_ = ResourceState.NOT_LAID_OUT; // TODO(dvoytenko): merge with the standard BUILT signal. this.element.signals().signal('res-built'); }, @@ -443,16 +427,6 @@ export class Resource { return this.element.getUpgradeDelayMs(); } - /** - * Stores a client rect that was "premeasured" by an IntersectionObserver. - * Should only be used in IntersectionObserver mode. - * @param {!ClientRect} clientRect - */ - premeasure(clientRect) { - devAssert(this.intersect_); - this.premeasuredRect_ = clientRect; - } - /** Removes the premeasured rect, likely forcing a manual measure. */ invalidatePremeasurementAndRequestMeasure() { this.premeasuredRect_ = null; @@ -631,14 +605,6 @@ export class Resource { return !!this.initialLayoutBox_; } - /** - * @return {boolean} - */ - hasBeenPremeasured() { - devAssert(this.intersect_); - return !!this.premeasuredRect_; - } - /** * Requests the element to be remeasured on the next pass. */ @@ -689,21 +655,16 @@ export class Resource { /** * Whether the resource is displayed, i.e. if it has non-zero width and * height. - * @param {boolean} usePremeasuredRect If true and a premeasured rect is - * available, use it. Otherwise, use the cached layout box. * @return {boolean} */ - isDisplayed(usePremeasuredRect = false) { + isDisplayed() { const isConnected = this.element.ownerDocument && this.element.ownerDocument.defaultView; if (!isConnected) { return false; } - devAssert(!usePremeasuredRect || this.intersect_); const isFluid = this.element.getLayout() == Layout.FLUID; - const box = usePremeasuredRect - ? devAssert(this.premeasuredRect_) - : this.getLayoutBox(); + const box = this.getLayoutBox(); const hasNonZeroSize = box.height > 0 && box.width > 0; return isFluid || hasNonZeroSize; } @@ -885,13 +846,9 @@ export class Resource { * Undoes `layoutScheduled`. */ layoutCanceled() { - if (this.intersect_) { - this.state_ = ResourceState.READY_FOR_LAYOUT; - } else { - this.state_ = this.hasBeenMeasured() - ? ResourceState.READY_FOR_LAYOUT - : ResourceState.NOT_LAID_OUT; - } + this.state_ = this.hasBeenMeasured() + ? ResourceState.READY_FOR_LAYOUT + : ResourceState.NOT_LAID_OUT; } /** @@ -1064,13 +1021,7 @@ export class Resource { this.setInViewport(false); if (this.element.unlayoutCallback()) { this.element.togglePlaceholder(true); - // With IntersectionObserver, the element won't receive another - // measurement if/when the document becomes active again. - // Therefore, its post-unlayout state must be READY_FOR_LAYOUT - // (built and measured) to become eligible for relayout later. - this.state_ = this.intersect_ - ? ResourceState.READY_FOR_LAYOUT - : ResourceState.NOT_LAID_OUT; + this.state_ = ResourceState.NOT_LAID_OUT; this.layoutCount_ = 0; this.layoutPromise_ = null; } diff --git a/src/service/resources-impl.js b/src/service/resources-impl.js index b3645a7ba6e1..258c41f6bba8 100644 --- a/src/service/resources-impl.js +++ b/src/service/resources-impl.js @@ -136,9 +136,6 @@ export class ResourcesImpl { /** @const @private {!Pass} */ this.remeasurePass_ = new Pass(this.win, () => { - // With IntersectionObserver, "remeasuring" hack no longer needed. - devAssert(!this.intersectionObserver_); - this.relayoutAll_ = true; this.schedulePass(); }); @@ -195,40 +192,6 @@ export class ResourcesImpl { this.ampdoc.getVisibilityState() ); - /** @private {?IntersectionObserver} */ - this.intersectionObserver_ = null; - - /** - * True if the callback for intersectionObserver_ has fired at least once. - * @private {boolean} - */ - this.intersectionObserverCallbackFired_ = false; - - // TODO(#33262): cleanup intersect-resources experiment. - if (false) { - const iframed = isIframed(this.win); - - // Classic IntersectionObserver doesn't support viewport tracking and - // rootMargin in x-origin iframes (#25428). As of 1/2020, only Chrome 81+ - // supports it via {root: document}, which throws on other browsers. - const root = /** @type {?Element} */ (this.ampdoc.isSingleDoc() && iframed - ? /** @type {*} */ (this.win.document) - : null); - try { - this.intersectionObserver_ = new IntersectionObserver( - (e) => this.intersect(e), - // rootMargin matches size of loadRect: (150vw 300vh) * 1.25. - {root, rootMargin: '250% 31.25%'} - ); - - // Wait for intersection callback instead of measuring all elements - // during the first pass. - this.relayoutAll_ = false; - } catch (e) { - dev().warn(TAG_, 'Falling back to classic Resources:', e); - } - } - // When user scrolling stops, run pass to check newly in-viewport elements. // When viewport is resized, we have to re-measure everything. this.viewport_.onChanged((event) => { @@ -239,13 +202,6 @@ export class ResourcesImpl { this.maybeChangeHeight_ = true; } - // Unfortunately, a viewport size change invalidates all premeasurements. - if (this.relayoutAll_ && this.intersectionObserver_) { - this.resources_.forEach((resource) => - resource.invalidatePremeasurementAndRequestMeasure() - ); - } - this.schedulePass(); }); this.viewport_.onScroll(() => { @@ -276,51 +232,13 @@ export class ResourcesImpl { this.rebuildDomWhenReady_(); - if (!this.intersectionObserver_) { - /** @private @const */ - this.throttledScroll_ = throttle(this.win, (e) => this.scrolled_(e), 250); - - listen(this.win.document, 'scroll', this.throttledScroll_, { - capture: true, - passive: true, - }); - } - } + /** @private @const */ + this.throttledScroll_ = throttle(this.win, (e) => this.scrolled_(e), 250); - /** @override */ - isIntersectionExperimentOn() { - return !!this.intersectionObserver_; - } - - /** - * @param {!Array} entries - * @visibleForTesting - */ - intersect(entries) { - devAssert(this.intersectionObserver_); - - if (getMode().localDev) { - const inside = []; - const outside = []; - entries.forEach((e) => { - const r = Resource.forElement(e.target); - (e.isIntersecting ? inside : outside).push({e, id: r.debugid}); - }); - dev().fine(TAG_, 'intersection', inside, outside); - } - - this.intersectionObserverCallbackFired_ = true; - - entries.forEach((entry) => { - const {boundingClientRect, target} = entry; - - const r = Resource.forElement(target); - // Strangely, JSC is missing x/y from typedefs of boundingClientRect - // despite it being a DOMRectReadOnly (ClientRect) by spec. - r.premeasure(/** @type {!ClientRect} */ (boundingClientRect)); + listen(this.win.document, 'scroll', this.throttledScroll_, { + capture: true, + passive: true, }); - - this.schedulePass(); } /** @private */ @@ -340,37 +258,34 @@ export class ResourcesImpl { ieIntrinsicCheckAndFix(this.win); - // With IntersectionObserver, no need for remeasuring hacks. - if (!this.intersectionObserver_) { - const fixPromise = ieMediaCheckAndFix(this.win); - const remeasure = () => this.remeasurePass_.schedule(); - if (fixPromise) { - fixPromise.then(remeasure); - } else { - // No promise means that there's no problem. - remeasure(); - } + const fixPromise = ieMediaCheckAndFix(this.win); + const remeasure = () => this.remeasurePass_.schedule(); + if (fixPromise) { + fixPromise.then(remeasure); + } else { + // No promise means that there's no problem. + remeasure(); + } - // Safari 10 and under incorrectly estimates font spacing for - // `@font-face` fonts. This leads to wild measurement errors. The best - // course of action is to remeasure everything on window.onload or font - // timeout (3s), whichever is earlier. This has to be done on the global - // window because this is where the fonts are always added. - // Unfortunately, `document.fonts.ready` cannot be used here due to - // https://bugs.webkit.org/show_bug.cgi?id=174030. - // See https://bugs.webkit.org/show_bug.cgi?id=174031 for more details. - Promise.race([ - loadPromise(this.win), - Services.timerFor(this.win).promise(3100), - ]).then(remeasure); - - // Remeasure the document when all fonts loaded. - if ( - this.win.document.fonts && - this.win.document.fonts.status != 'loaded' - ) { - this.win.document.fonts.ready.then(remeasure); - } + // Safari 10 and under incorrectly estimates font spacing for + // `@font-face` fonts. This leads to wild measurement errors. The best + // course of action is to remeasure everything on window.onload or font + // timeout (3s), whichever is earlier. This has to be done on the global + // window because this is where the fonts are always added. + // Unfortunately, `document.fonts.ready` cannot be used here due to + // https://bugs.webkit.org/show_bug.cgi?id=174030. + // See https://bugs.webkit.org/show_bug.cgi?id=174031 for more details. + Promise.race([ + loadPromise(this.win), + Services.timerFor(this.win).promise(3100), + ]).then(remeasure); + + // Remeasure the document when all fonts loaded. + if ( + this.win.document.fonts && + this.win.document.fonts.status != 'loaded' + ) { + this.win.document.fonts.ready.then(remeasure); } }); } @@ -424,13 +339,7 @@ export class ResourcesImpl { dev().fine(TAG_, 'resource added:', resource.debugid); } this.resources_.push(resource); - - if (this.intersectionObserver_) { - // The observer callback will schedule a pass to process this element. - this.intersectionObserver_.observe(element); - } else { - this.remeasurePass_.schedule(1000); - } + this.remeasurePass_.schedule(1000); } /** @@ -586,10 +495,6 @@ export class ResourcesImpl { if (resource.isBuilt()) { resource.pauseOnRemove(); } - if (this.intersectionObserver_) { - // TODO(willchou): Fix observe/unobserve/remeasure churn in reparenting. - this.intersectionObserver_.unobserve(resource.element); - } if (resource.getState() === ResourceState.LAYOUT_SCHEDULED) { resource.layoutCanceled(); @@ -784,10 +689,6 @@ export class ResourcesImpl { if ( this.documentReady_ && this.ampInitialized_ && - // With IntersectionObserver, elements are not measured until the first - // intersection callback. - (!this.intersectionObserver_ || - this.intersectionObserverCallbackFired_) && !this.ampdoc.signals().get(READY_SCAN_SIGNAL) ) { // This signal mainly signifies that most of elements have been measured @@ -1176,9 +1077,8 @@ export class ResourcesImpl { ) { this.buildOrScheduleBuildForResource_(r, /* checkForDupes */ true); } - if (this.intersectionObserver_) { - // Do nothing. - } else if ( + + if ( relayoutAll || !r.hasBeenMeasured() || // NOT_LAID_OUT is the state after build() but before measure(). @@ -1194,43 +1094,7 @@ export class ResourcesImpl { // Phase 2: Remeasure if there were any relayouts. Unfortunately, currently // there's no way to optimize this. All reads happen here. let toUnload; - if (this.intersectionObserver_) { - // The IntersectionObserver variant for phase 2 is just a simplification - // that ignores `relayoutTop` and `elementsThatScrolled`. - for (let i = 0; i < this.resources_.length; i++) { - const r = this.resources_[i]; - const requested = r.isMeasureRequested(); - if ((r.hasOwner() && !requested) || r.element.V1()) { - continue; - } - const premeasured = r.hasBeenPremeasured(); - if (requested) { - dev().fine(TAG_, 'force remeasure:', r.debugid); - } - // Immediately measure (vs. waiting for async premeasure) for certain - // elements with sensitive time-to-measure. - const expediteFirstMeasure = - !r.hasBeenMeasured() && r.element.tagName == 'AMP-AD'; - const needsMeasure = - premeasured || requested || relayoutAll || expediteFirstMeasure; - if (needsMeasure) { - const isDisplayed = this.measureResource_( - r, - /* usePremeasuredRect */ premeasured - ); - if (!isDisplayed) { - devAssert( - r.getState() != ResourceState.NOT_BUILT, - 'Should not unload unbuilt elements.' - ); - if (!toUnload) { - toUnload = []; - } - toUnload.push(r); - } - } - } - } else if ( + if ( relayoutCount > 0 || remeasureCount > 0 || relayoutAll || @@ -1461,19 +1325,9 @@ export class ResourcesImpl { const {resource} = task; let stillDisplayed = true; - if (this.intersectionObserver_) { - // With IntersectionObserver, peek at the premeasured rect to see - // if the resource is still displayed (has a non-zero size). - // The premeasured rect is most analogous to an immediate measure. - if (resource.hasBeenPremeasured()) { - stillDisplayed = resource.isDisplayed( - /* usePremeasuredRect */ true - ); - } - } else { - // Remeasure can only update isDisplayed(), not in-viewport state. - resource.measure(); - } + // Remeasure can only update isDisplayed(), not in-viewport state. + resource.measure(); + // Check if the element has exited the viewport or the page has changed // visibility since the layout was scheduled. if ( diff --git a/src/service/resources-interface.js b/src/service/resources-interface.js index d6a54a5e93c4..59ebd57dba8c 100644 --- a/src/service/resources-interface.js +++ b/src/service/resources-interface.js @@ -174,12 +174,6 @@ export class ResourcesInterface { * @param {!Element} element * @param {number} newLayoutPriority */ - updateLayoutPriority(element, newLayoutPriority) {} - - /** - * https://github.com/ampproject/amphtml/issues/25428 - * @return {boolean} - */ - isIntersectionExperimentOn() {} + updateLayoutPriority(element, newLayoutPriority) {} } /* eslint-enable no-unused-vars */ diff --git a/test/integration/test-visibility-states.js b/test/integration/test-visibility-states.js index df80519d9e09..30b29c36fbef 100644 --- a/test/integration/test-visibility-states.js +++ b/test/integration/test-visibility-states.js @@ -91,19 +91,9 @@ t.run('Viewer Visibility State', () => { function waitForNextPass() { return new Promise((resolve) => { - notifyPass = resolve; - - if (resources.isIntersectionExperimentOn()) { - // Element lifecycle callbacks depend on the observer taking its - // initial measurements, so wait for an intersection first. - return intersected.then(() => { - shouldPass = true; - resources.schedulePass(); - }); - } else { - shouldPass = true; - resources.schedulePass(); - } + notifyPass = resolve; + shouldPass = true; + resources.schedulePass(); }); } diff --git a/test/unit/test-resource.js b/test/unit/test-resource.js index 1f9fae74c52e..91df4a8c6336 100644 --- a/test/unit/test-resource.js +++ b/test/unit/test-resource.js @@ -107,41 +107,6 @@ describes.realWin('Resource', {amp: true}, (env) => { }); }); - describe('intersect-resources', () => { - beforeEach(() => { - sandbox.stub(resources, 'isIntersectionExperimentOn').returns(true); - resource = new Resource(1, element, resources); - }); - - it('should be ready for layout if measured before build', () => { - resource.premeasure({left: 0, top: 0, width: 100, height: 100}); - resource.measure(/* usePremeasuredRect */ true); - elementMock.expects('isUpgraded').returns(true).atLeast(1); - elementMock.expects('buildInternal').returns(Promise.resolve()).once(); - elementMock.expects('onMeasure').withArgs(/* sizeChanged */ true).once(); - return resource.build().then(() => { - expect(resource.getState()).to.equal(ResourceState.READY_FOR_LAYOUT); - }); - }); - - it('should remeasure if measured before upgrade and isFixed', () => { - // First measure - element.isAlwaysFixed = () => false; - resource.premeasure({left: 0, top: 0, width: 100, height: 100}); - resource.measure(/* usePremeasuredRect */ true); - - // Now adjust implementation to be alwaysFixed and call build. - element.isUpgraded = () => true; - element.isAlwaysFixed = () => true; - element.buildInternal = () => Promise.resolve(); - element.onMeasure = () => {}; - resource.requestMeasure = env.sandbox.stub(); - return resource.build().then(() => { - expect(resource.requestMeasure).calledOnce; - }); - }); - }); - it('should build if element is currently building', () => { elementMock.expects('isBuilt').returns(false).once(); elementMock.expects('isBuilding').returns(true).once(); diff --git a/test/unit/test-resources.js b/test/unit/test-resources.js index 0d8227053b54..e767679ac54a 100644 --- a/test/unit/test-resources.js +++ b/test/unit/test-resources.js @@ -722,45 +722,7 @@ describes.realWin('Resources discoverWork', {amp: true}, (env) => { resources.discoverWork_(); expect(resource1.hasBeenMeasured()).to.be.true; - }); - - describe('intersect-resources', () => { - beforeEach(() => { - // Enable "intersect-resources" experiment. - resources.intersectionObserver_ = {}; - resource1.intersect_ = resource2.intersect_ = true; - }); - - it('should not force relayout after build', () => { - resources.relayoutAll_ = false; - - // Unmeasured elements. - env.sandbox.stub(resource1, 'hasBeenMeasured').returns(false); - - // Measured elements that need relayout. - env.sandbox.stub(resource2, 'hasBeenMeasured').returns(true); - env.sandbox - .stub(resource2, 'getState') - .returns(ResourceState.NOT_LAID_OUT); - - resources.discoverWork_(); - }); - - it('should invalidate premeasurements after resize event', () => { - resource1.premeasure({}); - expect(resource1.hasBeenPremeasured()).true; - expect(resource1.isMeasureRequested()).false; - resources.viewport_.changeObservable_.fire({relayoutAll_: true}); - expect(resource1.hasBeenPremeasured()).false; - expect(resource1.isMeasureRequested()).true; - }); - - it('should schedule a pass after resize event', () => { - const schedulePassStub = sandbox.stub(resources, 'schedulePass'); - resources.viewport_.changeObservable_.fire({relayoutAll_: false}); - expect(schedulePassStub).calledOnce; - }); - }); + }); it('should render two screens when visible', () => { resources.visible_ = true; @@ -951,46 +913,7 @@ describes.realWin('Resources discoverWork', {amp: true}, (env) => { resources.work_(); expect(resource1.getState()).to.equal(ResourceState.LAYOUT_SCHEDULED); expect(resource1.element.layoutScheduleTime).to.be.greaterThan(0); - }); - - describe('intersect-resources', () => { - beforeEach(() => { - // Enables the "intersect-resources" experiment. - resources.intersectionObserver_ = {}; - }); - - it('should not remeasure before layout', () => { - sandbox.stub(resource1, 'hasBeenPremeasured').returns(false); - sandbox.stub(resource1, 'measure'); - - resources.scheduleLayoutOrPreload(resource1, /* layout */ true); - resources.work_(); - - expect(resources.exec_.getSize()).to.equal(1); - expect(resource1.measure).to.not.be.called; - expect(resource1.getState()).to.equal(ResourceState.LAYOUT_SCHEDULED); - }); - - it('should check premeasured rect before layout', () => { - sandbox.stub(resource1, 'hasBeenMeasured').returns(true); - sandbox.stub(resource1, 'hasBeenPremeasured').returns(true); - sandbox.stub(resource1, 'isDisplayed').returns(true); - resource1.isDisplayed - .withArgs(/* usePremeasuredRect */ true) - .returns(false); - sandbox.spy(resource1, 'layoutCanceled'); - - resources.scheduleLayoutOrPreload(resource1, /* layout */ true); - resources.work_(); - - expect(resources.exec_.getSize()).to.equal(0); - expect(resource1.isDisplayed).to.be.calledWith( - /* usePremeasuredRect */ true - ); - expect(resource1.layoutCanceled).to.be.calledOnce; - expect(resource1.getState()).to.equal(ResourceState.READY_FOR_LAYOUT); - }); - }); + }); it('should not schedule resource execution outside viewport', () => { resources.scheduleLayoutOrPreload(resource1, true); @@ -1518,22 +1441,6 @@ describes.fakeWin('Resources.add/upgrade/remove', {amp: true}, (env) => { } ); - it('should observe element after adding it', () => { - // Enables the 'intersect-resources' experiment. - const observer = (resources.intersectionObserver_ = { - observe: env.sandbox.spy(), - }); - // Avoid creating a new Resource, which is tricky to spy on. - env.sandbox.stub(child1, 'reconstructWhenReparented').returns(false); - env.sandbox.stub(resource1, 'getState').returns(ResourceState.NOT_LAID_OUT); - env.sandbox.spy(resource1, 'requestMeasure'); - - resources.add(child1); - - expect(resource1.requestMeasure).to.be.calledOnce; - expect(observer.observe).to.be.calledOnceWith(child1); - }); - describe('buildReadyResources_', () => { let schedulePassStub; diff --git a/tools/experiments/experiments-config.js b/tools/experiments/experiments-config.js index 06323fd0054d..c9929d3de478 100644 --- a/tools/experiments/experiments-config.js +++ b/tools/experiments/experiments-config.js @@ -161,12 +161,6 @@ export const EXPERIMENTS = [ spec: 'https://github.com/ampproject/amphtml/issues/23568', cleanupIssue: 'https://github.com/ampproject/amphtml/issues/24165', }, - { - id: 'intersect-resources', - name: 'Use IntersectionObserver for resource scheduling.', - spec: 'https://github.com/ampproject/amphtml/issues/25428', - cleanupIssue: 'https://github.com/ampproject/amphtml/issues/26233', - }, { id: 'amp-stream-gallery', name: 'Enables component', From 1df679346afccd763bb4a9f94062be3b3903a84e Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Thu, 18 Mar 2021 13:34:18 -0400 Subject: [PATCH 2/3] lint --- src/service/mutator-impl.js | 2 +- src/service/resources-impl.js | 5 ++--- src/service/resources-interface.js | 2 +- test/integration/test-visibility-states.js | 16 +--------------- test/unit/test-resource.js | 2 -- test/unit/test-resources.js | 4 ++-- 6 files changed, 7 insertions(+), 24 deletions(-) diff --git a/src/service/mutator-impl.js b/src/service/mutator-impl.js index fd3637bc6c44..e0b2a6a3d1ee 100644 --- a/src/service/mutator-impl.js +++ b/src/service/mutator-impl.js @@ -21,7 +21,7 @@ import {Services} from '../services'; import {areMarginsChanged} from '../layout-rect'; import {closest} from '../dom'; import {computedStyle} from '../style'; -import {dev, devAssert} from '../log'; +import {dev} from '../log'; import {isExperimentOn} from '../experiments'; import {registerServiceBuilderForDoc} from '../service'; diff --git a/src/service/resources-impl.js b/src/service/resources-impl.js index 258c41f6bba8..ad75d4c88ed1 100644 --- a/src/service/resources-impl.js +++ b/src/service/resources-impl.js @@ -27,9 +27,8 @@ import {VisibilityState} from '../visibility-state'; import {dev, devAssert} from '../log'; import {dict} from '../utils/object'; import {expandLayoutRect} from '../layout-rect'; -import {getMode} from '../mode'; import {getSourceUrl} from '../url'; -import {hasNextNodeInDocumentOrder, isIframed} from '../dom'; +import {hasNextNodeInDocumentOrder} from '../dom'; import {ieIntrinsicCheckAndFix} from './ie-intrinsic-bug'; import {ieMediaCheckAndFix} from './ie-media-bug'; import {isBlockedByConsent, reportError} from '../error'; @@ -1324,7 +1323,7 @@ export class ResourcesImpl { } else { const {resource} = task; - let stillDisplayed = true; + const stillDisplayed = true; // Remeasure can only update isDisplayed(), not in-viewport state. resource.measure(); diff --git a/src/service/resources-interface.js b/src/service/resources-interface.js index 59ebd57dba8c..300250716506 100644 --- a/src/service/resources-interface.js +++ b/src/service/resources-interface.js @@ -174,6 +174,6 @@ export class ResourcesInterface { * @param {!Element} element * @param {number} newLayoutPriority */ - updateLayoutPriority(element, newLayoutPriority) {} + updateLayoutPriority(element, newLayoutPriority) {} } /* eslint-enable no-unused-vars */ diff --git a/test/integration/test-visibility-states.js b/test/integration/test-visibility-states.js index 30b29c36fbef..f71b32e8a406 100644 --- a/test/integration/test-visibility-states.js +++ b/test/integration/test-visibility-states.js @@ -70,12 +70,8 @@ t.run('Viewer Visibility State', () => { let shouldPass = false; let doPass_; - let intersect_; let notifyPass = noop; - let intersected; - let notifyIntersected; - function doPass() { if (shouldPass) { doPass_.call(this); @@ -84,14 +80,9 @@ t.run('Viewer Visibility State', () => { } } - function intersect() { - intersect_.apply(this, arguments); - notifyIntersected(); - } - function waitForNextPass() { return new Promise((resolve) => { - notifyPass = resolve; + notifyPass = resolve; shouldPass = true; resources.schedulePass(); }); @@ -108,9 +99,6 @@ t.run('Viewer Visibility State', () => { win = env.win; notifyPass = noop; shouldPass = false; - intersected = new Promise((resolve) => { - notifyIntersected = resolve; - }); const vsync = Services.vsyncFor(win); env.sandbox.stub(vsync, 'mutate').callsFake((mutator) => { @@ -130,9 +118,7 @@ t.run('Viewer Visibility State', () => { resources = Services.resourcesForDoc(win.document); doPass_ = resources.doPass; - intersect_ = resources.intersect; env.sandbox.stub(resources, 'doPass').callsFake(doPass); - env.sandbox.stub(resources, 'intersect').callsFake(intersect); const img = win.document.createElement('amp-img'); img.setAttribute('width', 100); diff --git a/test/unit/test-resource.js b/test/unit/test-resource.js index 91df4a8c6336..ca781e4ed472 100644 --- a/test/unit/test-resource.js +++ b/test/unit/test-resource.js @@ -29,11 +29,9 @@ describes.realWin('Resource', {amp: true}, (env) => { let elementMock; let resources; let resource; - let sandbox; beforeEach(() => { win = env.win; - sandbox = env.sandbox; doc = win.document; element = env.createAmpElement('amp-fake-element'); diff --git a/test/unit/test-resources.js b/test/unit/test-resources.js index e767679ac54a..9ce59e785570 100644 --- a/test/unit/test-resources.js +++ b/test/unit/test-resources.js @@ -722,7 +722,7 @@ describes.realWin('Resources discoverWork', {amp: true}, (env) => { resources.discoverWork_(); expect(resource1.hasBeenMeasured()).to.be.true; - }); + }); it('should render two screens when visible', () => { resources.visible_ = true; @@ -913,7 +913,7 @@ describes.realWin('Resources discoverWork', {amp: true}, (env) => { resources.work_(); expect(resource1.getState()).to.equal(ResourceState.LAYOUT_SCHEDULED); expect(resource1.element.layoutScheduleTime).to.be.greaterThan(0); - }); + }); it('should not schedule resource execution outside viewport', () => { resources.scheduleLayoutOrPreload(resource1, true); From 11355e5c50cae00eaba3cb7805357e763bd58d50 Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Mon, 22 Mar 2021 15:53:27 -0400 Subject: [PATCH 3/3] remove premeasuredRect flows --- src/service/resource.js | 28 +++---------------- src/service/resources-impl.js | 5 ++-- src/service/viewport/viewport-binding-def.js | 8 +----- .../viewport-binding-ios-embed-wrapper.js | 4 +-- .../viewport/viewport-binding-natural.js | 4 +-- src/service/viewport/viewport-impl.js | 11 ++------ src/service/viewport/viewport-interface.js | 4 +-- 7 files changed, 15 insertions(+), 49 deletions(-) diff --git a/src/service/resource.js b/src/service/resource.js index 664aeaac722e..26d250fd53a9 100644 --- a/src/service/resource.js +++ b/src/service/resource.js @@ -225,12 +225,6 @@ export class Resource { // TODO(#30620): remove isInViewport_ and whenWithinViewport. /** @const @private {boolean} */ this.isInViewport_ = false; - - /** - * A client rect that was "premeasured" by an IntersectionObserver. - * @private {?ClientRect} - */ - this.premeasuredRect_ = null; } /** @@ -427,19 +421,11 @@ export class Resource { return this.element.getUpgradeDelayMs(); } - /** Removes the premeasured rect, likely forcing a manual measure. */ - invalidatePremeasurementAndRequestMeasure() { - this.premeasuredRect_ = null; - this.requestMeasure(); - } - /** * Measures the resource's boundaries. An upgraded element will be * transitioned to the "ready for layout" state. - * @param {boolean} usePremeasuredRect If true, consumes the previously - * premeasured ClientRect instead of calling getBoundingClientRect(). */ - measure(usePremeasuredRect = false) { + measure() { // Check if the element is ready to be measured. // Placeholders are special. They are technically "owned" by parent AMP // elements, sized by parents, but laid out independently. This means @@ -470,12 +456,7 @@ export class Resource { this.isMeasureRequested_ = false; const oldBox = this.layoutBox_; - if (usePremeasuredRect) { - this.computeMeasurements_(devAssert(this.premeasuredRect_)); - } else { - this.computeMeasurements_(); - } - this.premeasuredRect_ = null; + this.computeMeasurements_(); const newBox = this.layoutBox_; // Note that "left" doesn't affect readiness for the layout. @@ -521,12 +502,11 @@ export class Resource { /** * Computes the current layout box and position-fixed state of the element. - * @param {!ClientRect=} opt_premeasuredRect * @private */ - computeMeasurements_(opt_premeasuredRect) { + computeMeasurements_() { const viewport = Services.viewportForDoc(this.element); - this.layoutBox_ = viewport.getLayoutRect(this.element, opt_premeasuredRect); + this.layoutBox_ = viewport.getLayoutRect(this.element); // Calculate whether the element is currently is or in `position:fixed`. let isFixed = false; diff --git a/src/service/resources-impl.js b/src/service/resources-impl.js index ad75d4c88ed1..f084bfba0844 100644 --- a/src/service/resources-impl.js +++ b/src/service/resources-impl.js @@ -1010,13 +1010,12 @@ export class ResourcesImpl { * Always returns true unless the resource was previously displayed but is * not displayed now (i.e. the resource should be unloaded). * @param {!Resource} r - * @param {boolean} usePremeasuredRect * @return {boolean} * @private */ - measureResource_(r, usePremeasuredRect = false) { + measureResource_(r) { const wasDisplayed = r.isDisplayed(); - r.measure(usePremeasuredRect); + r.measure(); return !(wasDisplayed && !r.isDisplayed()); } diff --git a/src/service/viewport/viewport-binding-def.js b/src/service/viewport/viewport-binding-def.js index c5113de02d80..16a90d1eabdf 100644 --- a/src/service/viewport/viewport-binding-def.js +++ b/src/service/viewport/viewport-binding-def.js @@ -179,15 +179,9 @@ export class ViewportBindingDef { * pass in, if they cached these values and would like to avoid * remeasure. Requires appropriate updating the values on scroll. * @param {number=} unusedScrollTop Same comment as above. - * @param {!ClientRect=} unusedPremeasuredRect * @return {!../../layout-rect.LayoutRectDef} */ - getLayoutRect( - unusedEl, - unusedScrollLeft, - unusedScrollTop, - unusedPremeasuredRect - ) {} + getLayoutRect(unusedEl, unusedScrollLeft, unusedScrollTop) {} /** * Returns the client rect of the current window. diff --git a/src/service/viewport/viewport-binding-ios-embed-wrapper.js b/src/service/viewport/viewport-binding-ios-embed-wrapper.js index 8426c0c91823..9f111d3689d9 100644 --- a/src/service/viewport/viewport-binding-ios-embed-wrapper.js +++ b/src/service/viewport/viewport-binding-ios-embed-wrapper.js @@ -272,8 +272,8 @@ export class ViewportBindingIosEmbedWrapper_ { contentHeightChanged() {} /** @override */ - getLayoutRect(el, opt_scrollLeft, opt_scrollTop, opt_premeasuredRect) { - const b = opt_premeasuredRect || el./*OK*/ getBoundingClientRect(); + getLayoutRect(el, opt_scrollLeft, opt_scrollTop) { + const b = el./*OK*/ getBoundingClientRect(); const scrollTop = opt_scrollTop != undefined ? opt_scrollTop : this.getScrollTop(); const scrollLeft = diff --git a/src/service/viewport/viewport-binding-natural.js b/src/service/viewport/viewport-binding-natural.js index afa1890f3397..a7b8e76ecf85 100644 --- a/src/service/viewport/viewport-binding-natural.js +++ b/src/service/viewport/viewport-binding-natural.js @@ -239,8 +239,8 @@ export class ViewportBindingNatural_ { } /** @override */ - getLayoutRect(el, opt_scrollLeft, opt_scrollTop, opt_premeasuredRect) { - const b = opt_premeasuredRect || el./*OK*/ getBoundingClientRect(); + getLayoutRect(el, opt_scrollLeft, opt_scrollTop) { + const b = el./*OK*/ getBoundingClientRect(); const scrollTop = opt_scrollTop != undefined ? opt_scrollTop : this.getScrollTop(); const scrollLeft = diff --git a/src/service/viewport/viewport-impl.js b/src/service/viewport/viewport-impl.js index e348e942372c..893b25d6a030 100644 --- a/src/service/viewport/viewport-impl.js +++ b/src/service/viewport/viewport-impl.js @@ -354,14 +354,14 @@ export class ViewportImpl { } /** @override */ - getLayoutRect(el, opt_premeasuredRect) { + getLayoutRect(el) { const scrollLeft = this.getScrollLeft(); const scrollTop = this.getScrollTop(); // Go up the window hierarchy through friendly iframes. const frameElement = getParentWindowFrameElement(el, this.ampdoc.win); if (frameElement) { - const b = this.binding_.getLayoutRect(el, 0, 0, opt_premeasuredRect); + const b = this.binding_.getLayoutRect(el, 0, 0); const c = this.binding_.getLayoutRect( frameElement, scrollLeft, @@ -375,12 +375,7 @@ export class ViewportImpl { ); } - return this.binding_.getLayoutRect( - el, - scrollLeft, - scrollTop, - opt_premeasuredRect - ); + return this.binding_.getLayoutRect(el, scrollLeft, scrollTop); } /** @override */ diff --git a/src/service/viewport/viewport-interface.js b/src/service/viewport/viewport-interface.js index 618fe1709881..f0067dd29cfb 100644 --- a/src/service/viewport/viewport-interface.js +++ b/src/service/viewport/viewport-interface.js @@ -144,11 +144,9 @@ export class ViewportInterface extends Disposable { * Note that this function should be called in vsync measure. Please consider * using `getClientRectAsync` instead. * @param {!Element} el - * @param {!ClientRect=} opt_premeasuredRect If provided, use this - * premeasured ClientRect instead of calling getBoundingClientRect. * @return {!../../layout-rect.LayoutRectDef} */ - getLayoutRect(el, opt_premeasuredRect) {} + getLayoutRect(el) {} /** * Returns the clientRect of the element.