✨ AoG: Add amp-viewer-assistance extension. #20725

hellokoji · 2019-02-07T20:24:01Z

Implements the amp-viewer-assistance extension proposed in Issue (#15195).

The new extension creates a messaging channel between AMP pages and AMP viewers to optimize for flows between the embedded page and the external viewer.

cc @choumx

Change-Id: I78dac3ec9f85c3bbd1ceb4ce88a61b3f7d0dc539

extensions/amp-viewer-assistance/validator-viewer-assistance.protoascii

Change-Id: I874c096e75e9428872b6609df4acb75a82ee5b09

Change-Id: I6692ccec7eaccfce9227115c1ebca24b3a5fe118

Change-Id: I3d282f2571cc48e388ddd222676a23ed0d61b67b

Change-Id: I5335d7d63843800a88319d760504fef7caab4d4f

Change-Id: If066222b53ed7912140c1e526e231aca43d9a38c

extensions/amp-viewer-assistance/0.1/amp-viewer-assistance.js

dreamofabear · 2019-02-13T21:39:47Z

extensions/amp-viewer-assistance/amp-viewer-assistance.md

+# amp-viewer-assistance
+
+The amp-viewer-assistance element provides specification of AMP viewer
+configuration information and invocation of AMP viewer assisted behavior.


This documentation needs some love. :)

We ought to:

Clearly describe the high-level product use case. E.g. this is meant for "assistive behaviors on AMP pages, facilitated by the viewer."

Document AMP page/viewer API contract e.g. "requestSignIn".

Document element actions like "signIn" and "updateActionState".

Follow the format from other extension documentation (see amp-access.md).

/cc @CrystalOnScript

Agreed! More documentation will be committed to this PR soon.

P.S. That is a very nice way of requesting for more documentation! 👍 😄

Just updated the documentation to address some of those comments.

extensions/amp-viewer-assistance/0.1/amp-viewer-assistance.js

dreamofabear · 2019-02-13T21:41:55Z

extensions/amp-viewer-assistance/0.1/amp-viewer-assistance.js

