From b99c00d1a074d01763c5ea82ad9a90c0be4a8ef6 Mon Sep 17 00:00:00 2001 From: zhouyx Date: Mon, 17 Apr 2017 10:23:56 -0700 Subject: [PATCH 1/3] add createReadyReportPromise func to visibility-model --- examples/analytics.amp.html | 1 + .../amp-analytics/0.1/analytics-root.js | 13 +++- extensions/amp-analytics/0.1/events.js | 30 ++++++++ .../amp-analytics/0.1/instrumentation.js | 16 +++-- .../amp-analytics/0.1/test/test-events.js | 17 +++++ .../0.1/test/test-instrumentation.js | 14 ++++ .../0.1/test/test-visibility-manager.js | 72 ++++++++++++++----- .../0.1/test/test-visibility-model.js | 49 ++++++++++++- .../amp-analytics/0.1/visibility-manager.js | 21 ++++-- .../amp-analytics/0.1/visibility-model.js | 36 +++++++++- 10 files changed, 238 insertions(+), 31 deletions(-) diff --git a/examples/analytics.amp.html b/examples/analytics.amp.html index 2b73677d8b2f..8f30130a8945 100644 --- a/examples/analytics.amp.html +++ b/examples/analytics.amp.html @@ -88,6 +88,7 @@ "visibilitySpec": { "selector": "#anim-id", "visiblePercentageMin": 20, + "visiblePercentageMin": 1000, "continuousTimeMax": 5000 }, "vars": { diff --git a/extensions/amp-analytics/0.1/analytics-root.js b/extensions/amp-analytics/0.1/analytics-root.js index 1b26a5cf5141..4694214d621e 100644 --- a/extensions/amp-analytics/0.1/analytics-root.js +++ b/extensions/amp-analytics/0.1/analytics-root.js @@ -27,7 +27,10 @@ import {dev, user} from '../../../src/log'; import {getMode} from '../../../src/mode'; import {layoutRectLtwh} from '../../../src/layout-rect'; import {map} from '../../../src/utils/object'; -import {viewportForDoc} from '../../../src/services'; +import { + viewerForDoc, + viewportForDoc, +} from '../../../src/services'; import {whenContentIniLoad} from '../../../src/friendly-iframe-embed'; const TAG = 'amp-analytics'; @@ -88,6 +91,14 @@ export class AnalyticsRoot { */ getRoot() {} + /** + * The viewer of analytics root + * @return {!../../../src/service/viewer-impl.Viewer} + */ + getViewer() { + return viewerForDoc(this.ampdoc); + } + /** * The root element within the analytics root. * diff --git a/extensions/amp-analytics/0.1/events.js b/extensions/amp-analytics/0.1/events.js index d62ecc76c0b1..fcbc1f35c31c 100644 --- a/extensions/amp-analytics/0.1/events.js +++ b/extensions/amp-analytics/0.1/events.js @@ -377,6 +377,11 @@ export class VisibilityTracker extends EventTracker { const selector = config['selector'] || visibilitySpec['selector']; const waitForSpec = visibilitySpec['waitFor']; const visibilityManager = this.root.getVisibilityManager(); + // special polyfill for eventType: 'hidden' + let createReadyReportPromiseFunc = null; + if (eventType == 'hidden-v3') { + createReadyReportPromiseFunc = this.createReportReadyPromise_.bind(this); + } // Root selectors are delegated to analytics roots. if (!selector || selector == ':root' || selector == ':host') { @@ -385,6 +390,7 @@ export class VisibilityTracker extends EventTracker { return visibilityManager.listenRoot( visibilitySpec, this.getReadyPromise(waitForSpec, selector), + createReadyReportPromiseFunc, this.onEvent_.bind( this, eventType, listener, this.root.getRootElement())); } @@ -404,6 +410,7 @@ export class VisibilityTracker extends EventTracker { element, visibilitySpec, this.getReadyPromise(waitForSpec, selector, element), + createReadyReportPromiseFunc, this.onEvent_.bind(this, eventType, listener, element)); }); return function() { @@ -413,6 +420,29 @@ export class VisibilityTracker extends EventTracker { }; } + /** + * @return {!Promise} + * @visibleForTesting + */ + createReportReadyPromise_() { + const viewer = this.root.getViewer(); + let resolver = null; + const viewerHiddenPromise = new Promise(resolve => { + resolver = resolve; + }); + + if (!viewer.isVisible()) { + return Promise.resolve(); + } + viewer.onVisibilityChanged(() => { + if (!viewer.isVisible()) { + resolver(); + } + }); + + return viewerHiddenPromise; + } + /** * @param {string|undefined} waitForSpec * @param {string|undefined} selector diff --git a/extensions/amp-analytics/0.1/instrumentation.js b/extensions/amp-analytics/0.1/instrumentation.js index ccd9ac6d602f..72a78d39467b 100644 --- a/extensions/amp-analytics/0.1/instrumentation.js +++ b/extensions/amp-analytics/0.1/instrumentation.js @@ -104,6 +104,11 @@ const EVENT_TRACKERS = { allowedFor: ALLOWED_FOR_ALL, klass: VisibilityTracker, }, + 'hidden-v3': { + name: 'hidden-v3', + allowedFor: ALLOWED_FOR_ALL, + klass: VisibilityTracker, + }, }; /** @const {string} */ @@ -565,12 +570,15 @@ export class AnalyticsGroup { */ addTrigger(config, handler) { let eventType = dev().assertString(config['on']); + let trackerType = eventType; // TODO(dvoytenko, #8121): Cleanup visibility-v3 experiment. - if (eventType == 'visible' && this.visibilityV3_) { - eventType = 'visible-v3'; + if ((eventType == 'visible' || eventType == 'hidden') + && this.visibilityV3_) { + eventType += '-v3'; + trackerType = 'visible-v3'; } - let trackerProfile = EVENT_TRACKERS[eventType]; - if (!trackerProfile && !isEnumValue(AnalyticsEventType, eventType)) { + let trackerProfile = EVENT_TRACKERS[trackerType]; + if (!trackerProfile && !isEnumValue(AnalyticsEventType, trackerType)) { trackerProfile = EVENT_TRACKERS['custom']; } if (trackerProfile) { diff --git a/extensions/amp-analytics/0.1/test/test-events.js b/extensions/amp-analytics/0.1/test/test-events.js index 1ae6e03f5355..74e59ad860be 100644 --- a/extensions/amp-analytics/0.1/test/test-events.js +++ b/extensions/amp-analytics/0.1/test/test-events.js @@ -455,6 +455,7 @@ describes.realWin('Events', {amp: 1}, env => { .withExactArgs( matchEmptySpec, /* readyPromise */ null, + /* createReadyReportPromiseFunc */ null, saveCallback) .returns(unlisten) .once(); @@ -482,6 +483,7 @@ describes.realWin('Events', {amp: 1}, env => { .withExactArgs( matchEmptySpec, readyPromise, + null, saveCallback) .returns(unlisten) .once(); @@ -510,6 +512,7 @@ describes.realWin('Events', {amp: 1}, env => { .withExactArgs( config.visibilitySpec, readyPromise, + /* createReadyReportPromiseFunc */ null, saveCallback) .returns(unlisten) .once(); @@ -540,6 +543,7 @@ describes.realWin('Events', {amp: 1}, env => { target, config.visibilitySpec, readyPromise, + /* createReadyReportPromiseFunc */ null, saveCallback) .returns(unlisten) .once(); @@ -580,6 +584,7 @@ describes.realWin('Events', {amp: 1}, env => { target, matchEmptySpec, readyPromise, + /* createReadyReportPromiseFunc */ null, saveCallback) .returns(unlisten) .once(); @@ -678,5 +683,17 @@ describes.realWin('Events', {amp: 1}, env => { }); }); }); + + describe('should create correct readyReportPromise', () => { + it('with viewer hidden', () => { + sandbox.stub(tracker.root, 'getViewer', () => { + return { + isVisible: () => {return false;}, + }; + }); + const promise = tracker.createReportReadyPromise_(); + return promise; + }); + }); }); }); diff --git a/extensions/amp-analytics/0.1/test/test-instrumentation.js b/extensions/amp-analytics/0.1/test/test-instrumentation.js index 8ed004092230..201bbee0002e 100644 --- a/extensions/amp-analytics/0.1/test/test-instrumentation.js +++ b/extensions/amp-analytics/0.1/test/test-instrumentation.js @@ -235,6 +235,20 @@ describes.realWin('InstrumentationService', {amp: 1}, env => { analyticsElement, 'visible-v3', config, handler); }); + it('should add "visible-v3" trigger for hidden', () => { + toggleExperiment(win, 'visibility-v3', true); + group = service.createAnalyticsGroup(analyticsElement); + const config = {on: 'hidden'}; + const getTrackerSpy = sandbox.spy(root, 'getTracker'); + group.addTrigger(config, () => {}); + expect(getTrackerSpy).to.be.calledWith('visible-v3'); + const tracker = root.getTrackerOptional('visible-v3'); + const unlisten = function() {}; + const stub = sandbox.stub(tracker, 'add', () => unlisten); + group.addTrigger(config, () => {}); + expect(stub).to.be.calledWith(analyticsElement, 'hidden-v3', config); + }); + it('should use "visible-v3" for "visible" w/experiment', () => { // TODO(dvoytenko, #8121): Cleanup visibility-v3 experiment. toggleExperiment(win, 'visibility-v3', true); diff --git a/extensions/amp-analytics/0.1/test/test-visibility-manager.js b/extensions/amp-analytics/0.1/test/test-visibility-manager.js index 5331b971fe92..acedfce27da9 100644 --- a/extensions/amp-analytics/0.1/test/test-visibility-manager.js +++ b/extensions/amp-analytics/0.1/test/test-visibility-manager.js @@ -202,9 +202,10 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, env => { const modelsCalled = sandbox.spy(); const otherTarget = win.document.createElement('div'); const spec = {totalTimeMin: 10}; - root.listenRoot(spec, null, modelsCalled); - root.listenElement(otherTarget, spec, null, modelsCalled); - root.listenElement(otherTarget, {totalTimeMin: 20}, null, modelsCalled); + root.listenRoot(spec, null, null, modelsCalled); + root.listenElement(otherTarget, spec, null, null, modelsCalled); + root.listenElement( + otherTarget, {totalTimeMin: 20}, null, null, modelsCalled); expect(root.models_).to.have.length(3); root.models_.forEach(model => { model.unsubscribe(modelsDisposed); @@ -261,7 +262,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, env => { const inOb = root.getIntersectionObserver_(); const rootElement = win.document.documentElement; - root.listenElement(rootElement, {}, null, eventResolver); + root.listenElement(rootElement, {}, null, null, eventResolver); expect(root.models_).to.have.length(1); const model = root.models_[0]; expect(inOb.observeEntries_).to.have.length(1); @@ -297,7 +298,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, env => { clock.tick(1); const disposed = sandbox.spy(); const spec = {totalTimeMin: 10}; - root.listenRoot(spec, null, eventResolver); + root.listenRoot(spec, null, null, eventResolver); expect(root.models_).to.have.length(1); const model = root.models_[0]; @@ -334,7 +335,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, env => { const readyPromise = Promise.resolve().then(() => { clock.tick(21); }); - root.listenRoot(spec, readyPromise, eventResolver); + root.listenRoot(spec, readyPromise, null, eventResolver); expect(root.models_).to.have.length(1); const model = root.models_[0]; @@ -357,11 +358,33 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, env => { }); }); + it('should pass func to create readyReportPromise to model', () => { + let testPromiseResolver; + const disposed = sandbox.spy(); + const testPromise = new Promise(resolve => { + testPromiseResolver = resolve; + }); + root.listenRoot({}, null, () => { + return testPromise; + }, eventResolver); + expect(root.models_).to.have.length(1); + const model = root.models_[0]; + model.unsubscribe(disposed); + clock.tick(11); + testPromiseResolver(); + return eventPromise.then(state => { + expect(disposed).to.be.calledOnce; + expect(state.totalVisibleTime).to.equal(11); + expect(state.totalTime).to.equal(11); + expect(state.maxContinuousVisibleTime).to.equal(11); + }); + }); + it('should unlisten root', () => { clock.tick(1); const disposed = sandbox.spy(); const spec = {totalTimeMin: 10}; - const unlisten = root.listenRoot(spec, null, eventResolver); + const unlisten = root.listenRoot(spec, null, null, eventResolver); expect(root.models_).to.have.length(1); expect(Object.keys(root.trackedElements_)).to.have.length(0); @@ -379,7 +402,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, env => { const disposed = sandbox.spy(); const target = win.document.createElement('div'); const spec = {totalTimeMin: 10}; - root.listenElement(target, spec, null, eventResolver); + root.listenElement(target, spec, null, null, eventResolver); expect(root.models_).to.have.length(1); const model = root.models_[0]; @@ -461,7 +484,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, env => { // Listen to the first spec. const disposed1 = sandbox.spy(); const spec1 = {totalTimeMin: 10}; - root.listenElement(target, spec1, null, eventResolver); + root.listenElement(target, spec1, null, null, eventResolver); expect(root.models_).to.have.length(1); const model1 = root.models_[0]; expect(model1.spec_.totalTimeMin).to.equal(10); @@ -487,7 +510,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, env => { const eventPromise2 = new Promise(resolve => { eventResolver2 = resolve; }); - root.listenElement(target, spec2, null, eventResolver2); + root.listenElement(target, spec2, null, null, eventResolver2); expect(root.models_).to.have.length(2); const model2 = root.models_[1]; expect(model2.spec_.totalTimeMin).to.equal(20); @@ -535,7 +558,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, env => { sandbox.stub(resources, 'getResourceForElementOptional', () => resource); const spec = {totalTimeMin: 10}; - root.listenElement(target, spec, null, eventResolver); + root.listenElement(target, spec, null, null, eventResolver); const inOb = root.getIntersectionObserver_(); inOb.callback([{ @@ -675,11 +698,11 @@ describes.realWin('EmbedAnalyticsRoot', { it('should depend on parent for visibility', () => { const callbackSpy = sandbox.spy(); const otherTarget = win.document.createElement('div'); - root.listenRoot({}, null, callbackSpy); + root.listenRoot({}, null, null, callbackSpy); expect(root.models_).to.have.length(1); const rootModel = root.models_[0]; - root.listenElement(otherTarget, {}, null, callbackSpy); + root.listenElement(otherTarget, {}, null, null, callbackSpy); expect(root.models_).to.have.length(2); const elementModel = root.models_[1]; @@ -770,6 +793,7 @@ describes.realWin('VisibilityManager integrated', {amp: true}, env => { let eventPromise, eventResolver; let eventPromise2, eventResolver2; let readyPromise, readyResolver; + let readyReportPromise, readyReportResolver; beforeEach(() => { win = env.win; @@ -799,6 +823,9 @@ describes.realWin('VisibilityManager integrated', {amp: true}, env => { readyPromise = new Promise(resolve => { readyResolver = resolve; }); + readyReportPromise = new Promise(resolve => { + readyReportResolver = resolve; + }); eventPromise = new Promise(resolve => { eventResolver = resolve; }); @@ -868,12 +895,17 @@ describes.realWin('VisibilityManager integrated', {amp: true}, env => { viewer.setVisibilityState_(VisibilityState.VISIBLE); visibility = new VisibilityManagerForDoc(ampdoc); - visibility.listenElement(ampElement, {}, readyPromise, eventResolver); + visibility.listenElement(ampElement, {}, readyPromise, () => { + return readyReportPromise; + }, eventResolver); return Promise.resolve().then(() => { clock.tick(100); fireIntersect(25); // visible readyResolver(); + }).then(() => { + clock.tick(5); + readyReportResolver(); return eventPromise; }).then(state => { expect(state).to.contains({ @@ -884,13 +916,13 @@ describes.realWin('VisibilityManager integrated', {amp: true}, env => { elementX: 0, elementY: 75, firstSeenTime: 100, - lastSeenTime: 100, - lastVisibleTime: 100, + lastSeenTime: 105, + lastVisibleTime: 105, loadTimeVisibility: 25, maxVisiblePercentage: 25, minVisiblePercentage: 25, - totalVisibleTime: 0, - maxContinuousVisibleTime: 0, + totalVisibleTime: 5, + maxContinuousVisibleTime: 5, }); }); }); @@ -903,6 +935,7 @@ describes.realWin('VisibilityManager integrated', {amp: true}, env => { ampElement, {visiblePercentageMin: 20}, readyPromise, + null, eventResolver); // add multiple triggers on the same element @@ -910,6 +943,7 @@ describes.realWin('VisibilityManager integrated', {amp: true}, env => { ampElement, {visiblePercentageMin: 30}, readyPromise, + null, eventResolver2); // "observe" should not have been called since resource not loaded yet. @@ -976,6 +1010,7 @@ describes.realWin('VisibilityManager integrated', {amp: true}, env => { ampElement, {continuousTimeMin: 1000}, readyPromise, + null, eventResolver); const model = visibility.models_[0]; @@ -1040,6 +1075,7 @@ describes.realWin('VisibilityManager integrated', {amp: true}, env => { ampElement, {}, readyPromise, + null, eventResolver); viewer.setVisibilityState_(VisibilityState.VISIBLE); diff --git a/extensions/amp-analytics/0.1/test/test-visibility-model.js b/extensions/amp-analytics/0.1/test/test-visibility-model.js index a3b11199c8d6..f077f218bb13 100644 --- a/extensions/amp-analytics/0.1/test/test-visibility-model.js +++ b/extensions/amp-analytics/0.1/test/test-visibility-model.js @@ -263,17 +263,27 @@ describes.sandboxed('VisibilityModel', {}, () => { let vh; let updateStub; let eventSpy; + let visibilityValueForTesting = null; beforeEach(() => { vh = new VisibilityModel({ minVisiblePercentage: 25, totalTimeMin: 10, continuousTimeMin: 10, + continuousTimeMax: 1000, }, NO_CALC); - updateStub = sandbox.stub(vh, 'update'); + updateStub = sandbox.stub(vh, 'update', () => { + if (visibilityValueForTesting) { + vh.update_(visibilityValueForTesting); + } + }); eventSpy = vh.eventResolver_ = sandbox.spy(); }); + afterEach(() => { + visibilityValueForTesting = null; + }); + it('conditions not met', () => { vh.update_(0.1); expect(vh.scheduledRunId_).to.be.ok; @@ -370,6 +380,43 @@ describes.sandboxed('VisibilityModel', {}, () => { clock.tick(1000); expect(updateStub).to.not.be.called; }); + + describe('with reportReadyPromise', () => { + let reportPromise; + let promiseResolver; + beforeEach(() => { + reportPromise = new Promise(resolve => { + promiseResolver = resolve; + }); + vh.setReportReady(() => {return reportPromise;}); + }); + + it('conditions met, send when report ready', () => { + visibilityValueForTesting = 0.5; + vh.update_(1); + clock.tick(20); + vh.update_(1); + expect(eventSpy).to.not.be.called; + promiseResolver(); + return reportPromise.then(() => { + expect(eventSpy).to.be.calledOnce; + }); + }); + + it('conditions met, but no longer met when ready to report', () => { + visibilityValueForTesting = 1; + vh.update_(1); + clock.tick(20); + vh.update_(1); + eventSpy.reset(); + expect(eventSpy).to.not.be.called; + clock.tick(1001); + promiseResolver(); + return reportPromise.then(() => { + expect(eventSpy).to.not.be.called; + }); + }); + }); }); diff --git a/extensions/amp-analytics/0.1/visibility-manager.js b/extensions/amp-analytics/0.1/visibility-manager.js index e96e958856a0..f3ada8e36705 100644 --- a/extensions/amp-analytics/0.1/visibility-manager.js +++ b/extensions/amp-analytics/0.1/visibility-manager.js @@ -200,14 +200,16 @@ export class VisibilityManager { * `readyPromise` is resolved, if specified. * @param {!Object} spec * @param {?Promise} readyPromise + * @param {?function()} createReportPromiseFunc * @param {function(!Object)} callback * @return {!UnlistenDef} */ - listenRoot(spec, readyPromise, callback) { + listenRoot(spec, readyPromise, createReportPromiseFunc, callback) { const model = new VisibilityModel( spec, this.getRootVisibility.bind(this)); - return this.listen_(model, spec, readyPromise, callback); + return this.listen_( + model, spec, readyPromise, createReportPromiseFunc, callback); } /** @@ -217,26 +219,31 @@ export class VisibilityManager { * @param {!Element} element * @param {!Object} spec * @param {?Promise} readyPromise + * @param {?function()} createReportPromiseFunc * @param {function(!Object)} callback * @return {!UnlistenDef} */ - listenElement(element, spec, readyPromise, callback) { + listenElement( + element, spec, readyPromise, createReportPromiseFunc, callback) { const model = new VisibilityModel( spec, this.getElementVisibility.bind(this, element)); - return this.listen_(model, spec, readyPromise, callback, element); + return this.listen_( + model, spec, readyPromise, createReportPromiseFunc, callback, element); } /** * @param {!VisibilityModel} model * @param {!Object} spec * @param {?Promise} readyPromise + * @param {?function()} createReportPromiseFunc * @param {function(!Object)} callback * @param {!Element=} opt_element * @return {!UnlistenDef} * @private */ - listen_(model, spec, readyPromise, callback, opt_element) { + listen_(model, spec, + readyPromise, createReportPromiseFunc, callback, opt_element) { // Block visibility. if (readyPromise) { model.setReady(false); @@ -245,6 +252,10 @@ export class VisibilityManager { }); } + if (createReportPromiseFunc) { + model.setReportReady(createReportPromiseFunc); + } + // Process the event. model.onTriggerEvent(() => { const startTime = this.getStartTime(); diff --git a/extensions/amp-analytics/0.1/visibility-model.js b/extensions/amp-analytics/0.1/visibility-model.js index 522f683cffc0..a586eb2ff448 100644 --- a/extensions/amp-analytics/0.1/visibility-model.js +++ b/extensions/amp-analytics/0.1/visibility-model.js @@ -68,6 +68,12 @@ export class VisibilityModel { /** @private {boolean} */ this.ready_ = true; + /** @private {boolean} */ + this.reportReady_ = true; + + /** @private {?function()} */ + this.createReportReadyPromise_ = null; + /** @private {?number} */ this.scheduledRunId_ = null; @@ -153,6 +159,16 @@ export class VisibilityModel { this.update(); } + /** + * Sets that the model needs to wait on extra report ready promise + * after all visibility conditions have been met to call report handler + * @param {!function()} callback + */ + setReportReady(callback) { + this.reportReady_ = false; + this.createReportReadyPromise_ = callback; + } + /** * @return {number} * @private @@ -207,8 +223,24 @@ export class VisibilityModel { clearTimeout(this.scheduledRunId_); this.scheduledRunId_ = null; } - this.eventResolver_(); - this.eventResolver_ = null; + if (this.reportReady_) { + this.eventResolver_(); + this.eventResolver_ = null; + } + if (!this.reportReady_ && this.createReportReadyPromise_) { + const reportReadyPromise = this.createReportReadyPromise_(); + dev().assert(reportReadyPromise + && typeof reportReadyPromise.then == 'function', + 'reportReadyPromise is not a promise'); + this.createReportReadyPromise_ = null; + reportReadyPromise.then(() => { + this.reportReady_ = true; + // Need to update one more time in case time exceeds + // maxContinuousVisibleTime. + this.update(); + }); + // create report ready promise, to wait to call report again. + } } else if (this.matchesVisibility_ && !this.scheduledRunId_) { // There is unmet duration condition, schedule a check const timeToWait = this.computeTimeToWait_(); From e12899d0f9f52ca9fc37eafc06de88311041b4ff Mon Sep 17 00:00:00 2001 From: zhouyx Date: Tue, 18 Apr 2017 17:23:50 -0700 Subject: [PATCH 2/3] address comment --- extensions/amp-analytics/0.1/events.js | 15 +++---- .../amp-analytics/0.1/instrumentation.js | 8 ++-- .../amp-analytics/0.1/test/test-events.js | 40 ++++++++++++++++++- .../amp-analytics/0.1/visibility-manager.js | 6 +-- .../amp-analytics/0.1/visibility-model.js | 12 ++---- 5 files changed, 53 insertions(+), 28 deletions(-) diff --git a/extensions/amp-analytics/0.1/events.js b/extensions/amp-analytics/0.1/events.js index fcbc1f35c31c..3a5709f92330 100644 --- a/extensions/amp-analytics/0.1/events.js +++ b/extensions/amp-analytics/0.1/events.js @@ -426,21 +426,16 @@ export class VisibilityTracker extends EventTracker { */ createReportReadyPromise_() { const viewer = this.root.getViewer(); - let resolver = null; - const viewerHiddenPromise = new Promise(resolve => { - resolver = resolve; - }); if (!viewer.isVisible()) { return Promise.resolve(); } - viewer.onVisibilityChanged(() => { - if (!viewer.isVisible()) { - resolver(); - } - }); - return viewerHiddenPromise; + return new Promise(resolve => { + viewer.onVisibilityChanged(() => { + resolve(); + }); + }); } /** diff --git a/extensions/amp-analytics/0.1/instrumentation.js b/extensions/amp-analytics/0.1/instrumentation.js index 72a78d39467b..4a72b1391c2a 100644 --- a/extensions/amp-analytics/0.1/instrumentation.js +++ b/extensions/amp-analytics/0.1/instrumentation.js @@ -105,7 +105,7 @@ const EVENT_TRACKERS = { klass: VisibilityTracker, }, 'hidden-v3': { - name: 'hidden-v3', + name: 'visible-v3', allowedFor: ALLOWED_FOR_ALL, klass: VisibilityTracker, }, @@ -570,15 +570,13 @@ export class AnalyticsGroup { */ addTrigger(config, handler) { let eventType = dev().assertString(config['on']); - let trackerType = eventType; // TODO(dvoytenko, #8121): Cleanup visibility-v3 experiment. if ((eventType == 'visible' || eventType == 'hidden') && this.visibilityV3_) { eventType += '-v3'; - trackerType = 'visible-v3'; } - let trackerProfile = EVENT_TRACKERS[trackerType]; - if (!trackerProfile && !isEnumValue(AnalyticsEventType, trackerType)) { + let trackerProfile = EVENT_TRACKERS[eventType]; + if (!trackerProfile && !isEnumValue(AnalyticsEventType, eventType)) { trackerProfile = EVENT_TRACKERS['custom']; } if (trackerProfile) { diff --git a/extensions/amp-analytics/0.1/test/test-events.js b/extensions/amp-analytics/0.1/test/test-events.js index 74e59ad860be..53c192898855 100644 --- a/extensions/amp-analytics/0.1/test/test-events.js +++ b/extensions/amp-analytics/0.1/test/test-events.js @@ -411,6 +411,7 @@ describes.realWin('Events', {amp: 1}, env => { let eventResolver, eventPromise; let saveCallback; let matchEmptySpec; + let matchFunc; beforeEach(() => { tracker = new VisibilityTracker(root); @@ -429,6 +430,15 @@ describes.realWin('Events', {amp: 1}, env => { matchEmptySpec = sinon.match(arg => { return Object.keys(arg).length == 0; }); + matchFunc = sinon.match(arg => { + if (typeof arg == 'function') { + const promise = arg(); + if (typeof promise.then == 'function') { + return true; + } + } + return false; + }); saveCallback = sinon.match(arg => { if (typeof arg == 'function') { saveCallback.callback = arg; @@ -598,6 +608,30 @@ describes.realWin('Events', {amp: 1}, env => { }); }); + it('should pass func to get reportReady with "hidden" trigger', () => { + const config = {visibilitySpec: {selector: '.target', waitFor: 'none'}}; + visibilityManagerMock + .expects('listenElement') + .withExactArgs( + target, + config.visibilitySpec, + /* readyPromise */ null, + /* createReadyReportPromiseFunc */ matchFunc, + saveCallback) + .returns(null) + .once(); + tracker.add(analyticsElement, 'hidden-v3', config, eventResolver); + // NOTE: createReadyReportPromiseFunc is + // fully tested in test-visibility-manager + return root.ampdoc.whenReady().then(() => { + saveCallback.callback({totalVisibleTime: 10}); + return eventPromise.then(event => { + expect(event.vars.totalVisibleTime).to.equal(10); + expect(event.type).to.equal('hidden-v3'); + }); + }); + }); + describe('should wait on correct readyPromise', () => { const selector = '.target'; @@ -686,13 +720,15 @@ describes.realWin('Events', {amp: 1}, env => { describe('should create correct readyReportPromise', () => { it('with viewer hidden', () => { - sandbox.stub(tracker.root, 'getViewer', () => { + const stub = sandbox.stub(tracker.root, 'getViewer', () => { return { isVisible: () => {return false;}, }; }); const promise = tracker.createReportReadyPromise_(); - return promise; + return promise.then(() => { + expect(stub).to.be.calledOnce; + }); }); }); }); diff --git a/extensions/amp-analytics/0.1/visibility-manager.js b/extensions/amp-analytics/0.1/visibility-manager.js index f3ada8e36705..9a9ed53c21b5 100644 --- a/extensions/amp-analytics/0.1/visibility-manager.js +++ b/extensions/amp-analytics/0.1/visibility-manager.js @@ -200,7 +200,7 @@ export class VisibilityManager { * `readyPromise` is resolved, if specified. * @param {!Object} spec * @param {?Promise} readyPromise - * @param {?function()} createReportPromiseFunc + * @param {?function():!Promise} createReportPromiseFunc * @param {function(!Object)} callback * @return {!UnlistenDef} */ @@ -219,7 +219,7 @@ export class VisibilityManager { * @param {!Element} element * @param {!Object} spec * @param {?Promise} readyPromise - * @param {?function()} createReportPromiseFunc + * @param {?function():!Promise} createReportPromiseFunc * @param {function(!Object)} callback * @return {!UnlistenDef} */ @@ -236,7 +236,7 @@ export class VisibilityManager { * @param {!VisibilityModel} model * @param {!Object} spec * @param {?Promise} readyPromise - * @param {?function()} createReportPromiseFunc + * @param {?function():!Promise} createReportPromiseFunc * @param {function(!Object)} callback * @param {!Element=} opt_element * @return {!UnlistenDef} diff --git a/extensions/amp-analytics/0.1/visibility-model.js b/extensions/amp-analytics/0.1/visibility-model.js index a586eb2ff448..5f6b0f3846bd 100644 --- a/extensions/amp-analytics/0.1/visibility-model.js +++ b/extensions/amp-analytics/0.1/visibility-model.js @@ -71,7 +71,7 @@ export class VisibilityModel { /** @private {boolean} */ this.reportReady_ = true; - /** @private {?function()} */ + /** @private {?function():!Promise} */ this.createReportReadyPromise_ = null; /** @private {?number} */ @@ -162,7 +162,7 @@ export class VisibilityModel { /** * Sets that the model needs to wait on extra report ready promise * after all visibility conditions have been met to call report handler - * @param {!function()} callback + * @param {!function():!Promise} callback */ setReportReady(callback) { this.reportReady_ = false; @@ -226,12 +226,9 @@ export class VisibilityModel { if (this.reportReady_) { this.eventResolver_(); this.eventResolver_ = null; - } - if (!this.reportReady_ && this.createReportReadyPromise_) { + } else if (this.createReportReadyPromise_) { + // Report when report ready promise resolve const reportReadyPromise = this.createReportReadyPromise_(); - dev().assert(reportReadyPromise - && typeof reportReadyPromise.then == 'function', - 'reportReadyPromise is not a promise'); this.createReportReadyPromise_ = null; reportReadyPromise.then(() => { this.reportReady_ = true; @@ -239,7 +236,6 @@ export class VisibilityModel { // maxContinuousVisibleTime. this.update(); }); - // create report ready promise, to wait to call report again. } } else if (this.matchesVisibility_ && !this.scheduledRunId_) { // There is unmet duration condition, schedule a check From 33a0d3c643947b9f9a9820b23b4bc62db0de962f Mon Sep 17 00:00:00 2001 From: zhouyx Date: Wed, 19 Apr 2017 10:11:00 -0700 Subject: [PATCH 3/3] fix --- extensions/amp-analytics/0.1/events.js | 4 +++- extensions/amp-analytics/0.1/instrumentation.js | 2 +- extensions/amp-analytics/0.1/test/test-visibility-manager.js | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/extensions/amp-analytics/0.1/events.js b/extensions/amp-analytics/0.1/events.js index 3a5709f92330..bd9a7b3f1511 100644 --- a/extensions/amp-analytics/0.1/events.js +++ b/extensions/amp-analytics/0.1/events.js @@ -433,7 +433,9 @@ export class VisibilityTracker extends EventTracker { return new Promise(resolve => { viewer.onVisibilityChanged(() => { - resolve(); + if (!viewer.isVisible()) { + resolve(); + } }); }); } diff --git a/extensions/amp-analytics/0.1/instrumentation.js b/extensions/amp-analytics/0.1/instrumentation.js index 4a72b1391c2a..bd80244cf50c 100644 --- a/extensions/amp-analytics/0.1/instrumentation.js +++ b/extensions/amp-analytics/0.1/instrumentation.js @@ -105,7 +105,7 @@ const EVENT_TRACKERS = { klass: VisibilityTracker, }, 'hidden-v3': { - name: 'visible-v3', + name: 'visible-v3', // Reuse tracker with visibility allowedFor: ALLOWED_FOR_ALL, klass: VisibilityTracker, }, diff --git a/extensions/amp-analytics/0.1/test/test-visibility-manager.js b/extensions/amp-analytics/0.1/test/test-visibility-manager.js index acedfce27da9..6fb1fd454131 100644 --- a/extensions/amp-analytics/0.1/test/test-visibility-manager.js +++ b/extensions/amp-analytics/0.1/test/test-visibility-manager.js @@ -450,7 +450,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, env => { it('should protect from invalid intersection values', () => { const target = win.document.createElement('div'); - root.listenElement(target, {}, null, eventResolver); + root.listenElement(target, {}, null, null, eventResolver); expect(root.models_).to.have.length(1); const model = root.models_[0];