Backgrounded Variable added to Timer Trigger #5240

dynes · 2016-09-26T21:30:42Z

Allows timer endpoints access to backgrounded variable.

…wer-background Conflicts: extensions/amp-analytics/0.1/instrumentation.js

dynes · 2016-09-26T23:00:30Z

I have not touched amp-analytics.visibility, and I cannot get those tests to fail locally. I have ran gulp test multiple times and cannot get those tests to fail. I'm not sure what the appropriate next step is here?

jridgewell · 2016-09-27T19:16:27Z

We have some known test flake. I've restarted the travis build.

jridgewell · 2016-09-27T19:17:21Z

extensions/amp-analytics/0.1/instrumentation.js

+    const viewer = this.viewer_;
+    const intervalId = this.win_.setInterval(() => {
+      const vars = {};
+      vars['backgrounded'] = viewer.isVisible() ? '0' : '1';


This can be done inline:

const vars = { backgrounded: viewer.isVisible ? '0' : '1', };

jridgewell · 2016-09-27T19:18:08Z

extensions/amp-analytics/0.1/instrumentation.js


    if (callImmediate) {
-      listener(new AnalyticsEvent(AnalyticsEventType.TIMER));
+      const vars = {};


We could instead call the function above directly.

…wer-background

jridgewell · 2016-09-30T17:42:02Z

extensions/amp-analytics/0.1/instrumentation.js

-      timerSpec['interval'] * 1000
-    );
+    const viewer = this.viewer_;
+    const intervalId = this.win_.setInterval(() => {


Let's hoist this function instead of passing it. Then we can call it in the callImmediate conditional below.

jridgewell · 2016-09-30T17:42:37Z

@avimehta: Please confirm this is all that's needed for #4844.

…wer-background Conflicts: extensions/amp-analytics/0.1/instrumentation.js

dynes · 2016-09-30T18:18:50Z

@jridgewell Hoisted function. Let me know if that's what you were looking for.

jridgewell · 2016-09-30T18:20:00Z

extensions/amp-analytics/0.1/instrumentation.js

@@ -496,16 +496,17 @@ export class InstrumentationService {
   */
  createTimerListener_(listener, timerSpec) {
    const hasImmediate = timerSpec.hasOwnProperty('immediate');
-    const callImmediate = hasImmediate ? Boolean(timerSpec.immediate) : true;
+    const callImmediate = hasImmediate ? Boolean(timerSpec['immediate']) : true;


That was a merged in change.

jridgewell

Yup, that's it. Pending @avimehta's review.

dynes · 2016-10-04T17:50:28Z

@avimehta Any changes that you would recommend, or does this satisfy the original issue #4844 ?

avimehta · 2016-10-05T20:19:56Z

@rudygalfi This changes the meaning of backgrounded variable depending on whether the trigger is visible or timer. I think that is not desirable. wdyt?

avimehta · 2016-10-05T22:16:31Z

Talked to @rudygalfi offline. Here are a few changes we'll need to make:

Rename the variable to backgroundState.
Instead of having this variable just for timer event, let's make it a platform level variable. so you'll need to edit url-replacements-impl.js
Add documentation for this in amp-analytics documentation and variable-spec. (I am working on combining the two docs but that is not read yet so you'll have to edit both the files.)
Add tests for this in url-replacements-test.js

Finally, apologies for not responding for almost a week. I'll try to move things faster going forward.

…wer-background

…ROUNDED_STATE in amp-analytics. Added documentation for BACKGROUNDED_STATE. Added tests for BACKGROUNDED_STATE.

dynes · 2016-10-06T20:44:12Z

Looks like there is an extra build step that is not in the documentation.

gulp check-types

Will be committing changes soon.

dynes · 2016-10-06T21:12:50Z

@avimehta My changes should satisfy the concerns/requests that you brought up. Let me know if any further changes are required.

dynes · 2016-10-06T21:25:28Z

Small mistake with the naming of the variable. Changed it to the appropriate name of BACKGROUND_STATE instead of BACKGROUNDED_STATE.

avimehta · 2016-10-06T22:52:57Z

extensions/amp-analytics/0.1/instrumentation.js

-      listener.bind(null, new AnalyticsEvent(AnalyticsEventType.TIMER)),
-      timerSpec['interval'] * 1000
-    );
+    const timerEventFunction = function() {


I think these changes can be reverted. no?

Reverted these changes.

avimehta · 2016-10-06T22:54:39Z

extensions/amp-analytics/0.1/vendors.js

@@ -30,6 +30,7 @@ export const ANALYTICS_CONFIG = /** @type {!JSONType} */ ({
      'authdata': 'AUTHDATA',
      'availableScreenHeight': 'AVAILABLE_SCREEN_HEIGHT',
      'availableScreenWidth': 'AVAILABLE_SCREEN_WIDTH',
+      'backgroundedState': 'BACKGROUNDED_STATE',


sorry about this being nitpicky. Can you change it to backgroundState and BACKGROUND_STATE. The difference being that past tense variables go from 0 to 1 only once and then never change. The present tense variable gives the current state and can change value either way.

Once you make the change, you'll have to update the docs/examples.

I did in the last commit added.

ah. I was looking at wrong commit then. Let me look again.

googlebot · 2016-10-06T23:00:24Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

avimehta · 2016-10-06T23:01:40Z

Your PR is good to go. Will merge as soon as tests pass.

jridgewell · 2016-10-06T23:13:03Z

@avimehta: You'll need to tell CLA Bot that you accept the CLA. :sigh:

avimehta · 2016-10-06T23:16:08Z

@googlebot I really do accept the CLA :)

dynes · 2016-10-06T23:25:00Z

@superbaddude is the original CLA signer. Will he need to always approve the PR's, or is there a way for me to be added as a approved author?

superbaddude · 2016-10-06T23:25:18Z

@googlebot I accept the CLA.

jridgewell · 2016-10-06T23:28:25Z

@dynes an @avimehta are the only one's that need to approve. However, you need to commit with the same email address that you signed the CLA with.

dynes · 2016-10-06T23:29:22Z

@googlebot I accept the CLA.

avimehta · 2016-10-06T23:30:18Z

It's a problem with my CLA. I had email privacy feature turned on which causes this problem. Talked to @erwinmombay and He'll force merge once the tests pass.

@erwinmombay

…mp_reddit_extension * 'master' of https://github.com/ampproject/amphtml: (65 commits) Send vars from carousel to amp-analytics (ampproject#5388) Rolling back pan-x on scrollable carousel. (ampproject#5490) Add context.library.name to Segment (ampproject#5467) Rollback pan-x on carousel as it does not work on android. (ampproject#5473) cron job from @erwinmombay to update size.txt (ampproject#5468) Added example of usage of carousel arrows (ampproject#5397) ALP for A4A (ampproject#5430) Add Affiliate-B support for amp-ad (ampproject#5223) First cut of changes for New Type Inference (ampproject#5438) Lightbox 2.0: Use object-fit:cover for thumbnails Upgrade Closure Compiler to v20160911 (Sep 11, 2016) (ampproject#5457) Fake and real window fixtures for tests (ampproject#5425) Backgrounded Variable added to Timer Trigger (ampproject#5240) make `touch-action: pan-x` on `amp-carousel` (ampproject#5391) Fix PR check on new branches (ampproject#5455) Do not rely on ampExtendedElements being created (ampproject#5454) Turn type checking for 3p code back on. (ampproject#5452) Update ads/README.md with gulp lint tip (ampproject#5444) Migrate document-submit to doc scope (ampproject#5437) Link to Building an AMP Extension document. (ampproject#5451) ...

* Added backgrounded variable to timer trigger. * Inlined backgrounded field in vars object. * Hoisted timer event function. * Added BACKGROUNDED_STATE to url replacements. Added support for BACKGROUNDED_STATE in amp-analytics. Added documentation for BACKGROUNDED_STATE. Added tests for BACKGROUNDED_STATE. * Use correct set call for BACKGROUNDED_STATE call. * Changed BACKGROUNDED_STATE to BACKGROUND_STATE * Update instrumentation.js

dynes added 2 commits September 26, 2016 11:31

Added backgrounded variable to timer trigger.

019e5fc

Merge branch 'master' of ssh://github.com/ampproject/amphtml into vie…

0d50d02

…wer-background Conflicts: extensions/amp-analytics/0.1/instrumentation.js

dynes mentioned this pull request Sep 26, 2016

Intent to Implement: Allow Timer Trigger to be aware of page visibility #4844

Closed

jridgewell reviewed Sep 27, 2016

View reviewed changes

dynes added 2 commits September 27, 2016 13:49

Merge branch 'master' of ssh://github.com/ampproject/amphtml into vie…

8284e88

…wer-background

Inlined backgrounded field in vars object.

e8799de

rudygalfi assigned jridgewell and avimehta Sep 27, 2016

jridgewell requested changes Sep 30, 2016

View reviewed changes

dynes added 2 commits September 30, 2016 10:44

Merge branch 'master' of ssh://github.com/ampproject/amphtml into vie…

ac17bf0

…wer-background Conflicts: extensions/amp-analytics/0.1/instrumentation.js

Hoisted timer event function.

12c2187

jridgewell reviewed Sep 30, 2016

View reviewed changes

jridgewell approved these changes Sep 30, 2016

View reviewed changes

dynes added 3 commits October 5, 2016 15:21

Merge branch 'master' of ssh://github.com/ampproject/amphtml into vie…

329663a

…wer-background

Merge branch 'master' of ssh://github.com/ampproject/amphtml into vie…

ec024b3

…wer-background

Added BACKGROUNDED_STATE to url replacements. Added support for BACKG…

e92bba9

…ROUNDED_STATE in amp-analytics. Added documentation for BACKGROUNDED_STATE. Added tests for BACKGROUNDED_STATE.

Use correct set call for BACKGROUNDED_STATE call.

d8591f3

Changed BACKGROUNDED_STATE to BACKGROUND_STATE

3dab5b2

avimehta reviewed Oct 6, 2016

View reviewed changes

Update instrumentation.js

33c59b3

erwinmombay merged commit 784d268 into ampproject:master Oct 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backgrounded Variable added to Timer Trigger #5240

Backgrounded Variable added to Timer Trigger #5240

dynes commented Sep 26, 2016 •

edited by jridgewell

dynes commented Sep 26, 2016

jridgewell commented Sep 27, 2016

jridgewell Sep 27, 2016

jridgewell Sep 27, 2016

jridgewell Sep 30, 2016

jridgewell commented Sep 30, 2016

dynes commented Sep 30, 2016

jridgewell Sep 30, 2016

dynes Sep 30, 2016

jridgewell left a comment

dynes commented Oct 4, 2016

avimehta commented Oct 5, 2016

avimehta commented Oct 5, 2016

dynes commented Oct 6, 2016

dynes commented Oct 6, 2016

dynes commented Oct 6, 2016

avimehta Oct 6, 2016

avimehta Oct 6, 2016

avimehta Oct 6, 2016

avimehta Oct 6, 2016

dynes Oct 6, 2016

avimehta Oct 6, 2016

googlebot commented Oct 6, 2016

avimehta commented Oct 6, 2016

jridgewell commented Oct 6, 2016

avimehta commented Oct 6, 2016

dynes commented Oct 6, 2016

superbaddude commented Oct 6, 2016

jridgewell commented Oct 6, 2016

dynes commented Oct 6, 2016

avimehta commented Oct 6, 2016

Backgrounded Variable added to Timer Trigger #5240

Backgrounded Variable added to Timer Trigger #5240

Conversation

dynes commented Sep 26, 2016 • edited by jridgewell

dynes commented Sep 26, 2016

jridgewell commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell commented Sep 30, 2016

dynes commented Sep 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell left a comment

Choose a reason for hiding this comment

dynes commented Oct 4, 2016

avimehta commented Oct 5, 2016

avimehta commented Oct 5, 2016

dynes commented Oct 6, 2016

dynes commented Oct 6, 2016

dynes commented Oct 6, 2016

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

googlebot commented Oct 6, 2016

avimehta commented Oct 6, 2016

jridgewell commented Oct 6, 2016

avimehta commented Oct 6, 2016

dynes commented Oct 6, 2016

superbaddude commented Oct 6, 2016

jridgewell commented Oct 6, 2016

dynes commented Oct 6, 2016

avimehta commented Oct 6, 2016

dynes commented Sep 26, 2016 •

edited by jridgewell