Support 'hidden' trigger for visibility-v3 #8805

zhouyx · 2017-04-18T01:18:45Z

I went with the approach that pass in a function to generate reportReady promise. Will only generate the promise after conditionMet.

I didn't add another experimental tag for this. Can always add one if anyone feels it is necessary.

noted the viewer visible doesn't work for inabox case, and will not address it in this PR.

dvoytenko · 2017-04-18T22:36:37Z

extensions/amp-analytics/0.1/events.js

+   */
+  createReportReadyPromise_() {
+    const viewer = this.root.getViewer();
+    let resolver = null;


Remove let resolver from here and move the new Promise and recombine with the listener below. Ideally, the less we leak resolve/reject methods the better. E.g.

if (!isVisible()) { return Promise.resolve(); } return new Promise(resolve => { viewer.onVisibilityChanged(() => { if (!isVisible()) { resolve(); } }); });

! it looks much better!
other than using extra memory, an extra line of code. Is there anything I am missing when you say leak?

It's a scope leak. Sometimes it makes sense to scope resolve outside of the promise. That communicates that a promise can be resolved several different ways. In this case the situation is simpler - only one resolution involved and thus it's better to scope it in. It's no different then defining a let variable inside the if if it's only used in that branch.

dvoytenko · 2017-04-18T22:39:41Z

extensions/amp-analytics/0.1/instrumentation.js

    }
-    let trackerProfile = EVENT_TRACKERS[eventType];
-    if (!trackerProfile && !isEnumValue(AnalyticsEventType, eventType)) {
+    let trackerProfile = EVENT_TRACKERS[trackerType];


Let's instead have new EVENT_TRACKERS['hidden'], it should be the same as visible, specifically it should have name = 'visible-v3'. Then the root will reuse the same tracker for both visibile and hidden. But I'd like to have all trigger event types to be as separate keys of EVENT_TRACKERS. On the plus side, you won't need trackerType var.

dvoytenko · 2017-04-18T22:41:35Z

extensions/amp-analytics/0.1/test/test-events.js

+          };
+        });
+        const promise = tracker.createReportReadyPromise_();
+        return promise;


Do this:

Remember stub instance above const stub = sandbox.stub(...)

In the then of this promise, check that this stub was called.

dvoytenko · 2017-04-18T22:43:41Z

extensions/amp-analytics/0.1/test/test-events.js

