Resolve macros before encoding for analytics variables

build-system/tasks/presubmit-checks.js

@@ -1100,6 +1100,7 @@ const forbiddenTermsSrcInclusive = {
       'extensions/amp-analytics/0.1/config.js',
       'extensions/amp-analytics/0.1/cookie-writer.js',
       'extensions/amp-analytics/0.1/requests.js',
+      'extensions/amp-analytics/0.1/variables.js',
     ],
   },
   '\\.expandInputValueSync\\(': {

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

@@ -301,8 +301,8 @@ export class AmpAnalytics extends AMP.BaseElement {
         const expansionOptions = this.expansionOptions_(
           dict({}),
           trigger,
-          undefined,
-          true
+          undefined /* opt_iterations */,
+          true /* opt_noEncode */
         );
         const TAG = this.getName_();
         if (!trigger) {
@@ -361,7 +361,11 @@ export class AmpAnalytics extends AMP.BaseElement {
             } else if (trigger['selector']) {
               // Expand the selector using variable expansion.
               return this.variableService_
-                .expandTemplate(trigger['selector'], expansionOptions)
+                .expandTemplate(
+                  trigger['selector'],
+                  expansionOptions,
+                  this.element
+                )
                 .then(selector => {
                   trigger['selector'] = selector;
                   this.addTrigger_(trigger);
@@ -753,7 +757,7 @@ export class AmpAnalytics extends AMP.BaseElement {
    */
   expandTemplateWithUrlParams_(spec, expansionOptions) {
     return this.variableService_
-      .expandTemplate(spec, expansionOptions)
+      .expandTemplate(spec, expansionOptions, this.element)
       .then(key =>
         Services.urlReplacementsForDoc(this.element).expandUrlAsync(
           key,

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

@@ -208,7 +208,7 @@ export class LinkerManager {
   expandTemplateWithUrlParams_(template, expansionOptions) {
     const bindings = this.variableService_.getMacros(this.element_);
     return this.variableService_
-      .expandTemplate(template, expansionOptions)
+      .expandTemplate(template, expansionOptions, this.element_)
       .then(expanded => {
         const urlReplacements = Services.urlReplacementsForDoc(this.element_);
         return urlReplacements.expandUrlAsync(expanded, bindings);

extensions/amp-analytics/0.1/requests.js

@@ -129,7 +129,7 @@ export class RequestHandler {
     this.lastTrigger_ = trigger;
     const bindings = this.variableService_.getMacros(this.element_);
     bindings['RESOURCE_TIMING'] = getResourceTiming(
-      this.ampdoc_,
+      this.element_,
       trigger['resourceTimingSpec'],
       this.startTime_
     );
@@ -139,7 +139,10 @@ export class RequestHandler {
 
       this.baseUrlTemplatePromise_ = this.variableService_.expandTemplate(
         this.baseUrl,
-        expansionOption
+        expansionOption,
+        this.element_,
+        bindings,
+        this.whiteList_
       );
 
       this.baseUrlPromise_ = this.baseUrlTemplatePromise_.then(baseUrl => {
@@ -162,7 +165,13 @@ export class RequestHandler {
 
       this.requestOriginPromise_ = this.variableService_
         // expand variables in request origin
-        .expandTemplate(this.requestOrigin_, requestOriginExpansionOpt)
+        .expandTemplate(
+          this.requestOrigin_,
+          requestOriginExpansionOpt,
+          this.element_,
+          bindings,
+          this.whiteList_
+        )
         // substitute in URL values e.g. DOCUMENT_REFERRER -> https://example.com
         .then(expandedRequestOrigin => {
           return this.urlReplacementService_.expandUrlAsync(
@@ -182,6 +191,7 @@ export class RequestHandler {
       params,
       expansionOption,
       bindings,
+      this.element_,
       this.whiteList_
     ).then(params => {
       return dict({
@@ -410,7 +420,7 @@ export function expandPostMessage(
   expansionOption.freezeVar('extraUrlParams');
 
   const basePromise = variableService
-    .expandTemplate(msg, expansionOption)
+    .expandTemplate(msg, expansionOption, element)
     .then(base => {
       return urlReplacementService.expandStringAsync(base, bindings);
     });
@@ -427,7 +437,8 @@ export function expandPostMessage(
       urlReplacementService,
       params,
       expansionOption,
-      bindings
+      bindings,
+      element
     ).then(extraUrlParams => {
       return defaultSerializer(expandedMsg, [
         dict({'extraUrlParams': extraUrlParams}),
@@ -443,6 +454,7 @@ export function expandPostMessage(
  * @param {!Object} params
  * @param {!./variables.ExpansionOptions} expansionOption
  * @param {!Object} bindings
+ * @param {!Element} element
  * @param {!Object=} opt_whitelist
  * @return {!Promise<!Object>}
  * @private
@@ -453,6 +465,7 @@ function expandExtraUrlParams(
   params,
   expansionOption,
   bindings,
+  element,
   opt_whitelist
 ) {
   const requestPromises = [];
@@ -469,7 +482,7 @@ function expandExtraUrlParams(
 
     if (typeof value === 'string') {
       const request = variableService
-        .expandTemplate(value, option)
+        .expandTemplate(value, option, element)
         .then(value =>
           urlReplacements.expandStringAsync(value, bindings, opt_whitelist)
         )

extensions/amp-analytics/0.1/resource-timing.js

@@ -244,14 +244,14 @@ function filterEntries(entries, resourceDefs) {
  * single string.
  * @param {!Array<!PerformanceResourceTiming>} entries
  * @param {!JsonObject} resourceTimingSpec
- * @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc
+ * @param {!Element} element amp-analytics element.
  * @return {!Promise<string>}
  */
-function serialize(entries, resourceTimingSpec, ampdoc) {
+function serialize(entries, resourceTimingSpec, element) {
   const resources = resourceTimingSpec['resources'];
   const encoding = resourceTimingSpec['encoding'];
 
-  const variableService = variableServiceForDoc(ampdoc);
+  const variableService = variableServiceForDoc(element);
   const format = (val, relativeTo = 0) =>
     Math.round(val - relativeTo).toString(encoding['base'] || 10);
 
@@ -261,19 +261,19 @@ function serialize(entries, resourceTimingSpec, ampdoc) {
       return entryToExpansionOptions(entry, name, format);
     })
     .map(expansion =>
-      variableService.expandTemplate(encoding['entry'], expansion)
+      variableService.expandTemplate(encoding['entry'], expansion, element)
     );
   return Promise.all(promises).then(vars => vars.join(encoding['delim']));
 }
 
 /**
  * Serializes resource timing entries according to the resource timing spec.
- * @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc
+ * @param {!Element} element amp-analytics element.
  * @param {!JsonObject} resourceTimingSpec
  * @return {!Promise<string>}
  */
-function serializeResourceTiming(ampdoc, resourceTimingSpec) {
-  const {win} = ampdoc;
+function serializeResourceTiming(element, resourceTimingSpec) {
+  const {win} = element.getAmpDoc();
   // Check that the performance timing API exists before and that the spec is
   // valid before proceeding. If not, we simply return an empty string.
   if (
@@ -307,19 +307,19 @@ function serializeResourceTiming(ampdoc, resourceTimingSpec) {
     return Promise.resolve('');
   }
   // Yield the thread in case iterating over all resources takes a long time.
-  return yieldThread(() => serialize(entries, resourceTimingSpec, ampdoc));
+  return yieldThread(() => serialize(entries, resourceTimingSpec, element));
 }
 
 /**
- * @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc
+ * @param {!Element} element amp-analytics element.
  * @param {!JsonObject|undefined} spec resource timing spec.
  * @param {number} startTime start timestamp.
  * @return {!Promise<string>}
  */
-export function getResourceTiming(ampdoc, spec, startTime) {
+export function getResourceTiming(element, spec, startTime) {
   // Only allow collecting timing within 1s
   if (spec && Date.now() < startTime + 60 * 1000) {
-    return serializeResourceTiming(ampdoc, spec);
+    return serializeResourceTiming(element, spec);
   } else {
     return Promise.resolve('');
   }

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

@@ -56,7 +56,7 @@ describes.realWin(
     const jsonMockResponses = {
       '//invalidConfig': '{"transport": {"iframe": "fake.com"}}',
       '//config1': '{"vars": {"title": "remote"}}',
-      'https://foo/Test%20Title': '{"vars": {"title": "magic"}}',
+      'https://foo/My%20Test%20Title': '{"vars": {"title": "magic"}}',
       '//config-rv2': '{"requests": {"foo": "https://example.com/remote"}}',
       'https://rewriter.com': '{"vars": {"title": "rewritten"}}',
     };
@@ -98,7 +98,7 @@ describes.realWin(
       doc = win.document;
       ampdoc = env.ampdoc;
       configWithCredentials = false;
-      doc.title = 'Test Title';
+      doc.title = 'My Test Title';
       resetServiceForTesting(win, 'xhr');
       jsonRequestConfigs = {};
       registerServiceBuilder(win, 'xhr', function() {
@@ -545,7 +545,7 @@ describes.realWin(
         });
         return waitForSendRequest(analytics).then(() => {
           requestVerifier.verifyRequestMatch(
-            /https:\/\/example.com\/title=Test%20Title&ref=http%3A%2F%2Ffake.example%2F%3Ffoo%3Dbar/
+            /https:\/\/example.com\/title=My%20Test%20Title&ref=http%3A%2F%2Ffake.example%2F%3Ffoo%3Dbar/
           );
         });
       });
@@ -556,7 +556,9 @@ describes.realWin(
           'triggers': [{'on': 'visible', 'request': 'foo'}],
         });
         return waitForSendRequest(analytics).then(() => {
-          requestVerifier.verifyRequest('https://example.com/Test%20Title');
+          requestVerifier.verifyRequest(
+            'https://example.com/My%20Test%20Title'
+          );
         });
       });
 
@@ -623,7 +625,7 @@ describes.realWin(
         });
         return waitForSendRequest(analytics).then(() => {
           requestVerifier.verifyRequest(
-            'https://example.com/test1=Test%20Title&test2=428'
+            'https://example.com/test1=My%20Test%20Title&test2=428'
           );
         });
       });
@@ -702,7 +704,7 @@ describes.realWin(
         });
         return waitForSendRequest(analytics).then(() => {
           requestVerifier.verifyRequestMatch(
-            /https:\/\/example.com\/test1=x&test2=http%3A%2F%2Ffake.example%2F%3Ffoo%3Dbar&title=Test%20Title/
+            /https:\/\/example.com\/test1=x&test2=http%3A%2F%2Ffake.example%2F%3Ffoo%3Dbar&title=My%20Test%20Title/
           );
         });
       });
@@ -800,7 +802,7 @@ describes.realWin(
         'p:nth-child(2)',
       ].map(selectorExpansionTest);
 
-      it('does not expands selector with platform variable', () => {
+      it('expands selector with platform variable', () => {
         const tracker = ins.root_.getTracker('click', ClickEventTracker);
         const addStub = env.sandbox.stub(tracker, 'add');
         const analytics = getAnalyticsTag({
@@ -810,7 +812,7 @@ describes.realWin(
         return waitForNoSendRequest(analytics).then(() => {
           expect(addStub).to.be.calledOnce;
           const config = addStub.args[0][2];
-          expect(config['selector']).to.equal('TITLE');
+          expect(config['selector']).to.equal('My Test Title');
         });
       });
     });

extensions/amp-analytics/0.1/test/test-linker-manager.js

@@ -188,7 +188,7 @@ describes.realWin('Linker Manager', {amp: true}, env => {
     windowInterface.getUserLanguage.returns('en-US');
     env.sandbox.useFakeTimers(1533329483292);
     env.sandbox.stub(Date.prototype, 'getTimezoneOffset').returns(420);
-    doc.title = 'TEST TITLE';
+    doc.title = 'Test Title';
     const config = {
       linkers: {
         enabled: true,
@@ -217,7 +217,7 @@ describes.realWin('Linker Manager', {amp: true}, env => {
       const finalUrl =
         'https://www.source.com/dest' +
         '?a=1' +
-        '&testLinker1=1*1pgvkob*_key*VEVTVCUyMFRJVExF*gclid*MjM0' +
+        '&testLinker1=1*drctf3*_key*VGVzdCBUaXRsZQ..*gclid*MjM0' +
         '&testLinker2=1*1u4ugj3*foo*YmFy';
       expect(clickAnchor(origUrl)).to.equal(finalUrl);
       expect(navigateTo(origUrl)).to.equal(finalUrl);

extensions/amp-analytics/0.1/test/test-resource-timing.js

@@ -89,6 +89,7 @@ export function newPerformanceResourceTiming(
 describes.realWin('resourceTiming', {amp: true}, env => {
   let win;
   let ampdoc;
+  let element;
 
   /**
    * @param {!Array<!PerformanceResourceTiming} fakeEntries
@@ -102,7 +103,7 @@ describes.realWin('resourceTiming', {amp: true}, env => {
     expectedResult
   ) {
     env.sandbox.stub(win.performance, 'getEntriesByType').returns(fakeEntries);
-    return getResourceTiming(ampdoc, resourceTimingSpec, Date.now()).then(
+    return getResourceTiming(element, resourceTimingSpec, Date.now()).then(
       result => {
         expect(result).to.equal(expectedResult);
       }
@@ -112,12 +113,15 @@ describes.realWin('resourceTiming', {amp: true}, env => {
   beforeEach(() => {
     win = env.win;
     ampdoc = env.ampdoc;
+    element = document.createElement('amp-analytics');
+    element.getAmpDoc = () => ampdoc;
+    env.win.document.body.appendChild(element);
     installVariableServiceForTesting(ampdoc);
     installLinkerReaderService(win);
   });
 
   it('should return empty if the performance API is not supported', () => {
-    return getResourceTiming(ampdoc, newResourceTimingSpec(), Date.now()).then(
+    return getResourceTiming(element, newResourceTimingSpec(), Date.now()).then(
       result => {
         expect(result).to.equal('');
       }
@@ -126,7 +130,7 @@ describes.realWin('resourceTiming', {amp: true}, env => {
 
   it('should return empty when resource timing is not supported', () => {
     // Performance API (ampdoc.performance) doesn't support resource timing.
-    return getResourceTiming(ampdoc, newResourceTimingSpec(), Date.now()).then(
+    return getResourceTiming(element, newResourceTimingSpec(), Date.now()).then(
       result => {
         expect(result).to.equal('');
       }
@@ -144,9 +148,11 @@ describes.realWin('resourceTiming', {amp: true}, env => {
     );
     const spec = newResourceTimingSpec();
     env.sandbox.stub(win.performance, 'getEntriesByType').returns([entry]);
-    return getResourceTiming(win, spec, Date.now() - 60 * 1000).then(result => {
-      expect(result).to.equal('');
-    });
+    return getResourceTiming(element, spec, Date.now() - 60 * 1000).then(
+      result => {
+        expect(result).to.equal('');
+      }
+    );
   });
 
   it('should return empty if resourceTimingSpec is empty', () => {
@@ -604,13 +610,13 @@ describes.realWin('resourceTiming', {amp: true}, env => {
     const spec = newResourceTimingSpec();
     spec['encoding']['entry'] = '${initiatorType}.${startTime}.${duration}';
 
-    return getResourceTiming(ampdoc, spec, Date.now())
+    return getResourceTiming(element, spec, Date.now())
       .then(result => {
         expect(result).to.equal('link.100.400');
         expect(spec['responseAfter']).to.equal(600);
 
         // Check resource timings a second time.
-        return getResourceTiming(ampdoc, spec, Date.now());
+        return getResourceTiming(element, spec, Date.now());
       })
       .then(result => {
         expect(result).to.equal('script.200.500');