From b35ffd4f08813c973120ccdab83fd75ef7e8e77f Mon Sep 17 00:00:00 2001 From: zhouyx Date: Wed, 26 Jul 2017 14:14:00 -0700 Subject: [PATCH 1/2] clean-sticky-ad-early-load-flag --- extensions/amp-sticky-ad/1.0/amp-sticky-ad.js | 14 +----- .../1.0/test/test-amp-sticky-ad.js | 45 +------------------ tools/experiments/experiments.js | 6 --- 3 files changed, 3 insertions(+), 62 deletions(-) diff --git a/extensions/amp-sticky-ad/1.0/amp-sticky-ad.js b/extensions/amp-sticky-ad/1.0/amp-sticky-ad.js index 347dada9d1c4..3d7ea1d5e342 100644 --- a/extensions/amp-sticky-ad/1.0/amp-sticky-ad.js +++ b/extensions/amp-sticky-ad/1.0/amp-sticky-ad.js @@ -20,16 +20,12 @@ import {Layout} from '../../../src/layout'; import {dev,user} from '../../../src/log'; import {removeElement} from '../../../src/dom'; import {toggle, computedStyle} from '../../../src/style'; -import {isExperimentOn} from '../../../src/experiments'; import { setStyle, removeAlphaFromColor, } from '../../../src/style'; import {whenUpgradedToCustomElement} from '../../../src/dom'; -/** @const */ -const EARLY_LOAD_EXPERIMENT = 'sticky-ad-early-load'; - class AmpStickyAd extends AMP.BaseElement { /** @param {!AmpElement} element */ constructor(element) { @@ -140,14 +136,8 @@ class AmpStickyAd extends AMP.BaseElement { */ onScroll_() { const scrollTop = this.viewport_.getScrollTop(); - if (isExperimentOn(this.win, EARLY_LOAD_EXPERIMENT) && scrollTop > 1) { - this.display_(); - return; - } - - const viewportHeight = this.viewport_.getSize().height; - // Check user has scrolled at least one viewport from init position. - if (scrollTop > viewportHeight) { + if (scrollTop > 1) { + // Check greater than 1 because AMP set scrollTop to 1 in iOS. this.display_(); } } diff --git a/extensions/amp-sticky-ad/1.0/test/test-amp-sticky-ad.js b/extensions/amp-sticky-ad/1.0/test/test-amp-sticky-ad.js index 494e805b4380..4b9825ec787c 100644 --- a/extensions/amp-sticky-ad/1.0/test/test-amp-sticky-ad.js +++ b/extensions/amp-sticky-ad/1.0/test/test-amp-sticky-ad.js @@ -18,7 +18,6 @@ import '../amp-sticky-ad'; import '../../../amp-ad/0.1/amp-ad'; import {poll} from '../../../../testing/iframe'; -import {toggleExperiment} from '../../../../src/experiments'; describes.realWin('amp-sticky-ad 1.0 version', { win: { /* window spec */ @@ -85,48 +84,7 @@ describes.realWin('amp-sticky-ad 1.0 version', { }); }); - it('should build on enough scroll dist, one more viewport ahead', () => { - const scheduleLayoutSpy = sandbox.stub(impl, 'scheduleLayoutForAd_', - () => {}); - const removeOnScrollListenerSpy = - sandbox.spy(impl, 'removeOnScrollListener_'); - const getScrollTopSpy = sandbox.spy(); - const getSizeSpy = sandbox.spy(); - const getScrollHeightSpy = sandbox.spy(); - - impl.viewport_.getScrollTop = function() { - getScrollTopSpy(); - return 100; - }; - impl.viewport_.getSize = function() { - getSizeSpy(); - return {height: 50}; - }; - impl.viewport_.getScrollHeight = function() { - getScrollHeightSpy(); - return 300; - }; - impl.deferMutate = function(callback) { - callback(); - }; - impl.vsync_.mutate = function(callback) { - callback(); - }; - - impl.onScroll_(); - expect(getScrollTopSpy).to.have.been.called; - expect(getSizeSpy).to.have.been.called; - expect(removeOnScrollListenerSpy).to.have.been.called; - // Layout on ad is called only after fixed layer is done. - expect(scheduleLayoutSpy).to.not.have.been.called; - expect(addToFixedLayerStub).to.have.been.calledOnce; - return addToFixedLayerPromise.then(() => { - expect(scheduleLayoutSpy).to.have.been.calledOnce; - }); - }); - - it('experiment version, should display once user scroll', () => { - toggleExperiment(win, 'sticky-ad-early-load'); + it('should display once user scroll', () => { const scheduleLayoutSpy = sandbox.stub(impl, 'scheduleLayoutForAd_', () => {}); const removeOnScrollListenerSpy = @@ -151,7 +109,6 @@ describes.realWin('amp-sticky-ad 1.0 version', { impl.onScroll_(); expect(removeOnScrollListenerSpy).to.have.been.called; - toggleExperiment(win, 'sticky-ad-early-load'); // Layout on ad is called only after fixed layer is done. expect(scheduleLayoutSpy).to.not.have.been.called; expect(addToFixedLayerStub).to.have.been.calledOnce; diff --git a/tools/experiments/experiments.js b/tools/experiments/experiments.js index 221d0fd97613..234de00600e1 100644 --- a/tools/experiments/experiments.js +++ b/tools/experiments/experiments.js @@ -212,12 +212,6 @@ const EXPERIMENTS = [ id: 'jank-meter', name: 'Display jank meter', }, - { - id: 'sticky-ad-early-load', - name: 'Load sticky-ad early after user first scroll' + - 'Only apply to 1.0 version', - cleanupIssue: 'https://github.com/ampproject/amphtml/issues/7479', - }, { id: 'amp-fx-parallax', name: 'Amp extension for a parallax effect', From ab2a4f618d751ae9c6cb4a2ee4abdd12470408f3 Mon Sep 17 00:00:00 2001 From: zhouyx Date: Thu, 27 Jul 2017 14:53:57 -0700 Subject: [PATCH 2/2] fix test --- .../amp-sticky-ad/1.0/test/test-amp-sticky-ad.js | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/extensions/amp-sticky-ad/1.0/test/test-amp-sticky-ad.js b/extensions/amp-sticky-ad/1.0/test/test-amp-sticky-ad.js index 4b9825ec787c..6248626b669a 100644 --- a/extensions/amp-sticky-ad/1.0/test/test-amp-sticky-ad.js +++ b/extensions/amp-sticky-ad/1.0/test/test-amp-sticky-ad.js @@ -53,21 +53,16 @@ describes.realWin('amp-sticky-ad 1.0 version', { expect(impl.scrollUnlisten_).to.be.function; }); - it('should not build when scrollTop less than viewportHeight', () => { + it('should not build when scrollTop not greater than 1', () => { const scheduleLayoutSpy = sandbox.spy(impl, 'scheduleLayout'); const removeOnScrollListenerSpy = sandbox.spy(impl, 'removeOnScrollListener_'); const getScrollTopSpy = sandbox.spy(); - const getSizeSpy = sandbox.spy(); const getScrollHeightSpy = sandbox.spy(); impl.viewport_.getScrollTop = function() { getScrollTopSpy(); - return 20; - }; - impl.viewport_.getSize = function() { - getSizeSpy(); - return {height: 50}; + return 1; }; impl.viewport_.getScrollHeight = function() { getScrollHeightSpy(); @@ -78,7 +73,6 @@ describes.realWin('amp-sticky-ad 1.0 version', { setTimeout(resolve, 0); }).then(() => { expect(getScrollTopSpy).to.have.been.called; - expect(getSizeSpy).to.have.been.called; expect(scheduleLayoutSpy).to.not.have.been.called; expect(removeOnScrollListenerSpy).to.not.have.been.called; });