Modified A4A from shadow DOM to installFriendlyIframeEmbed by keithwrightbos · Pull Request #5196 · ampproject/amphtml

keithwrightbos · 2016-09-23T17:57:45Z

Only implementation, still need to update tests.

The previous change was made in error (wrong branch).

jridgewell · 2016-09-23T19:47:56Z

/to @lannka

lannka

LGTM in general. some minor comments

lannka · 2016-09-23T22:06:20Z

extensions/amp-a4a/0.1/amp-a4a.js

-            bodyElement./*OK*/innerHTML = bodyBlock;
-            this.rendered_ = true;
-            this.onAmpCreativeShadowDomRender();
+          return this.vsync_.mutatePromise().then(() => {


you might want to do this.vsync_.mutatePromise(() => {

Yes. It definitely should be mutatePromise(() => {...}). But generally, only appendChild would need to go in vsync. Since installFriendlyIframeEmbed is not quite ready for that, I recommend to just to just remove vsync.mutate completely and I will instead implement vsync.mutate inside installFriendlyIframeEmbed.

Did you push already? Not seeing changes...

lannka · 2016-09-23T22:07:46Z

extensions/amp-a4a/0.1/amp-a4a.js

-            this.onAmpCreativeShadowDomRender();
+          return this.vsync_.mutatePromise().then(() => {
+            // Create and setup friendly iframe.
+            const iframe = this.element.ownerDocument.createElement('iframe');


you can leverage the util createElementWithAttributes in dom.js to save some lines.

lannka · 2016-09-23T22:15:18Z

extensions/amp-a4a/0.1/amp-a4a.js

+            for (let i = 0; i <= extensions.length; i++) {
+              // Remove AMP runtime as well.
+              const extensionName =
+                (i == extensions.length) ? 'v0\.js' : extensions[i];


hmm, this is not that intuitive.
instead, we can do

extensions.push('v0\.js`);

before the for loop

Done... note that I have to then pop it from the array as it is passed to installFriendlyIframeEmbed

dvoytenko · 2016-09-24T00:32:19Z

extensions/amp-a4a/0.1/amp-a4a.js

-            bodyElement./*OK*/innerHTML = bodyBlock;
-            this.rendered_ = true;
-            this.onAmpCreativeShadowDomRender();
+          return this.vsync_.mutatePromise().then(() => {


Yes. It definitely should be mutatePromise(() => {...}). But generally, only appendChild would need to go in vsync. Since installFriendlyIframeEmbed is not quite ready for that, I recommend to just to just remove vsync.mutate completely and I will instead implement vsync.mutate inside installFriendlyIframeEmbed.

dvoytenko · 2016-09-24T00:34:25Z

extensions/amp-a4a/0.1/amp-a4a.js

+              creativeMetaData.customElementExtensions || [];
+            for (let i = 0; i <= extensions.length; i++) {
+              // Remove AMP runtime as well.
+              const extensionName =


I don't quiet understand these changes. Why not just use the old way of doing this as was before, but simply combine cssBlock and bodyBlock into one string before sending it to installFriendlyIframeEmbed?

As discussed, I want to minimize that amount of work done by the validator rewrite. Had we done this from the beginning, we would only have required it provide an array of extension ids and how to remove script tags (including v0.js).

dvoytenko · 2016-09-28T16:26:45Z

extensions/amp-a4a/0.1/amp-a4a.js

    // valid AMP.
    this.timerId_ = incrementLoadingAds(this.win);
    return this.adPromise_.then(rendered => {
+      console.log('layoutCallback promise result', rendered, this.rendered_);


Please remove.

dvoytenko · 2016-09-28T16:28:03Z

extensions/amp-a4a/0.1/amp-a4a.js

-            bodyElement./*OK*/innerHTML = bodyBlock;
-            this.rendered_ = true;
-            this.onAmpCreativeShadowDomRender();
+          return this.vsync_.mutatePromise().then(() => {


Did you push already? Not seeing changes...

keithwrightbos · 2016-09-28T17:56:39Z

Yes apologies, just pushed update to a4a.js. Working on test modifications now.

zhouyx · 2016-09-28T23:21:54Z

extensions/amp-a4a/0.1/amp-a4a.js

-            this.rendered_ = true;
-            this.onAmpCreativeShadowDomRender();
+          // Create and setup friendly iframe.
+          const iframe = createElementWithAttributes(


what i understand is A4A will always create an iframe (friendly / cross domain) now. Can we make iframe a variable of the class this.iframe = iframe here to make code sharing class cleaner?

hm, I actually start to think this is a good idea now. we could instantiate the class with this.iframe = null. Making it a public field so that other shared modules can access.

But not necessary in this PR. We can change it in your own refactoring PR for code-sharing .

lannka

LGTM with some indentation nitpicks.

lannka · 2016-09-30T21:35:05Z

extensions/amp-a4a/0.1/amp-a4a.js

+          // validation providing offsets to extension & AMP runtime
+          // locations.
+          const extensions =
+            creativeMetaData.customElementExtensions || [];


for forced line breaks, I think we indent 4 spaces.

lannka · 2016-09-30T21:36:14Z

extensions/amp-a4a/0.1/amp-a4a.js

+          extensions.push('v0\.js');
+          extensions.forEach(extension => {
+            creative = creative.replace(new RegExp(
+              `<script[^>]+${extensionName}[^>]+>\s*</script\s*>\n?`),


4 spaces here as well.

the previous line is fine though, as it's not a forced line break.

lannka · 2016-09-30T21:36:40Z

extensions/amp-a4a/0.1/amp-a4a.js

+          // have moved to AMP AD document format validation, ensure it does
+          // not match amp4ads-boilerplate.
+          creative = creative.replace(
+            /<style\s+([^>]*\s+)?amp-boilerplate[^>]*>[^>]+<\/style\s*>/, '');


lannka · 2016-09-30T21:36:49Z

extensions/amp-a4a/0.1/amp-a4a.js

+          creative = creative.replace(
+            /<style\s+([^>]*\s+)?amp-boilerplate[^>]*>[^>]+<\/style\s*>/, '');
+          return installFriendlyIframeEmbed(
+            iframe, this.element, {


The previous change was made in error (wrong branch).

lannka · 2016-10-04T18:44:23Z

We need this PR to get in to unblock some other work.
@keithwrightbos would you fix the lint and resolve the conflicts?
@dvoytenko do you still want to take another look?

…iendly-iframe-embed

googlebot · 2016-10-04T19:38:45Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

keithwrightbos · 2016-10-04T19:39:56Z

@lannka @dvoytenko - after discussions with AMP team, modified code to stitch together creative from validator rewrite meta object as opposed to using regexps to remove portions. Validator team is working on modifying output to place portions to be removed (scripts & boilerplate) next to each other and provide offsets to ease removal. PTAL

googlebot · 2016-10-04T21:02:04Z

CLAs look good, thanks!

keithwrightbos · 2016-10-04T21:07:05Z

@lannka I fixed the lint errors and merged to upstream/master so I believe conflicts have been addressed (correct me if I'm wrong!). FYI - I'm OOTO tomorrow through Monday so please contact @tdrl for assistance going forward. Thanks!

dvoytenko

Just a couple of nits.

dvoytenko · 2016-10-04T22:10:48Z

extensions/amp-a4a/0.1/amp-a4a.js

+          dev().assert(!!this.element.ownerDocument);
+          const iframe = /** @type {!HTMLIFrameElement} */(
+            createElementWithAttributes(
+              /** @type {!Document} */(this.element.ownerDocument), 'iframe', {


nit: I property per line.

dvoytenko · 2016-10-04T22:11:43Z

extensions/amp-a4a/0.1/amp-a4a.js

+          }
+          const modifiedCreative =
+            `<!doctype html><html ⚡4ads>
+            <head>


Generally, this is not needed for friendly iframes. My code manages the visibility state.

lannka · 2016-10-04T23:04:58Z

I'll merge this PR now to unblock other's work, and we can get the nit fixed in separate PR.

…t#5196) * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial integration with installFriendlyIframeEmbed * Review comments * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial integration with installFriendlyIframeEmbed * Review comments * Change to stitching creative instead of removing portions * Update unit tests * Fix after merge with upstream/master; correct type failures within friendly-iframe-embed * Fix lint errors * remove console log

glevitzky and others added 9 commits August 31, 2016 18:00

Update amp-ad-network-doubleclick-impl.md

2687820

Reverting doc changes

8d8c6dd

The previous change was made in error (wrong branch).

Merge branch 'master' of github.com:google/amphtml

91938ab

Merge branch 'master' of github.com:google/amphtml

cbc5e27

Merge remote-tracking branch 'upstream/master'

5476512

Merge remote-tracking branch 'upstream/master'

f670b2c

Merge remote-tracking branch 'upstream/master'

90057aa

Merge remote-tracking branch 'upstream/master'

f6db020

Initial integration with installFriendlyIframeEmbed

8a6446e

jridgewell added the NEEDS REVIEW label Sep 23, 2016

jridgewell assigned lannka Sep 23, 2016

lannka suggested changes Sep 23, 2016

View reviewed changes

dvoytenko suggested changes Sep 24, 2016

View reviewed changes

Merge remote-tracking branch 'origin/master' into a4a_friendlyIframe

fa15a4f

dvoytenko suggested changes Sep 28, 2016

View reviewed changes

keithwrightbos mentioned this pull request Sep 28, 2016

Allow for white listed replacement of URL placements and use it in A4A. #5275

Merged

Review comments

b1de431

zhouyx reviewed Sep 28, 2016

View reviewed changes

avimehta mentioned this pull request Sep 29, 2016

amp-analytics vars inside A4A. #5318

Closed

lannka approved these changes Sep 30, 2016

View reviewed changes

lannka added LGTM and removed NEEDS REVIEW labels Sep 30, 2016

zhouyx mentioned this pull request Sep 30, 2016

3pAd A4A sharing logic #5356

Merged

glevitzky and others added 5 commits October 4, 2016 11:31

Update amp-ad-network-doubleclick-impl.md

fd50cc8

Reverting doc changes

a3957ab

The previous change was made in error (wrong branch).

Initial integration with installFriendlyIframeEmbed

72ccb42

Review comments

d6bcaba

Change to stitching creative instead of removing portions

d99ae8c

Update unit tests

4758e2a

keithwrightbos added 2 commits October 4, 2016 15:33

Fix after merge with upstream/master; correct type failures within fr…

c2c2b32

…iendly-iframe-embed

Fix merge conflict

587fc72

keithwrightbos added 3 commits October 4, 2016 16:57

Fix lint errors

d78737a

Merge branch 'master' into a4a_friendlyIframe

894b2c2

remove console log

d44f3e6

dvoytenko approved these changes Oct 4, 2016

View reviewed changes

lannka merged commit 49a0a48 into ampproject:master Oct 4, 2016

Conversation

keithwrightbos commented Sep 23, 2016

Uh oh!

jridgewell commented Sep 23, 2016

Uh oh!

lannka left a comment

Choose a reason for hiding this comment

Uh oh!

lannka Sep 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithwrightbos commented Sep 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lannka Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lannka left a comment

Choose a reason for hiding this comment

Uh oh!

lannka Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lannka commented Oct 4, 2016

Uh oh!

googlebot commented Oct 4, 2016

Uh oh!

keithwrightbos commented Oct 4, 2016

Uh oh!

googlebot commented Oct 4, 2016

Uh oh!

keithwrightbos commented Oct 4, 2016

Uh oh!

dvoytenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lannka commented Oct 4, 2016

Uh oh!

Reviewers

Assignees

lannka Sep 23, 2016 •

edited

Loading

lannka Sep 30, 2016 •

edited

Loading

lannka Sep 30, 2016 •

edited

Loading