♻️ Allow dynamically setting 3p bootstrap script #32573
Conversation
|
This pull request introduces 2 alerts when merging a318d6f into f0cdb5f - view on LGTM.com new alerts:
|
|
This pull request introduces 2 alerts when merging b110cd9 into f0cdb5f - view on LGTM.com new alerts:
|
3p/integration.js
Outdated
| @@ -926,3 +926,8 @@ function lightweightErrorReport(e, isCanary) { | |||
| '&s=' + | |||
| encodeURIComponent(e.stack || ''); | |||
| } | |||
|
|
|||
| window.draw3p(); | |||
There was a problem hiding this comment.
- How do you assure that this code is executed after
<div id="c">has been parsed? - Does
draw3pever executedocument.writetoday?
There was a problem hiding this comment.
You are right. I switched it to document.write so script is loaded synchronously.
There was a problem hiding this comment.
Sorry, I meant something different. Does the draw3p() contents ever use document.write()? It's very strategically placed there inside the specific <div id="c"> and that makes me think that it's aimed to enable document.write.
There was a problem hiding this comment.
Yes, document.write is being used. I changed this also to load the js w/ document.write so that document.write is still available during draw3p(). The previous version did break the document.write call.
3p/frame.max.html
Outdated
| @@ -2,12 +2,14 @@ | |||
| <head> | |||
| <meta charset="utf-8"> | |||
| <meta name="robots" content="noindex"> | |||
| <script src="./integration.js"></script> | |||
| <script> | |||
| var s = document.createElement('script'); | |||
There was a problem hiding this comment.
This might need some protection from exceptions.
There was a problem hiding this comment.
Make sense. Added.
powerivq
force-pushed
the
vendor-on
branch
8 times, most recently
from
fbd8bdf
to
010fa9c
Compare
|
This pull request introduces 3 alerts when merging 010fa9c into 494c865 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging b32560d into 432c6f4 - view on LGTM.com new alerts:
|
powerivq
force-pushed
the
vendor-on
branch
2 times, most recently
from
90b5bae
to
cd2c647
Compare
|
This pull request introduces 3 alerts when merging cd2c647 into 432c6f4 - view on LGTM.com new alerts:
|
|
This pull request introduces 3 alerts when merging 76a5981 into 432c6f4 - view on LGTM.com new alerts:
|
This PR gets rid of the hard coded
integration.js/f.js, and replace it with a URL that is assigned by AMP runtime. In addition, it moves script calls from HTML to the Javascript.The risk here is that some 3p script might depend on
draw3p()being ran within the container div or a specific script tag, but I see no sign any 3p integration actually does that. Tested several ads and 3p (facebook and twitter extensions) and all worked fine.Pulling in @dvoytenko do you see this being a problems with this?