@@ -580,6 +584,7 @@ describes.realWin('Events', {amp: 1}, env => {
              target,
              matchEmptySpec,
              readyPromise,
+              /* createReadyReportPromiseFunc */ null,


We also need tests to make sure we add calls with non-null createReadyReportPromiseFunc?

added a test, to check if createReadyReportPromiseFunc is a function that return a promise when eventType is hidden-v3.

dvoytenko · 2017-04-18T22:47:06Z

extensions/amp-analytics/0.1/visibility-manager.js

@@ -200,14 +200,16 @@ export class VisibilityManager {
   * `readyPromise` is resolved, if specified.
   * @param {!Object<string, *>} spec
   * @param {?Promise} readyPromise
+   * @param {?function()} createReportPromiseFunc


?function():!Promise

dvoytenko · 2017-04-18T22:47:24Z

extensions/amp-analytics/0.1/visibility-manager.js

  }

  /**
   * @param {!VisibilityModel} model
   * @param {!Object<string, *>} spec
   * @param {?Promise} readyPromise
+   * @param {?function()} createReportPromiseFunc


dvoytenko · 2017-04-18T22:47:43Z

extensions/amp-analytics/0.1/visibility-model.js

+    /** @private {boolean} */
+    this.reportReady_ = true;
+
+    /** @private {?function()} */


dvoytenko · 2017-04-18T22:48:49Z

extensions/amp-analytics/0.1/visibility-model.js

+        this.eventResolver_();
+        this.eventResolver_ = null;
+      }
+      if (!this.reportReady_ && this.createReportReadyPromise_) {


Convert this to else if (...)?

dvoytenko · 2017-04-18T22:49:41Z

extensions/amp-analytics/0.1/visibility-model.js

+        this.eventResolver_ = null;
+      }
+      if (!this.reportReady_ && this.createReportReadyPromise_) {
+        const reportReadyPromise = this.createReportReadyPromise_();


With the return type of function():!Promise we wouldn't need this assert.

This is exact the type checking I am looking for!

zhouyx · 2017-04-19T00:29:12Z

@dvoytenko comments addressed

dvoytenko · 2017-04-19T03:47:21Z

extensions/amp-analytics/0.1/events.js

+
+    return new Promise(resolve => {
+      viewer.onVisibilityChanged(() => {
+        resolve();


Let's keep the check of !viewer.isVisible() before calling resolve. It's possible that visibility changed is called many times w/o actually changing isVisible boolean.

my bad! fixed

dvoytenko · 2017-04-19T03:48:01Z

extensions/amp-analytics/0.1/instrumentation.js

@@ -104,6 +104,11 @@ const EVENT_TRACKERS = {
    allowedFor: ALLOWED_FOR_ALL,
    klass: VisibilityTracker,
  },
+  'hidden-v3': {
+    name: 'visible-v3',


Let's add a comment here, e.g. // Reuse tracker with visibility

dvoytenko · 2017-04-19T03:52:54Z

LGTM with a couple of additional requests.

zhouyx requested a review from dvoytenko April 18, 2017 01:18

dvoytenko suggested changes Apr 18, 2017

View reviewed changes

zhouyx added 2 commits April 18, 2017 18:31

add createReadyReportPromise func to visibility-model

b99c00d

address comment

e12899d

zhouyx force-pushed the visibility-hidden branch from c4842a2 to e12899d Compare April 19, 2017 01:31

dvoytenko reviewed Apr 19, 2017

View reviewed changes

dvoytenko approved these changes Apr 19, 2017

View reviewed changes

fix

33a0d3c

zhouyx merged commit 1f15f5d into ampproject:master Apr 19, 2017

zhouyx deleted the visibility-hidden branch April 19, 2017 20:58

ampprojectbot added the PR use: In Beta / Experimental label Apr 24, 2017

DarXector pushed a commit to DarXector/amphtml that referenced this pull request Apr 25, 2017

Support 'hidden' trigger for visibility-v3 (ampproject#8805)

cbee848

dvoytenko mentioned this pull request Apr 26, 2017

Analytics: implement "hidden" visibility reporting for V3 #8315

Closed

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

Support 'hidden' trigger for visibility-v3 (ampproject#8805)

f7f6ccd

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels May 1, 2017

KenneyE pushed a commit to spotxchange/amphtml that referenced this pull request May 3, 2017

Support 'hidden' trigger for visibility-v3 (ampproject#8805)

00b3dc1

caroqliu mentioned this pull request Aug 17, 2021

amp-analytics: Remove unused getViewer method #35710

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support 'hidden' trigger for visibility-v3 #8805

Support 'hidden' trigger for visibility-v3 #8805

zhouyx commented Apr 18, 2017

dvoytenko Apr 18, 2017

zhouyx Apr 19, 2017 •

edited

dvoytenko Apr 19, 2017

dvoytenko Apr 18, 2017

zhouyx Apr 19, 2017

dvoytenko Apr 18, 2017

zhouyx Apr 19, 2017

dvoytenko Apr 18, 2017

zhouyx Apr 19, 2017

dvoytenko Apr 18, 2017

dvoytenko Apr 18, 2017

dvoytenko Apr 18, 2017

dvoytenko Apr 18, 2017

dvoytenko Apr 18, 2017

zhouyx Apr 19, 2017 •

edited

zhouyx commented Apr 19, 2017

dvoytenko Apr 19, 2017

zhouyx Apr 19, 2017

dvoytenko Apr 19, 2017

zhouyx Apr 19, 2017

dvoytenko commented Apr 19, 2017

Support 'hidden' trigger for visibility-v3 #8805

Support 'hidden' trigger for visibility-v3 #8805

Conversation

zhouyx commented Apr 18, 2017

Choose a reason for hiding this comment

zhouyx Apr 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx Apr 19, 2017 • edited

Choose a reason for hiding this comment

zhouyx commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Apr 19, 2017

zhouyx Apr 19, 2017 •

edited

zhouyx Apr 19, 2017 •

edited