Implementing a more comprehensive strategy for amp-auto-ads. #7106
Conversation
|
/to @lannka |
| return Promise.resolve(true); | ||
| } | ||
| return this.placeNextAd_().then(success => { | ||
| if (success) { |
There was a problem hiding this comment.
This recursive (exhaustive?) run behavior is new. Is this intentional?
There was a problem hiding this comment.
Yes. The strategy now places multiple ads. So it keeps trying to place another ad until the target number of ads are on the page or it runs out of placements.
| return nextPlacement.getEstimatedPosition().then(yPosition => { | ||
| return this.adTracker_.isTooNearAnAd( | ||
| yPosition, this.strategyConfig_.minAdSpacing).then(tooNear => { | ||
| if (tooNear) { |
There was a problem hiding this comment.
This duplication (it's minimal, but there) could be reduced if we move this "too near" detection into Placement#placeAd.
nextPlacement.placeAd(...).then(state => {
if (state == PLACED) {
} else {
// too near, or some other failure
}
});There was a problem hiding this comment.
I've reworked it a bit so the too near detection is done inside placeAd
| * @param {!Element} element | ||
| * @return {!Promise<!../layout-rect.LayoutRectDef>} | ||
| */ | ||
| getElementLayoutBox(element) { |
| * @return {!Array<!Element>} | ||
| */ | ||
| export function getExistingAds(win) { | ||
| return [].slice.call(win.document.getElementsByTagName('AMP-AD')); |
| */ | ||
| constructor(ads) { | ||
| /** @type {!Array<!Element>} */ | ||
| this.ads_ = ads.slice(0); |
There was a problem hiding this comment.
I don't think the slice is necessary, since it'll always be a new array instance.
| return Promise.resolve(resource.getLayoutBox()); | ||
| } | ||
| return new Promise(resolve => { | ||
| this.vsync_.measure(() => { |
There was a problem hiding this comment.
We have Vsync#measurePromise to make this a little nicer.
| * @param {!Element} element | ||
| * @return {!Promise<!../layout-rect.LayoutRectDef>} | ||
| */ | ||
| getElementLayoutBox(element) { |
There was a problem hiding this comment.
should this be a method in resource.js instead?
There was a problem hiding this comment.
The point in this method is to allow an element to be measured regardless of whether it is a Resource instance associated with it or not, so it would probably be quite messy if this where in resource.js as it'd have to be a static method that then got a reference to Resources to make use of the vsync and viewport instances (which would also require making this.vsync_ public).
There was a problem hiding this comment.
I'm ok with this. But /cc @jridgewell - this is a good case for the kind of data that layers system should be able to produce in the future.
There was a problem hiding this comment.
Yup, Layers will be able to take any element and give you its bounds (it'll almost be this exact method).
There was a problem hiding this comment.
The only nuance will be (actually is) - the distance between two elements can only be really calculated when they are in the same layer - otherwise it'd be mostly meaningless. We could consider some form of multi-dimensional vector, but that's probably overkill.
Implements a more comprehensive strategy for amp-auto-ads. Strategy tries to ensure that there are 3 ads on the page and that no ads are within 500px of one another. The 3 ads and 500px are hard coded for now, but in a future change these will be configurable based on the configuration served by the ad network.
#6196