Changes to implement A4A (AMP ads for AMP pages) #3534
Conversation
| 'ads/**->src/log.js', | ||
| 'ads/**->src/mode.js', | ||
| 'ads/**->src/timer.js', |
There was a problem hiding this comment.
Should we be whitelisting these for everything under ads, or only for specific modules? (More specifically, the mode.js whitelist here subsumes the one for traffic-experiments.js only, below.)
| const slotTop = slot.y + (viewport ? viewport.y : 0); | ||
| const slotLeft = slot.x + (viewport ? viewport.x : 0); | ||
| return formatFixedWidthInteger(slotNumber, 2) + | ||
| // ad slot top, in 1/5 viewport height units |
|
Alright, made it through a first pass :) Very impressive and awesome CL. |
|
@jridgewell Okay, we have merged to master, run our tests, cleaned up lint, and cleaned up types. The Travis build still finds check-types errors in src/service.js, but I'm pretty sure that those are independent of us. As far as I can tell, we're ready for you to push the merge button. Thanks for all your help! |
|
We will need to get the build green. I can fix the compilation errors for you (because things are still so early and these are very hard to fix). But for now, add the files that don't compile here There are also And there appear to be test failures. |
|
Yeah... The compilation system seems a bit... Difficult. Every time I update one set of files, I get brand new errors in other files (some of which I know we haven't touched). E.g., initially it reported a pile of type errors in src/service.js and 3p/environment.js. When I whitelist those in compile.js, it then discovered a bunch of errors in ads/google/utils.js and ads/google/traffic-experiments.js. As I'm beginning to fix those, I'm now getting errors in src/document-state.js. It's hard to see how we could have caused some of these errors, since I'm virtually certain that we didn't touch those files. So I'm guessing that they were pre-existing and just weren't revealed until this run? Which means they're in production now? Regardless, we'll fix what we can and whitelist the ones that we're pretty sure we didn't cause. |
- Changed element-stub to insert lower-case version of tag name. - Removed (deprecated?) test that insert-extension only inserts amp-ad/amp-embed -- not sure why that restriction was there in the first place. - Changed cid test to check amp-ad-3p-impl, rather than amp-ad, because the cid will be written to the child / delegate node rather than the parent.
…ected by closure. :-P
It is causing problems with presubmit checks. So deal with this part later.
* master: (236 commits) trim all the columns (ampproject#3894) Refactoring: Turn private custom element methods into functions. (ampproject#3882) Lower the load priority of ad shaped iframes. (ampproject#3863) JsDoc fix (ampproject#3892) Add screenshots for Opera to AMP Validator extension. (ampproject#3866) Fix renaming of generated JSCompiler_prototypeAlias variable. (ampproject#3887) fix typo in amp-sidebar.md (ampproject#3833) Validator Roll-up (ampproject#3885) [CryptoService] Leverage browser native Crypto API to hash strings. (ampproject#3850) Size update (ampproject#3883) copy amp-ad docs to builtins (ampproject#3879) move doc to extension (ampproject#3878) [amp-experiment] Exposes isDismissed() method in AmpUserNotification (ampproject#3832) fix action-impl warning on dist (ampproject#3867) Add params for microad (ampproject#3827) Fixed some A4A tests. (ampproject#3859) Updates to colanalytics vendor config for amp-analytics. (ampproject#3849) Changes to implement A4A (AMP ads for AMP pages) (ampproject#3534) Addresses comment left over from PR#3841 (ampproject#3853) Expose submit event with on=submit:el.action syntax. (ampproject#3739) ...
| if (data.width !== undefined) { | ||
| newWidth = Math.max(this.element_./*OK*/offsetWidth + | ||
| data.width - this.iframe_./*OK*/offsetWidth, data.width); | ||
| this.iframe_.width = newWidth; |
There was a problem hiding this comment.
@bobcassels I'm wondering why we changed
this.iframe_.width = data.width;
to
this.iframe_.width = newWidth;
See the code before the change.
Was it intentional to cope with something?
cc @zhouyx
There was a problem hiding this comment.
#4840 is trying to revert this logic back as we think it's probably a bug.
This implements infrastructure to support AMP Ads for AMP Pages (A4A) as outlined in #3133. This is not ready for general use yet but rather initial work on implementing an A4A experiment. In particular, there is currently no public signing service. An example implementation for a specific ad network is at https://github.com/google/amphtml/tree/a4a-adsense. However, ad networks may wish to wait to implement their own A4A integration until a public AMP creative signing service is available.
This is a no-retagging change: existing publisher pages should continue to work as written, with unchanged behavior.
Considerations:
<amp-ad>) serves as a loader which will add either the 3p or a network-specific A4A extension element as a child. The 3p implementation (named AmpAd3PImpl) implements the same functionality as the original AMPHTML<amp-ad>tag, and is included with the amp-ad extension bundle to ensure no change in latency.TODO: