Enables 'msz' and 'psz' parameters on doubleclick ad requests. #21159
Conversation
glevitzky
changed the title
ads/google/a4a/test/test-utils.js
Outdated
| return element; | ||
| } | ||
|
|
||
| it('should return the fixed the width for FIXED layout', () => { |
ads/google/a4a/utils.js
Outdated
| export function getContainerWidth(win, element) { | ||
| let el = element; | ||
| let maxDepth = 100; | ||
| // Find the first ancestor with a fixed size |
There was a problem hiding this comment.
nit: period at end of sentence
| case Layout.FLUID: | ||
| // The above layouts determine the width of the element by the | ||
| // containing element, or by CSS max-width property. | ||
| const maxWidth = parseInt(computedStyle(win, el).maxWidth, 10); |
There was a problem hiding this comment.
Is calling computedStyle ok here?
There was a problem hiding this comment.
There's no strict rule against it, and I don't think this loop will iterate more than twice in practice.
There was a problem hiding this comment.
Talked to @glevitzky offline. We already have code calculating computedStyle from every nested parent here. It would be great if we can leverage the logic there later. But I'm fine with us calling computedStyle here if the getContainerWidth is only called once.
There was a problem hiding this comment.
Technically, we call the method twice, but the second call checks at most one element (different from any of the elements checked in the first call).
| // The above layouts determine the width of the element by the | ||
| // containing element, or by CSS max-width property. | ||
| const maxWidth = parseInt(computedStyle(win, el).maxWidth, 10); | ||
| if (maxWidth || maxWidth == 0) { |
There was a problem hiding this comment.
===? Or is the crazy behavior of == 0 needed?
There was a problem hiding this comment.
If !maxWidth, then maxWidth is NaN, and NaN != 0.
| case Layout.FLEX_ITEM: | ||
| return 0; | ||
| default: | ||
| // If no layout is provided, we must use getComputedStyle. |
There was a problem hiding this comment.
Is this valid AMP? If not, can we complain somehow?
There was a problem hiding this comment.
I think this is valid. I'm thinking of a case of an <amp-ad> being wrapped in a simple <div>, or by <body>.
| /** @const @enum{string} */ | ||
| const FLEXIBLE_AD_SLOTS_BRANCHES = { | ||
| EXPERIMENT: '21063174', | ||
| CONTROL: '21063173', |
There was a problem hiding this comment.
nit: don't we normally list control first?
| `${getContainerWidth(this.win, this.element)}x-1` : null, | ||
| 'psz': this.sendFlexibleAdSlotParams_ ? | ||
| `${getContainerWidth(this.win, this.element.parentElement)}x-1` : | ||
| null, |
There was a problem hiding this comment.
comment here that we're intentionally sending ${width}x-1 because height can't change?
| return impl.getAdUrl().then(url => { | ||
| expect(url).to.not.match(/(\?|&)msz=[0-9]+x-1(&|$)/); | ||
| expect(url).to.not.match(/(\?|&)psz=[0-9]+x-1(&|$)/); | ||
| expect(url).to.not.match(/(=|%2C)2106317(3|4)(%2C|&|$)/); |
There was a problem hiding this comment.
Here and above, can we assert "no msz and psz params" instead of "no valid params"?
| return impl.getAdUrl().then(url => { | ||
| expect(url).to.match(/(\?|&)msz=[0-9]+x-1(&|$)/); | ||
| expect(url).to.match(/(\?|&)psz=[0-9]+x-1(&|$)/); | ||
| expect(url).to.match(/(=|%2C)21063174(%2C|&|$)/); |
There was a problem hiding this comment.
Is there really no utility function for pulling out a url param?
[optional] would you be up for writing one?
There was a problem hiding this comment.
It would only save us so much, since we'd still have to verify the pulled out param. I can look into it in a follow up, though.
…oject#21159) * Initial * stash * Remove unrelated test page. * Move function to utils and some minor clenaup. * Added tests. * Fixes & test updates. * Fix * Experiment logic. * Test regexp fix, lint fix, dep-check fix, and check-type fix. * Remove stray comment. * Minor tweak + testing non-presence of eid. * Use assertElement over cast. * Fix test desc + minor test regexp fix. * Use url encoded commas in regexp. * PR feedback. * Refactor so that getContainerWidth is executed at most once per request.
…oject#21159) * Initial * stash * Remove unrelated test page. * Move function to utils and some minor clenaup. * Added tests. * Fixes & test updates. * Fix * Experiment logic. * Test regexp fix, lint fix, dep-check fix, and check-type fix. * Remove stray comment. * Minor tweak + testing non-presence of eid. * Use assertElement over cast. * Fix test desc + minor test regexp fix. * Use url encoded commas in regexp. * PR feedback. * Refactor so that getContainerWidth is executed at most once per request.
Enables 'msz' and 'psz' parameters on doubleclick ad requests.
These parameters, respectively, for a given element, represent the element's size, and the element's parent container's size. These parameters are used in determining flexible ad slot size options. Height is always -1 as height expansion is forbidden in AMP.