+    }
+    return this.viewer_.isTrustedViewer().then(isTrustedViewer => {
+      if (!isTrustedViewer &&
+         !isExperimentOn(this.ampdoc_.win, 'amp-viewer-assistance-untrusted')) {


Is this intended to be enabled eventually?

No, the experiment was our method of forgoing the trusted viewer check with testing locally since local viewers are inherently not trusted.

If there are other ways of accomplishing the same effect, we are open to suggestions.

Ah, we usually use getMode().localDev for this. It returns true when compiling with --fortesting flag, e.g. gulp dist --extensions=amp-viewer-assistance --fortesting.

Though if you don't do testing with a locally-build AMP runtime, then you'd need local proxy software like Charles to replace the CDN-fetched JS with your local version. Is that the case?

Yes, this is specifically for developing/testing with a local viewer using a remote (or local) runtime. Does that make sense?

extensions/amp-viewer-assistance/0.1/amp-viewer-assistance.js

src/url.js

Change-Id: I1c06c45a2159fcb5286db2a36e168907073a77fa

Change-Id: I9031cc6930c18de3ca691f41274f868670b72c7a

hellokoji · 2019-02-19T18:25:28Z

Friendly ping.

extensions/amp-viewer-assistance/validator-viewer-assistance.protoascii

extensions/amp-viewer-assistance/0.1/test/validator-amp-viewer-assistance.html

extensions/amp-viewer-assistance/0.1/test/validator-amp-viewer-assistance.out

googlebot · 2019-02-19T19:23:32Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

Googlers can find more info about SignCLA and this PR by following this link.

Change-Id: I5c3194a511b64321500025e59375f618ed4660a4

googlebot · 2019-02-19T19:30:49Z

CLAs look good, thanks!

Googlers can find more info about SignCLA and this PR by following this link.

honeybadgerdontcare

thank you, from validation looks fine. @choumx for other changes which still need approval

dreamofabear

Looks good overall. Just one comment and a couple questions.

dreamofabear · 2019-02-20T18:46:51Z

extensions/amp-viewer-assistance/0.1/amp-viewer-assistance.js

+    }
+    return this.viewer_.isTrustedViewer().then(isTrustedViewer => {
+      if (!isTrustedViewer &&
+         !isExperimentOn(this.ampdoc_.win, 'amp-viewer-assistance-untrusted')) {


Ah, we usually use getMode().localDev for this. It returns true when compiling with --fortesting flag, e.g. gulp dist --extensions=amp-viewer-assistance --fortesting.

Though if you don't do testing with a locally-build AMP runtime, then you'd need local proxy software like Charles to replace the CDN-fetched JS with your local version. Is that the case?

extensions/amp-viewer-assistance/0.1/amp-viewer-assistance.js

src/url.js

tools/experiments/experiments.js

Change-Id: Ib20566f920d3c92da648d49dc7dd30fc78eb313f

Change-Id: If962d4a174593b2eed997acfbc01844c0a3b0f72

Change-Id: Icaf921248e6f5f7f096c43cde4688d28f4943545

Change-Id: I65dfc764cb7a1a226b6aac444392f5936b21478d

Change-Id: I78849d7d82c98d3353b2a9ad5a1ed99d5c04e2d4

dreamofabear · 2019-02-21T06:18:17Z

extensions/amp-viewer-assistance/0.1/amp-viewer-assistance.js

+   * @return {?Promise}
+   * @private
+   */
+  actionHandler_(invocation) {


Sorry didn't catch this earlier. You should actually use this.registerAction() here instead. See base-element.js#registerAction.

Then you can also remove the whitelisting in presubmit-checks.js.

As we discussed offline, we are following the pattern set by amp-access to justify using our own action handler since we are using a <script> tag instead of an <amp-viewer-assistance> element.

* Add amp-viewer-assistance extension (ampproject#15195). Change-Id: I78dac3ec9f85c3bbd1ceb4ce88a61b3f7d0dc539 * Address PR comments 1. Change-Id: I874c096e75e9428872b6609df4acb75a82ee5b09 * remove amp-viewer-integration styles Change-Id: I6692ccec7eaccfce9227115c1ebca24b3a5fe118 * Fix javascript closure typing. Change-Id: I3d282f2571cc48e388ddd222676a23ed0d61b67b * Add copyright and license comments to amp-viewer-assistance files. Change-Id: I5335d7d63843800a88319d760504fef7caab4d4f * Remove newline. Change-Id: If066222b53ed7912140c1e526e231aca43d9a38c * Address PR comments 2. Change-Id: I1c06c45a2159fcb5286db2a36e168907073a77fa * Update amp-viewer-assistance documentation. Change-Id: I9031cc6930c18de3ca691f41274f868670b72c7a * Remove redundant amp-viewer-assistnace validator files. Change-Id: I5c3194a511b64321500025e59375f618ed4660a4 * Remove amp-viewer-assistance-unstrusted experiement from experiments.js. Change-Id: Ib20566f920d3c92da648d49dc7dd30fc78eb313f * Address amp-viewer-assistance presubmit errors. Change-Id: If962d4a174593b2eed997acfbc01844c0a3b0f72 * Lint. Change-Id: Icaf921248e6f5f7f096c43cde4688d28f4943545 * Fix types. Change-Id: I65dfc764cb7a1a226b6aac444392f5936b21478d * Add expectAsyncConsoleError to catch error message in test. Change-Id: I78849d7d82c98d3353b2a9ad5a1ed99d5c04e2d4

Add amp-viewer-assistance extension (ampproject#15195).

c6724fd

Change-Id: I78dac3ec9f85c3bbd1ceb4ce88a61b3f7d0dc539

googlebot added the cla: yes label Feb 7, 2019

honeybadgerdontcare requested changes Feb 7, 2019

View reviewed changes

Address PR comments 1.

2ba113b

Change-Id: I874c096e75e9428872b6609df4acb75a82ee5b09

dreamofabear self-requested a review February 7, 2019 22:22

hellokoji added 4 commits February 7, 2019 14:26

remove amp-viewer-integration styles

dbb5d5e

Change-Id: I6692ccec7eaccfce9227115c1ebca24b3a5fe118

Fix javascript closure typing.

8a99fa2

Change-Id: I3d282f2571cc48e388ddd222676a23ed0d61b67b

Add copyright and license comments to amp-viewer-assistance files.

619a444

Change-Id: I5335d7d63843800a88319d760504fef7caab4d4f

Remove newline.

3c134ac

Change-Id: If066222b53ed7912140c1e526e231aca43d9a38c

dreamofabear reviewed Feb 13, 2019

View reviewed changes

hellokoji added 2 commits February 14, 2019 14:05

Address PR comments 2.

faba3f3

Change-Id: I1c06c45a2159fcb5286db2a36e168907073a77fa

Update amp-viewer-assistance documentation.

2ce0926

Change-Id: I9031cc6930c18de3ca691f41274f868670b72c7a

honeybadgerdontcare requested changes Feb 19, 2019

View reviewed changes

googlebot added cla: no and removed cla: yes labels Feb 19, 2019

Remove redundant amp-viewer-assistnace validator files.

b537af0

Change-Id: I5c3194a511b64321500025e59375f618ed4660a4

hellokoji force-pushed the amp-viewer-assistance branch from 0e164be to b537af0 Compare February 19, 2019 19:30

googlebot added cla: yes and removed cla: no labels Feb 19, 2019

honeybadgerdontcare approved these changes Feb 19, 2019

View reviewed changes

dreamofabear approved these changes Feb 20, 2019

View reviewed changes

hellokoji added 5 commits February 20, 2019 12:01

Remove amp-viewer-assistance-unstrusted experiement from experiments.js.

4af6bec

Change-Id: Ib20566f920d3c92da648d49dc7dd30fc78eb313f

Address amp-viewer-assistance presubmit errors.

1905b42

Change-Id: If962d4a174593b2eed997acfbc01844c0a3b0f72

Lint.

360c6a5

Change-Id: Icaf921248e6f5f7f096c43cde4688d28f4943545

Fix types.

b78226d

Change-Id: I65dfc764cb7a1a226b6aac444392f5936b21478d

Add expectAsyncConsoleError to catch error message in test.

1bb8e77

Change-Id: I78849d7d82c98d3353b2a9ad5a1ed99d5c04e2d4

dreamofabear reviewed Feb 21, 2019

View reviewed changes

dreamofabear merged commit 518a6b0 into ampproject:master Feb 21, 2019

hellokoji deleted the amp-viewer-assistance branch February 21, 2019 23:42

hellokoji mentioned this pull request Mar 13, 2019

✨ Add "crossorigin" attribute to remote xhr elements to attach id token. #21107

Merged

dreamofabear mentioned this pull request Jul 6, 2020

<amp-viewer-assistance> extension #15195

Closed

honeybadgerdontcare mentioned this pull request Aug 4, 2020

Review amp-viewer-assistance extension for use post AMP Actions #29676

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ AoG: Add amp-viewer-assistance extension. #20725

✨ AoG: Add amp-viewer-assistance extension. #20725

hellokoji commented Feb 7, 2019

dreamofabear Feb 13, 2019

hellokoji Feb 14, 2019

hellokoji Feb 15, 2019

dreamofabear Feb 13, 2019

hellokoji Feb 14, 2019

dreamofabear Feb 20, 2019

hellokoji Feb 20, 2019

hellokoji commented Feb 19, 2019

googlebot commented Feb 19, 2019

googlebot commented Feb 19, 2019

honeybadgerdontcare left a comment

dreamofabear left a comment

dreamofabear Feb 20, 2019

dreamofabear Feb 21, 2019

hellokoji Feb 21, 2019

✨ AoG: Add amp-viewer-assistance extension. #20725

✨ AoG: Add amp-viewer-assistance extension. #20725

Conversation

hellokoji commented Feb 7, 2019

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

Choose a reason for hiding this comment

hellokoji commented Feb 19, 2019

googlebot commented Feb 19, 2019

googlebot commented Feb 19, 2019

honeybadgerdontcare left a comment

Choose a reason for hiding this comment

dreamofabear left a 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