Merge pull request #1146 from cramforce/cid-url

Implement URL replacement for client ids available to amp-analytics
ampproject · Dec 12, 2015 · 49de3bc · 49de3bc
2 parents d9955c5 + 118819e
commit 49de3bc
Show file tree

Hide file tree

Showing 12 changed files with 329 additions and 156 deletions.
diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js
@@ -29,6 +29,10 @@ var requiresReviewPrivacy =
     'being privacy sensitive. Please file an issue asking for permission' +
     ' to use if you have not yet done so.';
 
+var privateServiceFactory = 'This service should only be installed in ' +
+    'the whitelisted files. Other modules should use a public function ' +
+    'typically called serviceNameFor.';
+
 // Terms that must not appear in our source files.
 var forbiddenTerms = {
   'DO NOT SUBMIT': '',
@@ -44,11 +48,24 @@ var forbiddenTerms = {
       'validator/validator-in-browser.js',
     ]
   },
+  // Service factories that should only be installed once.
+  'installCidService': {
+    message: privateServiceFactory,
+    whitelist: [
+      'src/service/cid-impl.js',
+      'extensions/amp-analytics/0.1/amp-analytics.js',
+      'extensions/amp-analytics/0.1/test/test-amp-analytics.js',
+      'test/functional/test-cid.js',
+      'test/functional/test-url-replacements.js'
+    ],
+  },
+  // Privacy sensitive
   'cidFor': {
     message: requiresReviewPrivacy,
     whitelist: [
       'src/cid.js',
       'src/service/cid-impl.js',
+      'src/url-replacements.js',
       'test/functional/test-cid.js',
     ],
   },
@@ -78,13 +95,15 @@ var forbiddenTerms = {
       'src/cookies.js',
       'src/experiments.js',
       'test/functional/test-cookies.js',
+      'test/functional/test-url-replacements.js',
       'tools/experiments/experiments.js',
     ]
   },
   'eval\\(': '',
   'localStorage': {
     message: requiresReviewPrivacy,
     whitelist: [
+      'extensions/amp-analytics/0.1/test/test-amp-analytics.js',
       'test/_init_tests.js',
       'src/service/cid-impl.js',
       'test/functional/test-cid.js',

diff --git a/builtins/amp-pixel.js b/builtins/amp-pixel.js
@@ -44,16 +44,17 @@ export function installPixel(win) {
 
     /** @override */
     layoutCallback() {
-      let src = this.element.getAttribute('src');
-      src = urlReplacementsFor(this.getWin()).expand(this.assertSource(src));
-      const image = new Image();
-      image.src = src;
-      image.width = 1;
-      image.height = 1;
-      // Make it take zero space
-      this.element.style.width = 0;
-      this.element.appendChild(image);
-      return Promise.resolve();
+      const src = this.element.getAttribute('src');
+      return urlReplacementsFor(this.getWin()).expand(this.assertSource(src))
+          .then(src => {
+            const image = new Image();
+            image.src = src;
+            image.width = 1;
+            image.height = 1;
+            // Make it take zero space
+            this.element.style.width = 0;
+            this.element.appendChild(image);
+          });
     }
 
     assertSource(src) {

diff --git a/extensions/amp-analytics/0.1/amp-analytics.js b/extensions/amp-analytics/0.1/amp-analytics.js
@@ -15,6 +15,7 @@
  */
 
 import {isExperimentOn} from '../../../src/experiments';
+import {installCidService} from '../../../src/service/cid-impl';
 import {Layout} from '../../../src/layout';
 import {log} from '../../../src/log';
 import {loadPromise} from '../../../src/event-helper';
@@ -25,6 +26,9 @@ import {addListener} from './instrumentation';
 import {ANALYTICS_CONFIG} from './vendors';
 
 
+installCidService(AMP.win);
+
+
 /** @const */
 const EXPERIMENT = 'amp-analytics';
 
@@ -59,9 +63,9 @@ export class AmpAnalytics extends AMP.BaseElement {
   }
 
   /** @override */
-  buildCallback() {
+  layoutCallback() {
     if (!this.isExperimentOn_()) {
-      return;
+      return Promise.resolve();
     }
 
     /**
@@ -85,15 +89,15 @@ export class AmpAnalytics extends AMP.BaseElement {
     if (this.hasOptedOut_()) {
       // Nothing to do when the user has opted out.
       log.fine(this.getName_(), 'User has opted out. No hits will be sent.');
-      return;
+      return Promise.resolve();
     }
 
     this.generateRequests_();
 
     if (!Array.isArray(this.config_['triggers'])) {
       log.error(this.getName_(), 'No triggers were found in the config. No ' +
           'analytics data will be sent.');
-      return;
+      return Promise.resolve();
     }
 
     // Trigger callback can be synchronous. Do the registration at the end.
@@ -107,6 +111,7 @@ export class AmpAnalytics extends AMP.BaseElement {
       addListener(this.getWin(), trigger['on'],
           this.handleEvent_.bind(this, trigger));
     }
+    return Promise.resolve();
   }
 
   /**
@@ -227,12 +232,12 @@ export class AmpAnalytics extends AMP.BaseElement {
     });
 
     // For consistentcy with amp-pixel we also expand any url replacements.
-    request = urlReplacementsFor(this.getWin()).expand(request);
-
-    // TODO(btownsend, #1061): Add support for sendBeacon.
-    if (host && request) {
-      this.sendRequest_('https://' + host + request);
-    }
+    urlReplacementsFor(this.getWin()).expand(request).then(request => {
+      // TODO(btownsend, #1061): Add support for sendBeacon.
+      if (host && request) {
+        this.sendRequest_('https://' + host + request);
+      }
+    });
   }
 
   /**