From f2decd8d48d121e06a14eea6780b87a025adad41 Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Sun, 19 May 2019 11:24:30 -0700
Subject: [PATCH 01/19] resolve macros in vars

---
 extensions/amp-analytics/0.1/amp-analytics.js | 12 ++-
 .../amp-analytics/0.1/linker-manager.js       |  2 +-
 extensions/amp-analytics/0.1/requests.js      | 14 ++-
 .../amp-analytics/0.1/resource-timing.js      | 18 ++--
 .../0.1/test/test-amp-analytics.js            | 14 ---
 .../0.1/test/test-linker-manager.js           |  4 +-
 .../0.1/test/test-resource-timing.js          | 53 ++++++-----
 .../amp-analytics/0.1/test/test-variables.js  | 30 ++++---
 extensions/amp-analytics/0.1/variables.js     | 89 +++++++++++++------
 9 files changed, 146 insertions(+), 90 deletions(-)

diff --git a/extensions/amp-analytics/0.1/amp-analytics.js b/extensions/amp-analytics/0.1/amp-analytics.js
index 2615928f8f1d..739bbfa37652 100644
--- a/extensions/amp-analytics/0.1/amp-analytics.js
+++ b/extensions/amp-analytics/0.1/amp-analytics.js
@@ -288,8 +288,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) {
@@ -346,7 +346,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);
@@ -733,7 +737,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,
diff --git a/extensions/amp-analytics/0.1/linker-manager.js b/extensions/amp-analytics/0.1/linker-manager.js
index 7c03673f21b4..76a8fc23d201 100644
--- a/extensions/amp-analytics/0.1/linker-manager.js
+++ b/extensions/amp-analytics/0.1/linker-manager.js
@@ -222,7 +222,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);
diff --git a/extensions/amp-analytics/0.1/requests.js b/extensions/amp-analytics/0.1/requests.js
index 8b1b0da3b9be..dc6b7d684cc6 100644
--- a/extensions/amp-analytics/0.1/requests.js
+++ b/extensions/amp-analytics/0.1/requests.js
@@ -130,6 +130,7 @@ export class RequestHandler {
     const bindings = this.variableService_.getMacros(this.element_);
     bindings['RESOURCE_TIMING'] = getResourceTiming(
       this.ampdoc_,
+      this.element_,
       trigger['resourceTimingSpec'],
       this.startTime_
     );
@@ -139,7 +140,8 @@ export class RequestHandler {
 
       this.baseUrlTemplatePromise_ = this.variableService_.expandTemplate(
         this.baseUrl,
-        expansionOption
+        expansionOption,
+        this.element_
       );
 
       this.baseUrlPromise_ = this.baseUrlTemplatePromise_.then(baseUrl => {
@@ -182,6 +184,7 @@ export class RequestHandler {
       params,
       expansionOption,
       bindings,
+      this.element_,
       this.whiteList_
     ).then(params => {
       return dict({
@@ -410,7 +413,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 +430,8 @@ export function expandPostMessage(
       urlReplacementService,
       params,
       expansionOption,
-      bindings
+      bindings,
+      element
     ).then(extraUrlParams => {
       return defaultSerializer(expandedMsg, [
         dict({'extraUrlParams': extraUrlParams}),
@@ -443,6 +447,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 +458,7 @@ function expandExtraUrlParams(
   params,
   expansionOption,
   bindings,
+  element,
   opt_whitelist
 ) {
   const requestPromises = [];
@@ -469,7 +475,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)
         )
diff --git a/extensions/amp-analytics/0.1/resource-timing.js b/extensions/amp-analytics/0.1/resource-timing.js
index 73d53ee1513b..61bc93137452 100644
--- a/extensions/amp-analytics/0.1/resource-timing.js
+++ b/extensions/amp-analytics/0.1/resource-timing.js
@@ -244,21 +244,21 @@ 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);
 
   const promises = filterEntries(entries, resources)
     .map(({entry, name}) => 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']));
 }
@@ -266,10 +266,11 @@ function serialize(entries, resourceTimingSpec, ampdoc) {
 /**
  * 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) {
+function serializeResourceTiming(ampdoc, element, resourceTimingSpec) {
   const {win} = ampdoc;
   // Check that the performance timing API exists before and that the spec is
   // valid before proceeding. If not, we simply return an empty string.
@@ -304,19 +305,20 @@ 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(ampdoc, element, spec, startTime) {
   // Only allow collecting timing within 1s
   if (spec && Date.now() < startTime + 60 * 1000) {
-    return serializeResourceTiming(ampdoc, spec);
+    return serializeResourceTiming(ampdoc, element, spec);
   } else {
     return Promise.resolve('');
   }
diff --git a/extensions/amp-analytics/0.1/test/test-amp-analytics.js b/extensions/amp-analytics/0.1/test/test-amp-analytics.js
index 23ea73b098ce..1196a3bfb137 100644
--- a/extensions/amp-analytics/0.1/test/test-amp-analytics.js
+++ b/extensions/amp-analytics/0.1/test/test-amp-analytics.js
@@ -794,20 +794,6 @@ describes.realWin(
         ':not(p)',
         'p:nth-child(2)',
       ].map(selectorExpansionTest);
-
-      it('does not expands selector with platform variable', () => {
-        const tracker = ins.root_.getTracker('click', ClickEventTracker);
-        const addStub = sandbox.stub(tracker, 'add');
-        const analytics = getAnalyticsTag({
-          requests: {foo: 'https://example.com/bar'},
-          triggers: [{on: 'click', selector: '${title}', request: 'foo'}],
-        });
-        return waitForNoSendRequest(analytics).then(() => {
-          expect(addStub).to.be.calledOnce;
-          const config = addStub.args[0][2];
-          expect(config['selector']).to.equal('TITLE');
-        });
-      });
     });
 
     describe('optout by function', () => {
diff --git a/extensions/amp-analytics/0.1/test/test-linker-manager.js b/extensions/amp-analytics/0.1/test/test-linker-manager.js
index e0e533238635..5d16b9d8dacf 100644
--- a/extensions/amp-analytics/0.1/test/test-linker-manager.js
+++ b/extensions/amp-analytics/0.1/test/test-linker-manager.js
@@ -190,7 +190,7 @@ describes.realWin('Linker Manager', {amp: true}, env => {
     windowInterface.getUserLanguage.returns('en-US');
     sandbox.useFakeTimers(1533329483292);
     sandbox.stub(Date.prototype, 'getTimezoneOffset').returns(420);
-    doc.title = 'TEST TITLE';
+    doc.title = 'TEST PAGE';
     const config = {
       linkers: {
         enabled: true,
@@ -219,7 +219,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*15chdxd*_key*VEVTVCBQQUdF*gclid*MjM0' +
         '&testLinker2=1*1u4ugj3*foo*YmFy';
       expect(clickAnchor(origUrl)).to.equal(finalUrl);
       expect(navigateTo(origUrl)).to.equal(finalUrl);
diff --git a/extensions/amp-analytics/0.1/test/test-resource-timing.js b/extensions/amp-analytics/0.1/test/test-resource-timing.js
index e8049a24fd5d..a430847824c2 100644
--- a/extensions/amp-analytics/0.1/test/test-resource-timing.js
+++ b/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,35 +103,45 @@ describes.realWin('resourceTiming', {amp: true}, env => {
     expectedResult
   ) {
     sandbox.stub(win.performance, 'getEntriesByType').returns(fakeEntries);
-    return getResourceTiming(ampdoc, resourceTimingSpec, Date.now()).then(
-      result => {
-        expect(result).to.equal(expectedResult);
-      }
-    );
+    return getResourceTiming(
+      ampdoc,
+      element,
+      resourceTimingSpec,
+      Date.now()
+    ).then(result => {
+      expect(result).to.equal(expectedResult);
+    });
   };
 
   beforeEach(() => {
     win = env.win;
     ampdoc = env.ampdoc;
+    element = env.win.document.documentElement;
     installVariableServiceForTesting(ampdoc);
     installLinkerReaderService(win);
   });
 
   it('should return empty if the performance API is not supported', () => {
-    return getResourceTiming(ampdoc, newResourceTimingSpec(), Date.now()).then(
-      result => {
-        expect(result).to.equal('');
-      }
-    );
+    return getResourceTiming(
+      ampdoc,
+      element,
+      newResourceTimingSpec(),
+      Date.now()
+    ).then(result => {
+      expect(result).to.equal('');
+    });
   });
 
   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(
-      result => {
-        expect(result).to.equal('');
-      }
-    );
+    return getResourceTiming(
+      ampdoc,
+      element,
+      newResourceTimingSpec(),
+      Date.now()
+    ).then(result => {
+      expect(result).to.equal('');
+    });
   });
 
   it('should return empty when start time has passed 1s', () => {
@@ -144,9 +155,11 @@ describes.realWin('resourceTiming', {amp: true}, env => {
     );
     const spec = newResourceTimingSpec();
     sandbox.stub(win.performance, 'getEntriesByType').returns([entry]);
-    return getResourceTiming(win, spec, Date.now() - 60 * 1000).then(result => {
-      expect(result).to.equal('');
-    });
+    return getResourceTiming(win, element, spec, Date.now() - 60 * 1000).then(
+      result => {
+        expect(result).to.equal('');
+      }
+    );
   });
 
   it('should return empty if resourceTimingSpec is empty', () => {
@@ -601,13 +614,13 @@ describes.realWin('resourceTiming', {amp: true}, env => {
     const spec = newResourceTimingSpec();
     spec['encoding']['entry'] = '${initiatorType}.${startTime}.${duration}';
 
-    return getResourceTiming(ampdoc, spec, Date.now())
+    return getResourceTiming(ampdoc, 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(ampdoc, element, spec, Date.now());
       })
       .then(result => {
         expect(result).to.equal('script.200.500');
diff --git a/extensions/amp-analytics/0.1/test/test-variables.js b/extensions/amp-analytics/0.1/test/test-variables.js
index 8773b6feb228..744e90e07930 100644
--- a/extensions/amp-analytics/0.1/test/test-variables.js
+++ b/extensions/amp-analytics/0.1/test/test-variables.js
@@ -28,6 +28,8 @@ import {
   linkerReaderServiceFor,
 } from '../linker-reader';
 
+const fakeElement = document.documentElement;
+
 describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
   let variables;
 
@@ -59,11 +61,12 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
     };
 
     function check(template, expected, vars) {
-      const actual = variables.expandTemplateSync(
+      const actual = variables.expandTemplate(
         template,
-        new ExpansionOptions(vars)
+        new ExpansionOptions(vars),
+        fakeElement
       );
-      expect(actual).to.equal(expected);
+      expect(actual).to.eventually.equal(expected);
     }
 
     it('expands nested vars (encode once)', () => {
@@ -71,11 +74,12 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
     });
 
     it('expands nested vars (no encode)', () => {
-      const actual = variables.expandTemplateSync(
+      const actual = variables.expandTemplate(
         '${a}',
-        new ExpansionOptions(vars, undefined, true)
+        new ExpansionOptions(vars, undefined, true),
+        fakeElement
       );
-      expect(actual).to.equal('https://www.google.com/a?b=1&c=2');
+      expect(actual).to.eventually.equal('https://www.google.com/a?b=1&c=2');
     });
 
     it('expands complicated string', () => {
@@ -132,11 +136,12 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
         'freeze': 'error',
       });
       vars.freezeVar('freeze');
-      const actual = variables.expandTemplateSync(
+      const actual = variables.expandTemplate(
         '${fooParam(foo,bar)}${nonfreeze}${freeze}',
-        vars
+        vars,
+        fakeElement
       );
-      expect(actual).to.equal('QUERY_PARAM(foo,bar)${freeze}');
+      expect(actual).to.eventually.equal('QUERY_PARAM(foo,bar)${freeze}');
     });
 
     it('expands array vars', () => {
@@ -178,11 +183,12 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
         expectAsyncConsoleError(
           /Maximum depth reached while expanding variables/
         );
-        const actual = variables.expandTemplateSync(
+        const actual = variables.expandTemplate(
           '${1}',
-          new ExpansionOptions(recursiveVars, 5)
+          new ExpansionOptions(recursiveVars, 5),
+          fakeElement
         );
-        expect(actual).to.equal('123412%24%7B3%7D');
+        expect(actual).to.eventually.equal('123412%24%7B3%7D');
       });
     });
   });
diff --git a/extensions/amp-analytics/0.1/variables.js b/extensions/amp-analytics/0.1/variables.js
index 89e1d150d182..cef05ae6ef02 100644
--- a/extensions/amp-analytics/0.1/variables.js
+++ b/extensions/amp-analytics/0.1/variables.js
@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+import {SANDBOX_AVAILABLE_VARS} from './sandbox-vars-whitelist';
 import {Services} from '../../../src/services';
 import {base64UrlEncodeFromString} from '../../../src/utils/base64';
 import {cookieReader} from './cookie-reader';
@@ -27,7 +28,6 @@ import {
 } from '../../../src/service';
 import {isArray, isFiniteNumber} from '../../../src/types';
 import {linkerReaderServiceFor} from './linker-reader';
-import {tryResolve} from '../../../src/utils/promise';
 
 /** @const {string} */
 const TAG = 'amp-analytics/variables';
@@ -189,6 +189,11 @@ export class VariableService {
     /** @const @private {!./linker-reader.LinkerReader} */
     this.linkerReader_ = linkerReaderServiceFor(this.ampdoc_.win);
 
+    /** @const @private {!../../../src/service/url-replacements-impl.UrlReplacements} */
+    this.urlReplacementService_ = Services.urlReplacementsForDoc(
+      this.ampdoc_.getHeadNode()
+    );
+
     this.register_('$DEFAULT', defaultMacro);
     this.register_('$SUBSTR', substrMacro);
     this.register_('$TRIM', value => value.trim());
@@ -237,22 +242,13 @@ export class VariableService {
   }
 
   /**
-   * @param {string} template The template to expand
-   * @param {!ExpansionOptions} options configuration to use for expansion
-   * @return {!Promise<string>} The expanded string
-   */
-  expandTemplate(template, options) {
-    return tryResolve(this.expandTemplateSync.bind(this, template, options));
-  }
-
-  /**
-   * @param {string} template The template to expand
-   * @param {!ExpansionOptions} options configuration to use for expansion
-   * @return {string} The expanded string
-   * @visibleForTesting
+   * @param {string} template The template to expand.
+   * @param {!ExpansionOptions} options configuration to use for expansion.
+   * @param {!Element} element amp-analytics element.
+   * @return {!Promise<string>} The expanded string.
    */
-  expandTemplateSync(template, options) {
-    return template.replace(/\${([^}]*)}/g, (match, key) => {
+  expandTemplate(template, options, element) {
+    return this.asyncStringReplace_(template, /\${([^}]*)}/g, (match, key) => {
       if (options.iterations < 0) {
         user().error(
           TAG,
@@ -277,26 +273,69 @@ export class VariableService {
       let value = options.getVar(name);
 
       if (typeof value == 'string') {
-        value = this.expandTemplateSync(
+        value = this.expandTemplate(
           value,
           new ExpansionOptions(
             options.vars,
             options.iterations - 1,
             true /* noEncode */
-          )
+          ),
+          element
         );
       }
 
-      if (!options.noEncode) {
-        value = encodeVars(/** @type {string|?Array<string>} */ (value));
-      }
-      if (value) {
-        value += argList;
-      }
-      return value;
+      const bindings = this.getMacros();
+      const urlReplacements = this.urlReplacementService_;
+      const whitelist = element.hasAttribute('sandbox')
+        ? SANDBOX_AVAILABLE_VARS
+        : undefined;
+
+      return Promise.resolve(value)
+        .then(value => {
+          if (isArray(value)) {
+            return Promise.all(
+              value.map(item =>
+                urlReplacements.expandStringAsync(item, bindings, whitelist)
+              )
+            );
+          }
+          return urlReplacements.expandStringAsync(
+            value + argList,
+            bindings,
+            whitelist
+          );
+        })
+        .then(value => {
+          if (!options.noEncode) {
+            value = encodeVars(/** @type {string|?Array<string>} */ (value));
+          }
+          return value;
+        });
     });
   }
 
+  /**
+   * Wrapper around String.replace that handles asynchronous resolution.
+   * @param {string} str
+   * @param {RegExp} regex
+   * @param {Function} replacer
+   */
+  asyncStringReplace_(str, regex, replacer) {
+    const stringBuilder = [];
+    let lastIndex = 0;
+
+    str.replace(regex, (match, key, matchIndex) => {
+      stringBuilder.push(str.slice(lastIndex, matchIndex));
+      // Store the promise in it's eventual string position.
+      const replacementPromise = replacer(match, key);
+      stringBuilder.push(replacementPromise);
+      lastIndex = matchIndex + match.length;
+    });
+
+    stringBuilder.push(str.slice(lastIndex));
+    return Promise.all(stringBuilder).then(resolved => resolved.join(''));
+  }
+
   /**
    * @param {string} value
    * @return {!Promise<string>}

From 07b0564e98cf930d1396c2a082b0f02204b2f561 Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Sun, 19 May 2019 12:15:30 -0700
Subject: [PATCH 02/19] add test

---
 .../amp-analytics/0.1/test/test-variables.js  | 78 +++++++++++--------
 1 file changed, 45 insertions(+), 33 deletions(-)

diff --git a/extensions/amp-analytics/0.1/test/test-variables.js b/extensions/amp-analytics/0.1/test/test-variables.js
index 744e90e07930..160f09f34238 100644
--- a/extensions/amp-analytics/0.1/test/test-variables.js
+++ b/extensions/amp-analytics/0.1/test/test-variables.js
@@ -66,11 +66,15 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
         new ExpansionOptions(vars),
         fakeElement
       );
-      expect(actual).to.eventually.equal(expected);
+      return expect(actual).to.eventually.equal(expected);
     }
 
     it('expands nested vars (encode once)', () => {
-      check('${a}', 'https%3A%2F%2Fwww.google.com%2Fa%3Fb%3D1%26c%3D2', vars);
+      return check(
+        '${a}',
+        'https%3A%2F%2Fwww.google.com%2Fa%3Fb%3D1%26c%3D2',
+        vars
+      );
     });
 
     it('expands nested vars (no encode)', () => {
@@ -83,7 +87,7 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
     });
 
     it('expands complicated string', () => {
-      check('${foo}', 'HELLO%2FWORLD%2BWORLD%2BHELLO%2BHELLO', {
+      return check('${foo}', 'HELLO%2FWORLD%2BWORLD%2BHELLO%2BHELLO', {
         'foo': '${a}+${b}+${c}+${hello}',
         'a': '${hello}/${world}',
         'b': '${world}',
@@ -94,41 +98,49 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
     });
 
     it('expands zeros', () => {
-      check('${zero}', '0', {'zero': 0});
+      return check('${zero}', '0', {'zero': 0});
     });
 
     it('drops unknown vars', () => {
-      check('a=${known}&b=${unknown}', 'a=KNOWN&b=', {'known': 'KNOWN'});
+      return check('a=${known}&b=${unknown}', 'a=KNOWN&b=', {'known': 'KNOWN'});
     });
 
     it('does not expand macros', () => {
-      check('MACRO(a,b)', 'MACRO(a,b)', {});
+      return check('MACRO(a,b)', 'MACRO(a,b)', {});
     });
 
-    it('supports macro args', () => {
+    it('supports macro args', () =>
       check('${foo}', 'AAA(BBB(1))', {
         'foo': 'AAA(BBB(1))',
-      });
-
-      // TODO: fix this, should be 'AAA(BBB(1,2))'
-      check('${foo}', 'AAA(BBB(1%2C2))', {
-        'foo': 'AAA(BBB(1,2))',
-      });
-
-      check('${foo}&${bar(3,4)}', 'FOO(1,2)&BAR(3,4)', {
-        'foo': 'FOO(1,2)',
-        'bar': 'BAR',
-      });
-
-      // TODO: fix this, should be 'AAA(1,2)%26BBB(3,4)%26CCC(5,6)%26DDD(7,8)'
-      check('${all}', 'AAA(1%2C2)%26BBB(3%2C4)%26CCC(5%2C6)%26DDD(7,8)', {
-        'a': 'AAA',
-        'b': 'BBB',
-        'c': 'CCC(5,6)',
-        'd': 'DDD(7,8)',
-        'all': '${a(1,2)}&${b(3,4)}&${c}&${d}',
-      });
-    });
+      })
+        .then(() =>
+          // TODO: fix this, should be 'AAA(BBB(1,2))'
+          check('${foo}', 'AAA(BBB(1%2C2))', {
+            'foo': 'AAA(BBB(1,2))',
+          })
+        )
+        .then(() =>
+          check('${foo}&${bar(3,4)}', 'FOO(1,2)&BAR(3,4)', {
+            'foo': 'FOO(1,2)',
+            'bar': 'BAR',
+          })
+        )
+        .then(() =>
+          // TODO: fix this, should be 'AAA(1,2)%26BBB(3,4)%26CCC(5,6)%26DDD(7,8)'
+          check('${all}', 'AAA(1%2C2)%26BBB(3%2C4)%26CCC(5%2C6)%26DDD(7,8)', {
+            'a': 'AAA',
+            'b': 'BBB',
+            'c': 'CCC(5,6)',
+            'd': 'DDD(7,8)',
+            'all': '${a(1,2)}&${b(3,4)}&${c}&${d}',
+          })
+        )
+        .then(() =>
+          check('${nested}', 'default', {
+            'nested': '${deeper}',
+            'deeper': '$IF(true, QUERY_PARAM(foo, default), never)',
+          })
+        ));
 
     it('respect freeze variables', () => {
       const vars = new ExpansionOptions({
@@ -141,11 +153,11 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
         vars,
         fakeElement
       );
-      expect(actual).to.eventually.equal('QUERY_PARAM(foo,bar)${freeze}');
+      return expect(actual).to.eventually.equal('bar${freeze}');
     });
 
     it('expands array vars', () => {
-      check(
+      return check(
         '${array}',
         'xy%26x,MACRO(abc,def),MACRO(abc%2Cdef)%26123,%24%7Bfoo%7D',
         {
@@ -161,7 +173,7 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
     });
 
     it('handles empty var name', () => {
-      check('${}', '', {});
+      return check('${}', '', {});
     });
 
     describe('should handle recursive vars', () => {
@@ -176,7 +188,7 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
         expectAsyncConsoleError(
           /Maximum depth reached while expanding variables/
         );
-        check('${1}', '123%24%7B4%7D', recursiveVars);
+        return check('${1}', '123%24%7B4%7D', recursiveVars);
       });
 
       it('customize recursions to 5', () => {
@@ -188,7 +200,7 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
           new ExpansionOptions(recursiveVars, 5),
           fakeElement
         );
-        expect(actual).to.eventually.equal('123412%24%7B3%7D');
+        return expect(actual).to.eventually.equal('123412%24%7B3%7D');
       });
     });
   });

From c9a829985bec69dd787a365d9a58d9b12baa3522 Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Tue, 9 Jul 2019 14:16:40 -0700
Subject: [PATCH 03/19] update for request origin

---
 extensions/amp-analytics/0.1/requests.js  | 6 +++++-
 extensions/amp-analytics/0.1/variables.js | 9 +--------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/extensions/amp-analytics/0.1/requests.js b/extensions/amp-analytics/0.1/requests.js
index dc6b7d684cc6..b5e07f2821d9 100644
--- a/extensions/amp-analytics/0.1/requests.js
+++ b/extensions/amp-analytics/0.1/requests.js
@@ -164,7 +164,11 @@ export class RequestHandler {
 
       this.requestOriginPromise_ = this.variableService_
         // expand variables in request origin
-        .expandTemplate(this.requestOrigin_, requestOriginExpansionOpt)
+        .expandTemplate(
+          this.requestOrigin_,
+          requestOriginExpansionOpt,
+          this.element_
+        )
         // substitute in URL values e.g. DOCUMENT_REFERRER -> https://example.com
         .then(expandedRequestOrigin => {
           return this.urlReplacementService_.expandUrlAsync(
diff --git a/extensions/amp-analytics/0.1/variables.js b/extensions/amp-analytics/0.1/variables.js
index cef05ae6ef02..31d87635f8b3 100644
--- a/extensions/amp-analytics/0.1/variables.js
+++ b/extensions/amp-analytics/0.1/variables.js
@@ -189,11 +189,6 @@ export class VariableService {
     /** @const @private {!./linker-reader.LinkerReader} */
     this.linkerReader_ = linkerReaderServiceFor(this.ampdoc_.win);
 
-    /** @const @private {!../../../src/service/url-replacements-impl.UrlReplacements} */
-    this.urlReplacementService_ = Services.urlReplacementsForDoc(
-      this.ampdoc_.getHeadNode()
-    );
-
     this.register_('$DEFAULT', defaultMacro);
     this.register_('$SUBSTR', substrMacro);
     this.register_('$TRIM', value => value.trim());
@@ -211,8 +206,6 @@ export class VariableService {
       '$EQUALS',
       (firstValue, secValue) => firstValue === secValue
     );
-    // TODO(ccordry): Make sure this stays a window level service when this
-    // VariableService is migrated to document level.
     this.register_('LINKER_PARAM', (name, id) =>
       this.linkerReader_.get(name, id)
     );
@@ -285,7 +278,7 @@ export class VariableService {
       }
 
       const bindings = this.getMacros();
-      const urlReplacements = this.urlReplacementService_;
+      const urlReplacements = Services.urlReplacementsForDoc(element);
       const whitelist = element.hasAttribute('sandbox')
         ? SANDBOX_AVAILABLE_VARS
         : undefined;

From 6464458f7015f95a03945386aa354238c096b66a Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Tue, 9 Jul 2019 14:41:00 -0700
Subject: [PATCH 04/19] whitelist expandStringAsync

---
 build-system/tasks/presubmit-checks.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js
index 4adc5048f04b..3c64627b7515 100644
--- a/build-system/tasks/presubmit-checks.js
+++ b/build-system/tasks/presubmit-checks.js
@@ -922,6 +922,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/varibles.js',
     ],
   },
   '\\.expandInputValueSync\\(': {

From 7c78bbc2b4e13c407d16008140f2ba36cfa10675 Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Thu, 8 Aug 2019 13:04:52 -0700
Subject: [PATCH 05/19] pass element to getMacros

---
 .../amp-analytics/0.1/test/test-variables.js  |  6 ++++-
 extensions/amp-analytics/0.1/variables.js     | 23 ++++++++++++++++++-
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/extensions/amp-analytics/0.1/test/test-variables.js b/extensions/amp-analytics/0.1/test/test-variables.js
index 160f09f34238..4c7ef34ace1f 100644
--- a/extensions/amp-analytics/0.1/test/test-variables.js
+++ b/extensions/amp-analytics/0.1/test/test-variables.js
@@ -115,6 +115,9 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
       })
         .then(() =>
           // TODO: fix this, should be 'AAA(BBB(1,2))'
+          // This is a resutl of `getNameArgs` strange behavior. Leaving here as
+          // pseudo documentation. We mostly avoid this problem, as now we expand any
+          // valid macros when they are seen in `Variables#expandTemplate`.
           check('${foo}', 'AAA(BBB(1%2C2))', {
             'foo': 'AAA(BBB(1,2))',
           })
@@ -126,7 +129,8 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
           })
         )
         .then(() =>
-          // TODO: fix this, should be 'AAA(1,2)%26BBB(3,4)%26CCC(5,6)%26DDD(7,8)'
+          // TODO: fix this, should be 'AAA(1,2)%26BBB(3,4)%26CCC(5,6)%26DDD(7,8)
+          // See comment about getNameArgs above.
           check('${all}', 'AAA(1%2C2)%26BBB(3%2C4)%26CCC(5%2C6)%26DDD(7,8)', {
             'a': 'AAA',
             'b': 'BBB',
diff --git a/extensions/amp-analytics/0.1/variables.js b/extensions/amp-analytics/0.1/variables.js
index 31d87635f8b3..80318f043ebc 100644
--- a/extensions/amp-analytics/0.1/variables.js
+++ b/extensions/amp-analytics/0.1/variables.js
@@ -277,7 +277,7 @@ export class VariableService {
         );
       }
 
-      const bindings = this.getMacros();
+      const bindings = this.getMacros(element);
       const urlReplacements = Services.urlReplacementsForDoc(element);
       const whitelist = element.hasAttribute('sandbox')
         ? SANDBOX_AVAILABLE_VARS
@@ -312,6 +312,7 @@ export class VariableService {
    * @param {string} str
    * @param {RegExp} regex
    * @param {Function} replacer
+   * @return {!Promise<string>}
    */
   asyncStringReplace_(str, regex, replacer) {
     const stringBuilder = [];
@@ -441,3 +442,23 @@ export function stringToBool(str) {
     str !== 'undefined'
   );
 }
+
+// ${some_macro(${a}, ${another_macro(${b}))}
+// // config
+// {
+//   final: ${some_macro(${a}, ${another_macro(${b}))},
+//   a: 'foo',
+//   b: [1, 2, 3],
+// }
+
+// 1st match in asyncStringReplace_ => 'some_macro(${a}, ${another_macro(${b}))}';
+// 2nd match => 'a'
+// 3rd match => 'another_macro(${b})'
+// 4th match => 'b'
+
+// // This should work
+// {
+//   top: ${some_macro(${a}, ${b})},
+//   b: ${another_macro(${c})},
+//   c: '123',
+// }

From 8c8baf8098d07e85b201415cd5e30e6be5b7e8f7 Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Thu, 8 Aug 2019 13:20:16 -0700
Subject: [PATCH 06/19] remove commented out

---
 extensions/amp-analytics/0.1/variables.js | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/extensions/amp-analytics/0.1/variables.js b/extensions/amp-analytics/0.1/variables.js
index 80318f043ebc..1ebac629b866 100644
--- a/extensions/amp-analytics/0.1/variables.js
+++ b/extensions/amp-analytics/0.1/variables.js
@@ -442,23 +442,3 @@ export function stringToBool(str) {
     str !== 'undefined'
   );
 }
-
-// ${some_macro(${a}, ${another_macro(${b}))}
-// // config
-// {
-//   final: ${some_macro(${a}, ${another_macro(${b}))},
-//   a: 'foo',
-//   b: [1, 2, 3],
-// }
-
-// 1st match in asyncStringReplace_ => 'some_macro(${a}, ${another_macro(${b}))}';
-// 2nd match => 'a'
-// 3rd match => 'another_macro(${b})'
-// 4th match => 'b'
-
-// // This should work
-// {
-//   top: ${some_macro(${a}, ${b})},
-//   b: ${another_macro(${c})},
-//   c: '123',
-// }

From 4c5fe97a824a84fac1a3e13e7defa99c0da4e32e Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Thu, 8 Aug 2019 17:01:22 -0700
Subject: [PATCH 07/19] easy comments

---
 extensions/amp-analytics/0.1/test/test-variables.js | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/extensions/amp-analytics/0.1/test/test-variables.js b/extensions/amp-analytics/0.1/test/test-variables.js
index 4c7ef34ace1f..7380c050c452 100644
--- a/extensions/amp-analytics/0.1/test/test-variables.js
+++ b/extensions/amp-analytics/0.1/test/test-variables.js
@@ -114,8 +114,7 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
         'foo': 'AAA(BBB(1))',
       })
         .then(() =>
-          // TODO: fix this, should be 'AAA(BBB(1,2))'
-          // This is a resutl of `getNameArgs` strange behavior. Leaving here as
+          // This is a result of `getNameArgs` strange behavior. Leaving here as
           // pseudo documentation. We mostly avoid this problem, as now we expand any
           // valid macros when they are seen in `Variables#expandTemplate`.
           check('${foo}', 'AAA(BBB(1%2C2))', {
@@ -129,7 +128,6 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
           })
         )
         .then(() =>
-          // TODO: fix this, should be 'AAA(1,2)%26BBB(3,4)%26CCC(5,6)%26DDD(7,8)
           // See comment about getNameArgs above.
           check('${all}', 'AAA(1%2C2)%26BBB(3%2C4)%26CCC(5%2C6)%26DDD(7,8)', {
             'a': 'AAA',

From 9916eb08584a750dbca0935a5c1432add69b1b7d Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Thu, 8 Aug 2019 17:02:30 -0700
Subject: [PATCH 08/19] move asyncReplace

---
 extensions/amp-analytics/0.1/variables.js | 26 ++---------------------
 src/string.js                             | 23 ++++++++++++++++++++
 2 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/extensions/amp-analytics/0.1/variables.js b/extensions/amp-analytics/0.1/variables.js
index 1ebac629b866..bbde464db274 100644
--- a/extensions/amp-analytics/0.1/variables.js
+++ b/extensions/amp-analytics/0.1/variables.js
@@ -16,6 +16,7 @@
 
 import {SANDBOX_AVAILABLE_VARS} from './sandbox-vars-whitelist';
 import {Services} from '../../../src/services';
+import {asyncStringReplace} from '../../../src/string';
 import {base64UrlEncodeFromString} from '../../../src/utils/base64';
 import {cookieReader} from './cookie-reader';
 import {dev, devAssert, user, userAssert} from '../../../src/log';
@@ -241,7 +242,7 @@ export class VariableService {
    * @return {!Promise<string>} The expanded string.
    */
   expandTemplate(template, options, element) {
-    return this.asyncStringReplace_(template, /\${([^}]*)}/g, (match, key) => {
+    return asyncStringReplace(template, /\${([^}]*)}/g, (match, key) => {
       if (options.iterations < 0) {
         user().error(
           TAG,
@@ -307,29 +308,6 @@ export class VariableService {
     });
   }
 
-  /**
-   * Wrapper around String.replace that handles asynchronous resolution.
-   * @param {string} str
-   * @param {RegExp} regex
-   * @param {Function} replacer
-   * @return {!Promise<string>}
-   */
-  asyncStringReplace_(str, regex, replacer) {
-    const stringBuilder = [];
-    let lastIndex = 0;
-
-    str.replace(regex, (match, key, matchIndex) => {
-      stringBuilder.push(str.slice(lastIndex, matchIndex));
-      // Store the promise in it's eventual string position.
-      const replacementPromise = replacer(match, key);
-      stringBuilder.push(replacementPromise);
-      lastIndex = matchIndex + match.length;
-    });
-
-    stringBuilder.push(str.slice(lastIndex));
-    return Promise.all(stringBuilder).then(resolved => resolved.join(''));
-  }
-
   /**
    * @param {string} value
    * @return {!Promise<string>}
diff --git a/src/string.js b/src/string.js
index 1f436ae993ee..e100d6876a8b 100644
--- a/src/string.js
+++ b/src/string.js
@@ -172,3 +172,26 @@ export function trimStart(str) {
 
   return (str + '_').trim().slice(0, -1);
 }
+
+/**
+ * Wrapper around String.replace that handles asynchronous resolution.
+ * @param {string} str
+ * @param {RegExp} regex
+ * @param {Function} replacer
+ * @return {!Promise<string>}
+ */
+export function asyncStringReplace(str, regex, replacer) {
+  const stringBuilder = [];
+  let lastIndex = 0;
+
+  str.replace(regex, (match, key, matchIndex) => {
+    stringBuilder.push(str.slice(lastIndex, matchIndex));
+    // Store the promise in it's eventual string position.
+    const replacementPromise = replacer(match, key);
+    stringBuilder.push(replacementPromise);
+    lastIndex = matchIndex + match.length;
+  });
+
+  stringBuilder.push(str.slice(lastIndex));
+  return Promise.all(stringBuilder).then(resolved => resolved.join(''));
+}

From 8e92011c956c703a1ca1c93a83076a8ca186f46f Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Fri, 9 Aug 2019 09:44:53 -0700
Subject: [PATCH 09/19] remove ampdoc from serializeResourceTiming

---
 extensions/amp-analytics/0.1/requests.js        |  1 -
 extensions/amp-analytics/0.1/resource-timing.js | 10 ++++------
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/extensions/amp-analytics/0.1/requests.js b/extensions/amp-analytics/0.1/requests.js
index b5e07f2821d9..5732a721a2d0 100644
--- a/extensions/amp-analytics/0.1/requests.js
+++ b/extensions/amp-analytics/0.1/requests.js
@@ -129,7 +129,6 @@ 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_
diff --git a/extensions/amp-analytics/0.1/resource-timing.js b/extensions/amp-analytics/0.1/resource-timing.js
index 61bc93137452..1a549fd56501 100644
--- a/extensions/amp-analytics/0.1/resource-timing.js
+++ b/extensions/amp-analytics/0.1/resource-timing.js
@@ -265,13 +265,12 @@ function serialize(entries, resourceTimingSpec, element) {
 
 /**
  * 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, element, 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 (
@@ -309,16 +308,15 @@ function serializeResourceTiming(ampdoc, element, resourceTimingSpec) {
 }
 
 /**
- * @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, element, spec, startTime) {
+export function getResourceTiming(element, spec, startTime) {
   // Only allow collecting timing within 1s
   if (spec && Date.now() < startTime + 60 * 1000) {
-    return serializeResourceTiming(ampdoc, element, spec);
+    return serializeResourceTiming(element, spec);
   } else {
     return Promise.resolve('');
   }

From 40bd5b6ae98630765b57ed39e01936e39b55ab62 Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Fri, 9 Aug 2019 10:24:14 -0700
Subject: [PATCH 10/19] more comments

---
 extensions/amp-analytics/0.1/test/test-variables.js | 8 ++++++++
 extensions/amp-analytics/0.1/variables.js           | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/extensions/amp-analytics/0.1/test/test-variables.js b/extensions/amp-analytics/0.1/test/test-variables.js
index 7380c050c452..ce683005f231 100644
--- a/extensions/amp-analytics/0.1/test/test-variables.js
+++ b/extensions/amp-analytics/0.1/test/test-variables.js
@@ -109,6 +109,14 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
       return check('MACRO(a,b)', 'MACRO(a,b)', {});
     });
 
+    it('does not handle nested macros using ${} syntax', () => {
+      // VariableService.expandTemplate's regex cannot parse nested ${}.
+      return check('${a${b}}', '}', {
+        'a': 'TITLE',
+        'b': 'TITLE',
+      });
+    });
+
     it('supports macro args', () =>
       check('${foo}', 'AAA(BBB(1))', {
         'foo': 'AAA(BBB(1))',
diff --git a/extensions/amp-analytics/0.1/variables.js b/extensions/amp-analytics/0.1/variables.js
index bbde464db274..e5c3dcb56bbc 100644
--- a/extensions/amp-analytics/0.1/variables.js
+++ b/extensions/amp-analytics/0.1/variables.js
@@ -236,6 +236,8 @@ export class VariableService {
   }
 
   /**
+   * Converts templates from ${} format to MACRO() and resolves any platform
+   * level macros when encountered.
    * @param {string} template The template to expand.
    * @param {!ExpansionOptions} options configuration to use for expansion.
    * @param {!Element} element amp-analytics element.

From 13ab0c94ed5f500a553849ea0132438eda8c0929 Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Fri, 9 Aug 2019 15:44:51 -0700
Subject: [PATCH 11/19] pass in whitelist and bindings

---
 extensions/amp-analytics/0.1/requests.js  |  4 +++-
 extensions/amp-analytics/0.1/variables.js | 18 +++++++++---------
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/extensions/amp-analytics/0.1/requests.js b/extensions/amp-analytics/0.1/requests.js
index 5732a721a2d0..68fdf5c41237 100644
--- a/extensions/amp-analytics/0.1/requests.js
+++ b/extensions/amp-analytics/0.1/requests.js
@@ -140,7 +140,9 @@ export class RequestHandler {
       this.baseUrlTemplatePromise_ = this.variableService_.expandTemplate(
         this.baseUrl,
         expansionOption,
-        this.element_
+        this.element_,
+        bindings,
+        this.whiteList_
       );
 
       this.baseUrlPromise_ = this.baseUrlTemplatePromise_.then(baseUrl => {
diff --git a/extensions/amp-analytics/0.1/variables.js b/extensions/amp-analytics/0.1/variables.js
index e5c3dcb56bbc..a371e21e0566 100644
--- a/extensions/amp-analytics/0.1/variables.js
+++ b/extensions/amp-analytics/0.1/variables.js
@@ -14,7 +14,6 @@
  * limitations under the License.
  */
 
-import {SANDBOX_AVAILABLE_VARS} from './sandbox-vars-whitelist';
 import {Services} from '../../../src/services';
 import {asyncStringReplace} from '../../../src/string';
 import {base64UrlEncodeFromString} from '../../../src/utils/base64';
@@ -241,9 +240,11 @@ export class VariableService {
    * @param {string} template The template to expand.
    * @param {!ExpansionOptions} options configuration to use for expansion.
    * @param {!Element} element amp-analytics element.
+   * @param {?} opt_bindings
+   * @param {?} opt_whitelist
    * @return {!Promise<string>} The expanded string.
    */
-  expandTemplate(template, options, element) {
+  expandTemplate(template, options, element, opt_bindings, opt_whitelist) {
     return asyncStringReplace(template, /\${([^}]*)}/g, (match, key) => {
       if (options.iterations < 0) {
         user().error(
@@ -276,29 +277,28 @@ export class VariableService {
             options.iterations - 1,
             true /* noEncode */
           ),
-          element
+          element,
+          opt_bindings,
+          opt_whitelist
         );
       }
 
-      const bindings = this.getMacros(element);
+      const bindings = opt_bindings || this.getMacros(element);
       const urlReplacements = Services.urlReplacementsForDoc(element);
-      const whitelist = element.hasAttribute('sandbox')
-        ? SANDBOX_AVAILABLE_VARS
-        : undefined;
 
       return Promise.resolve(value)
         .then(value => {
           if (isArray(value)) {
             return Promise.all(
               value.map(item =>
-                urlReplacements.expandStringAsync(item, bindings, whitelist)
+                urlReplacements.expandStringAsync(item, bindings, opt_whitelist)
               )
             );
           }
           return urlReplacements.expandStringAsync(
             value + argList,
             bindings,
-            whitelist
+            opt_whitelist
           );
         })
         .then(value => {

From 797c872ac130b640d9fbca0e961f1d9ede2909f4 Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Fri, 9 Aug 2019 16:01:15 -0700
Subject: [PATCH 12/19] fix resourceTiming test

---
 .../0.1/test/test-resource-timing.js          | 21 +++++++++----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/extensions/amp-analytics/0.1/test/test-resource-timing.js b/extensions/amp-analytics/0.1/test/test-resource-timing.js
index a430847824c2..84933a4f1331 100644
--- a/extensions/amp-analytics/0.1/test/test-resource-timing.js
+++ b/extensions/amp-analytics/0.1/test/test-resource-timing.js
@@ -103,20 +103,19 @@ describes.realWin('resourceTiming', {amp: true}, env => {
     expectedResult
   ) {
     sandbox.stub(win.performance, 'getEntriesByType').returns(fakeEntries);
-    return getResourceTiming(
-      ampdoc,
-      element,
-      resourceTimingSpec,
-      Date.now()
-    ).then(result => {
-      expect(result).to.equal(expectedResult);
-    });
+    return getResourceTiming(element, resourceTimingSpec, Date.now()).then(
+      result => {
+        expect(result).to.equal(expectedResult);
+      }
+    );
   };
 
   beforeEach(() => {
     win = env.win;
     ampdoc = env.ampdoc;
-    element = env.win.document.documentElement;
+    element = document.createElement('amp-analytics');
+    element.getAmpDoc = () => ampdoc;
+    env.win.document.body.appendChild(element);
     installVariableServiceForTesting(ampdoc);
     installLinkerReaderService(win);
   });
@@ -614,13 +613,13 @@ describes.realWin('resourceTiming', {amp: true}, env => {
     const spec = newResourceTimingSpec();
     spec['encoding']['entry'] = '${initiatorType}.${startTime}.${duration}';
 
-    return getResourceTiming(ampdoc, element, 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, element, spec, Date.now());
+        return getResourceTiming(element, spec, Date.now());
       })
       .then(result => {
         expect(result).to.equal('script.200.500');

From d16512018ef4697cd47d0f85e440e3937c0174ce Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Fri, 9 Aug 2019 16:42:23 -0700
Subject: [PATCH 13/19] manual doc file

---
 .../amp-analytics/analytics-nested.html       | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 test/manual/amp-analytics/analytics-nested.html

diff --git a/test/manual/amp-analytics/analytics-nested.html b/test/manual/amp-analytics/analytics-nested.html
new file mode 100644
index 000000000000..49e3d1e7e2c8
--- /dev/null
+++ b/test/manual/amp-analytics/analytics-nested.html
@@ -0,0 +1,55 @@
+<!doctype html>
+<html ⚡>
+<head>
+  <meta charset="utf-8">
+  <title>AMP Analytics</title>
+  <link rel="canonical" href="analytics.amp.html" >
+  <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
+  </style>
+  <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
+  <script async src="https://cdn.ampproject.org/v0.js"></script>
+  <script async custom-element="amp-analytics" src="https://cdn.ampproject.org/v0/amp-analytics-0.1.js"></script>
+</head>
+<body>
+  <!-- 
+    Macros in `vars` are now resolved asynchronously before macros in `requests`. As a result 
+    it is possible to have expensive macros in `vars` that will delay macro resolution in requests
+    by a meaningful amount of time. In the example below `ts2` will be resolved slightly ahead of `ts1`.
+    This would be further delayed if there were more complicated nested macros inside of the `vars` config.
+
+    RESOURCE_TIMING will only resolve to anything other than an empty string as part of a request. This
+    excludes this macro from selectors.
+
+    Our regex for finding macros in the ${} format will not work when nested. See broken below.
+
+    Multiple ${} are fine as long as they are not nested, see adjacent below.
+
+    You can use the ${} format in conjuction with nested macros, as long as there are no nested ${}.
+    See combo below.
+   -->
+  <amp-analytics>
+    <script type="application/json">
+      {
+        "requests": {
+          "base": "https://www.example.com",
+          "complicated": "${base}?ts1=TIMESTAMP&ts2=${ts2}&n=${nested}&b=${broken}&c=${combo}&a=${adjacent}"
+        },
+        "triggers": {
+          "visible": {
+            "on": "visible",
+            "request": "complicated"
+          }
+        },
+        "vars": {
+          "ts2": "TIMESTAMP",
+          "nested": "$IF(QUERY_PARAM(foo), QUERY_PARAM(foo), default)",
+          "broken": "${queryParam(foo), ${a}}",
+          "combo": "${deeper}",
+          "deeper":  "${nested}",
+          "adjacent": "${ts2}sometext${ts2}"
+        }
+      }
+    </script> 
+  </amp-analytics>
+  <h1>Nested macros in analytics</h1>
+</body>

From e760e34c201890835af2d8417db7e4c75d4ee504 Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Mon, 12 Aug 2019 09:41:49 -0700
Subject: [PATCH 14/19] jsdoc

---
 build-system/tasks/presubmit-checks.js    | 2 +-
 extensions/amp-analytics/0.1/variables.js | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js
index 3c64627b7515..b9269b1795df 100644
--- a/build-system/tasks/presubmit-checks.js
+++ b/build-system/tasks/presubmit-checks.js
@@ -922,7 +922,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/varibles.js',
+      'extensions/amp-analytics/0.1/variables.js',
     ],
   },
   '\\.expandInputValueSync\\(': {
diff --git a/extensions/amp-analytics/0.1/variables.js b/extensions/amp-analytics/0.1/variables.js
index a371e21e0566..9f65657f6ee6 100644
--- a/extensions/amp-analytics/0.1/variables.js
+++ b/extensions/amp-analytics/0.1/variables.js
@@ -240,8 +240,8 @@ export class VariableService {
    * @param {string} template The template to expand.
    * @param {!ExpansionOptions} options configuration to use for expansion.
    * @param {!Element} element amp-analytics element.
-   * @param {?} opt_bindings
-   * @param {?} opt_whitelist
+   * @param {!JsonObject=} opt_bindings
+   * @param {!Object=} opt_whitelist
    * @return {!Promise<string>} The expanded string.
    */
   expandTemplate(template, options, element, opt_bindings, opt_whitelist) {

From 3ca88913cf17ebe0077757d4e5fbae20083c0886 Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Mon, 19 Aug 2019 15:10:03 -0700
Subject: [PATCH 15/19] pass binding for requests

---
 extensions/amp-analytics/0.1/requests.js | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/extensions/amp-analytics/0.1/requests.js b/extensions/amp-analytics/0.1/requests.js
index 68fdf5c41237..a387c750c039 100644
--- a/extensions/amp-analytics/0.1/requests.js
+++ b/extensions/amp-analytics/0.1/requests.js
@@ -168,7 +168,9 @@ export class RequestHandler {
         .expandTemplate(
           this.requestOrigin_,
           requestOriginExpansionOpt,
-          this.element_
+          this.element_,
+          bindings,
+          this.whiteList_
         )
         // substitute in URL values e.g. DOCUMENT_REFERRER -> https://example.com
         .then(expandedRequestOrigin => {

From 2ee0787ae556e88e1669e6381d69bd250c8ac250 Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Mon, 19 Aug 2019 15:16:23 -0700
Subject: [PATCH 16/19] asyncStringReplace test

---
 test/unit/test-string.js | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/test/unit/test-string.js b/test/unit/test-string.js
index bfd74af6bf23..acad32ff712d 100644
--- a/test/unit/test-string.js
+++ b/test/unit/test-string.js
@@ -15,6 +15,7 @@
  */
 
 import {
+  asyncStringReplace,
   camelCaseToDash,
   dashToCamelCase,
   endsWith,
@@ -157,3 +158,12 @@ describe('trimEnd', () => {
     expect(trimEnd('\n\tabc')).to.equal('\n\tabc');
   });
 });
+
+describe('asyncStringReplace', () => {
+  it('should replace asynchronously', () => {
+    const result = asyncStringReplace('the quick brown fox', /brown/, () =>
+      Promise.resolve('red')
+    );
+    expect(result).to.eventually.equal('the quick red fox');
+  });
+});

From 7179c9c48443943fabfc236ce2fe39f74c4f0766 Mon Sep 17 00:00:00 2001
From: calebcordry <ccordry@google.com>
Date: Mon, 19 Aug 2019 16:15:37 -0700
Subject: [PATCH 17/19] fix async replace

---
 src/string.js            | 14 +++++++++++---
 test/unit/test-string.js | 34 ++++++++++++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/src/string.js b/src/string.js
index e100d6876a8b..61d2ca777c7f 100644
--- a/src/string.js
+++ b/src/string.js
@@ -183,11 +183,19 @@ export function trimStart(str) {
 export function asyncStringReplace(str, regex, replacer) {
   const stringBuilder = [];
   let lastIndex = 0;
-
-  str.replace(regex, (match, key, matchIndex) => {
+  str.replace(regex, function() {
+    // String.prototype.replace will pass 3 to n number of arguments to the
+    // callback function based on how many capture groups the regex may or may
+    // not contain. We know that the match will always be first, and the
+    // index will always be second to last.
+    const match = arguments[0];
+    const matchIndex = arguments[arguments.length - 2];
     stringBuilder.push(str.slice(lastIndex, matchIndex));
     // Store the promise in it's eventual string position.
-    const replacementPromise = replacer(match, key);
+    const replacementPromise =
+      typeof replacer === 'function'
+        ? replacer.apply(null, arguments)
+        : replacer;
     stringBuilder.push(replacementPromise);
     lastIndex = matchIndex + match.length;
   });
diff --git a/test/unit/test-string.js b/test/unit/test-string.js
index acad32ff712d..fda2a75c03a9 100644
--- a/test/unit/test-string.js
+++ b/test/unit/test-string.js
@@ -160,10 +160,40 @@ describe('trimEnd', () => {
 });
 
 describe('asyncStringReplace', () => {
-  it('should replace asynchronously', () => {
+  it('should replace with string as callback', () => {
+    const result = asyncStringReplace('the quick brown fox', /brown/, 'red');
+    return expect(result).to.eventually.equal('the quick red fox');
+  });
+
+  it('should replace with sync function as callback', () => {
+    const result = asyncStringReplace(
+      'the quick brown fox',
+      /brown/,
+      () => 'red'
+    );
+    return expect(result).to.eventually.equal('the quick red fox');
+  });
+
+  it('should replace with no capture groups', () => {
     const result = asyncStringReplace('the quick brown fox', /brown/, () =>
       Promise.resolve('red')
     );
-    expect(result).to.eventually.equal('the quick red fox');
+    return expect(result).to.eventually.equal('the quick red fox');
+  });
+
+  it('should replace with one capture group', () => {
+    const result = asyncStringReplace('item 798', /item (\d*)/, (match, p1) =>
+      Promise.resolve(p1)
+    );
+    return expect(result).to.eventually.equal('798');
+  });
+
+  it('should replace with two capture groups', () => {
+    const result = asyncStringReplace(
+      'John Smith',
+      /(\w+)\s(\w+)/,
+      (match, p1, p2) => Promise.resolve(`${p2}, ${p1}`)
+    );
+    return expect(result).to.eventually.equal('Smith, John');
   });
 });

From 3dc052618c4e7056ee888b70fdff8b2b1609e596 Mon Sep 17 00:00:00 2001
From: Micajuine Ho <micajuineho@google.com>
Date: Wed, 29 Jan 2020 17:28:35 -0800
Subject: [PATCH 18/19] Handle special patterns

---
 src/string.js            | 19 ++++++++++---------
 test/unit/test-string.js | 15 +++++++++++++++
 2 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/src/string.js b/src/string.js
index 41367ab8fd2d..c9b3d857ed90 100644
--- a/src/string.js
+++ b/src/string.js
@@ -177,30 +177,31 @@ export function trimStart(str) {
  * Wrapper around String.replace that handles asynchronous resolution.
  * @param {string} str
  * @param {RegExp} regex
- * @param {Function} replacer
+ * @param {Function|string} replacer
  * @return {!Promise<string>}
  */
 export function asyncStringReplace(str, regex, replacer) {
+  if (typeof replacer === 'string') {
+    return Promise.resolve(str.replace(regex, replacer));
+  }
   const stringBuilder = [];
   let lastIndex = 0;
-  str.replace(regex, function() {
+
+  str.replace(regex, function(match) {
     // String.prototype.replace will pass 3 to n number of arguments to the
     // callback function based on how many capture groups the regex may or may
     // not contain. We know that the match will always be first, and the
     // index will always be second to last.
-    const match = arguments[0];
     const matchIndex = arguments[arguments.length - 2];
     stringBuilder.push(str.slice(lastIndex, matchIndex));
+    lastIndex = matchIndex + match.length;
+
     // Store the promise in it's eventual string position.
-    const replacementPromise =
-      typeof replacer === 'function'
-        ? replacer.apply(null, arguments)
-        : replacer;
+    const replacementPromise = replacer.apply(null, arguments);
     stringBuilder.push(replacementPromise);
-    lastIndex = matchIndex + match.length;
   });
-
   stringBuilder.push(str.slice(lastIndex));
+
   return Promise.all(stringBuilder).then(resolved => resolved.join(''));
 }
 
diff --git a/test/unit/test-string.js b/test/unit/test-string.js
index 47ed250fdcc3..1e44ea8c844a 100644
--- a/test/unit/test-string.js
+++ b/test/unit/test-string.js
@@ -171,6 +171,21 @@ describe('asyncStringReplace', () => {
     return expect(result).to.eventually.equal('the quick red fox');
   });
 
+  it('should use replacer with special pattern', async () => {
+    await expect(
+      asyncStringReplace('the quick brown fox', /brown/, '$$')
+    ).to.eventually.equal('the quick $ fox');
+    await expect(
+      asyncStringReplace('the quick brown fox', /brown/, 'purple $& grey')
+    ).to.eventually.equal('the quick purple brown grey fox');
+    await expect(
+      asyncStringReplace('the quick brown fox', /brown/, "sweet $'")
+    ).to.eventually.equal('the quick sweet  fox fox');
+    await expect(
+      asyncStringReplace('the quick brown fox', /brown/, '$`empathetic')
+    ).to.eventually.equal('the quick the quick empathetic fox');
+  });
+
   it('should replace with sync function as callback', () => {
     const result = asyncStringReplace(
       'the quick brown fox',

From e83de12eae9d254b03257ea3c15e324d866316bd Mon Sep 17 00:00:00 2001
From: Micajuine Ho <micajuineho@google.com>
Date: Wed, 29 Jan 2020 17:28:35 -0800
Subject: [PATCH 19/19] Handle special patterns

---
 .../amp-analytics/0.1/test/test-variables.js  | 31 +++++++++++++++----
 src/string.js                                 | 19 ++++++------
 test/unit/test-string.js                      | 15 +++++++++
 3 files changed, 50 insertions(+), 15 deletions(-)

diff --git a/extensions/amp-analytics/0.1/test/test-variables.js b/extensions/amp-analytics/0.1/test/test-variables.js
index 4e0ee9fe3b33..7a9809c8faa4 100644
--- a/extensions/amp-analytics/0.1/test/test-variables.js
+++ b/extensions/amp-analytics/0.1/test/test-variables.js
@@ -130,19 +130,38 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, env => {
           })
         )
         .then(() =>
-          check('${foo}&${bar(3,4)}', 'FOO(1,2)&BAR(3,4)', {
+          check('${foo}', 'true', {
+            'foo': '$EQUALS($SUBSTR(zyxabc,3),abc)',
+          })
+        )
+        .then(() =>
+          // No macros in the arglist (3,4), QUERY_PARAM works
+          check('${foo}&${bar(3,4)}', 'FOO(1,2)&4', {
             'foo': 'FOO(1,2)',
-            'bar': 'BAR',
+            'bar': 'QUERY_PARAM',
+          })
+        )
+        .then(() =>
+          // Macros in the arglist, so getNameArgs returns
+          // an undefined variable/macro bar(5, QUERY_PARAM(6,7))
+          check('${foo}&${bar(5,QUERY_PARAM(6,7))}', 'FOO(3,4)&', {
+            'foo': 'FOO(3,4)',
+            'bar': 'QUERY_PARAM',
           })
         )
         .then(() =>
           // See comment about getNameArgs above.
-          check('${all}', 'AAA(1%2C2)%26BBB(3%2C4)%26CCC(5%2C6)%26DDD(7,8)', {
-            'a': 'AAA',
-            'b': 'BBB',
+          check('${all}', '2%264', {
+            'a': 'QUERY_PARAM',
+            'b': 'QUERY_PARAM(3,4)',
+            'all': '${a(1,2)}&${b}',
+          })
+        )
+        .then(() =>
+          check('${all}&${c}&${d}', 'CCC(5%2C6)%26DDD(7,8)&CCC(5,6)&DDD(7,8)', {
             'c': 'CCC(5,6)',
             'd': 'DDD(7,8)',
-            'all': '${a(1,2)}&${b(3,4)}&${c}&${d}',
+            'all': '${c}&${d}',
           })
         )
         .then(() =>
diff --git a/src/string.js b/src/string.js
index 41367ab8fd2d..c9b3d857ed90 100644
--- a/src/string.js
+++ b/src/string.js
@@ -177,30 +177,31 @@ export function trimStart(str) {
  * Wrapper around String.replace that handles asynchronous resolution.
  * @param {string} str
  * @param {RegExp} regex
- * @param {Function} replacer
+ * @param {Function|string} replacer
  * @return {!Promise<string>}
  */
 export function asyncStringReplace(str, regex, replacer) {
+  if (typeof replacer === 'string') {
+    return Promise.resolve(str.replace(regex, replacer));
+  }
   const stringBuilder = [];
   let lastIndex = 0;
-  str.replace(regex, function() {
+
+  str.replace(regex, function(match) {
     // String.prototype.replace will pass 3 to n number of arguments to the
     // callback function based on how many capture groups the regex may or may
     // not contain. We know that the match will always be first, and the
     // index will always be second to last.
-    const match = arguments[0];
     const matchIndex = arguments[arguments.length - 2];
     stringBuilder.push(str.slice(lastIndex, matchIndex));
+    lastIndex = matchIndex + match.length;
+
     // Store the promise in it's eventual string position.
-    const replacementPromise =
-      typeof replacer === 'function'
-        ? replacer.apply(null, arguments)
-        : replacer;
+    const replacementPromise = replacer.apply(null, arguments);
     stringBuilder.push(replacementPromise);
-    lastIndex = matchIndex + match.length;
   });
-
   stringBuilder.push(str.slice(lastIndex));
+
   return Promise.all(stringBuilder).then(resolved => resolved.join(''));
 }
 
diff --git a/test/unit/test-string.js b/test/unit/test-string.js
index 47ed250fdcc3..1e44ea8c844a 100644
--- a/test/unit/test-string.js
+++ b/test/unit/test-string.js
@@ -171,6 +171,21 @@ describe('asyncStringReplace', () => {
     return expect(result).to.eventually.equal('the quick red fox');
   });
 
+  it('should use replacer with special pattern', async () => {
+    await expect(
+      asyncStringReplace('the quick brown fox', /brown/, '$$')
+    ).to.eventually.equal('the quick $ fox');
+    await expect(
+      asyncStringReplace('the quick brown fox', /brown/, 'purple $& grey')
+    ).to.eventually.equal('the quick purple brown grey fox');
+    await expect(
+      asyncStringReplace('the quick brown fox', /brown/, "sweet $'")
+    ).to.eventually.equal('the quick sweet  fox fox');
+    await expect(
+      asyncStringReplace('the quick brown fox', /brown/, '$`empathetic')
+    ).to.eventually.equal('the quick the quick empathetic fox');
+  });
+
   it('should replace with sync function as callback', () => {
     const result = asyncStringReplace(
       'the quick brown fox',