Did some investigation on why the tests are failing, they seem to all pass on my machine, as long as I keep the window in focus. Will investigate more later, but this should still be good for review 😄

@torch2424 I do remember we agreed to change variable a document level service. But could you remind the reason why we are doing that 😂

And usually I wouldn't mind that much. But the refactoring and change to move variables both sounds risky to me. Could we break this very PR to two please : )Thank you

Discussed offline:

We can split this PR into:

1. Moving the variables:
2. Moving the variable service onto doc level

Also, as a reminder, we are moving the variable service to doc level for things like stateful variables that will be upcoming in things like amp-next-page (Scroll X, Scroll Y).

Removing @zhouyx as a reviewer. Going to keep this open to use travis to help me debug tests. And then will close this and move into two seperate PRs

@zhouyx So I was fixing the tests for this PR, and realized something that I'd like your opinion on.

So in extensions/amp-a4a/0.1/a4a-variable-source.js, we have an allowed list for al of the URL replacements we support in A4A. But we moved some of these into AMP Analytics. Thus, the tests fail when we cannot initialize the variables when creating our A4A variable source.

Thus, in order to keep these variables only in AMP analytics, we must also remove them here. And, If we remove SCROLL_TOP and SCROLL_LEFT from the A4A Variable source, the tests pass:

I know we decided this would be okay for the AMP use case, but is this also okay for A4A? I'll go ahead and commit this change to the PR, and if the tests no pass, I'll go ahead and split the PR. Thanks! 😄

Great point! According to @jonkeller SCROLL_TOP and SCROLL_LEFT is only used with <amp-analytics> in A4A. So it should be fine to remove them from the A4A list.

I agree we need to be extra careful to not move non-whitelisted variables to the analytics variable list. Could you please add a comment there to the variable list that it will be available to all services who is using amp-analytics including A4A? Thank you!

Another thing that would be helpful is to document what variables are supported in A4A. And if so will the variable resolve to different value when compared to AMP doc. I couldn't find such documentation, if there's one please let me know. Thanks!

@zhouyx Awesome! Thanks for the feedback, yeah I can implement all of those changes.

And just as a personal note, The last failing test is the --single-pass --compiled integration test. Appears that it is not doing the replacements for scrollTop in the single pass case. Will be sure to reach out to @erwinmombay after the holidays.

If we come back to this, @calebcordry is super rad, and copy pasta's a lot of the code from here to move the service to the doc level in #22137 . Thank you!

This PR just needs to be updated to move the variables around now.

Closing this PR as inactive. Please reopen if that isn't the case (preferably as a new PR).