Skip to content

Commit

Permalink
Enables 'msz' and 'psz' parameters on doubleclick ad requests. (amppr…
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
glevitzky authored and Noran Azmy committed Mar 22, 2019
1 parent 99e090c commit 6ad1cb5
Show file tree
Hide file tree
Showing 6 changed files with 249 additions and 4 deletions.
128 changes: 128 additions & 0 deletions ads/google/a4a/test/test-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
extractAmpAnalyticsConfig,
extractHost,
getAmpRuntimeTypeParameter,
getContainerWidth,
getCorrelator,
getCsiAmpAnalyticsVariables,
getEnclosingContainerTypes,
Expand Down Expand Up @@ -988,3 +989,130 @@ describes.realWin('#groupAmpAdsByType', {amp: true}, env => {
});
});
});

describes.realWin('#getContainerWidth', {amp: true}, env => {
let doc, win;
beforeEach(() => {
win = env.win;
doc = win.document;
});

function createResource(
config, layout, tagName = 'amp-ad', parent = doc.body) {
config['layout'] = layout;
const element = createElementWithAttributes(doc, tagName, config);
parent.appendChild(element);
return element;
}

it('should return the fixed width for FIXED layout', () => {
const element = createResource({width: 300, height: 250}, 'fixed');
expect(getContainerWidth(win, element)).to.equal(300);
});

it('should return 0 for FIXED layout and invalid width', () => {
allowConsoleError(() => {
const element = createResource({width: 'auto', height: 250}, 'fixed');
expect(getContainerWidth(win, element)).to.equal(0);
});
});

it('should return 0 for NODISPLAY layout', () => {
const element = createResource({width: 500}, 'nodisplay');
expect(getContainerWidth(win, element)).to.equal(0);
});

it('should return 0 for FLEX_ITEM layout', () => {
const element = createResource({width: 500}, 'flex-item');
expect(getContainerWidth(win, element)).to.equal(0);
});

it('should return 0 for invalid layout', () => {
allowConsoleError(() => {
const element = createResource({width: 500}, 'qwerty');
expect(getContainerWidth(win, element)).to.equal(0);
});
});

it('should return the max-width, if present, for FILL layout', () => {
const element = createResource({maxWidth: 300}, 'fill');
expect(getContainerWidth(win, element)).to.equal(300);
});

it('should return parent\'s fixed width for FILL layout', () => {
const parent = document.createElement('div');
parent.setAttribute('width', 300);
parent.setAttribute('layout', 'fixed');
doc.body.appendChild(parent);
const element = createResource({} /* config */, 'fill', 'amp-ad', parent);
expect(getContainerWidth(win, element)).to.equal(300);
});

it('should return the max-width, if present, for FIXED_HEIGHT layout',
() => {
const element = createResource({height: 300}, 'fixed-height');
element.style.maxWidth = '250px';
expect(getContainerWidth(win, element)).to.equal(250);
});

it('should return parent\'s fixed width for FIXED_HEIGHT layout', () => {
const parent = document.createElement('div');
parent.setAttribute('width', 300);
parent.setAttribute('layout', 'fixed');
doc.body.appendChild(parent);
const element = createResource(
{height: 250}, 'fixed-height', 'amp-ad', parent);
expect(getContainerWidth(win, element)).to.equal(300);
});

it('should return the max-width, if present, for FLUID layout', () => {
const element = createResource({height: 300}, 'fluid');
element.style.maxWidth = '250px';
expect(getContainerWidth(win, element)).to.equal(250);
});

it('should return parent\'s fixed width for FLUID layout', () => {
const parent = document.createElement('div');
parent.setAttribute('width', 300);
parent.setAttribute('layout', 'fixed');
doc.body.appendChild(parent);
const element = createResource(
{height: 250}, 'fluid', 'amp-ad', parent);
expect(getContainerWidth(win, element)).to.equal(300);
});

it('should return the max-width, if present, for RESPONSIVE layout', () => {
const element = createResource({height: 200, width: 200}, 'responsive');
element.style.maxWidth = '250px';
expect(getContainerWidth(win, element)).to.equal(250);
});

it('should return parent\'s fixed width for RESPONSIVE layout', () => {
const parent = document.createElement('div');
parent.setAttribute('width', 300);
parent.setAttribute('layout', 'fixed');
doc.body.appendChild(parent);
const element = createResource(
{height: 250, width: 250}, 'responsive', 'amp-ad', parent);
expect(getContainerWidth(win, element)).to.equal(300);
});

it('should return the viewport width for CONTAINER layout', () => {
const element = createResource({} /* config */, 'container');
sandbox.stub(Services.viewportForDoc(element), 'getSize')
.returns({width: 300});
expect(getContainerWidth(win, element)).to.equal(300);
});

it('should return -1 width for non-fixed layouts, maxDepth = 1', () => {
['fill', 'fixed-height', 'fluid', 'responsive'].forEach(layout => {
const parent = document.createElement('div');
parent.setAttribute('width', 300);
parent.setAttribute('layout', 'fixed');
doc.body.appendChild(parent);
const element = createResource(
{height: 250}, layout, 'amp-ad', parent);
expect(getContainerWidth(win, element, 1)).to.equal(-1);
});
});
});
50 changes: 50 additions & 0 deletions ads/google/a4a/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@

import {CONSENT_POLICY_STATE} from '../../../src/consent-state';
import {DomFingerprint} from '../../../src/utils/dom-fingerprint';
import {Layout} from '../../../src/layout';
import {Services} from '../../../src/services';
import {buildUrl} from './shared/url-builder';
import {computedStyle} from '../../../src/style';
import {dev, devAssert} from '../../../src/log';
import {dict} from '../../../src/utils/object';
import {
Expand Down Expand Up @@ -960,3 +962,51 @@ export function getAmpRuntimeTypeParameter(win) {
const art = getBinaryTypeNumericalCode(getBinaryType(win));
return isCdnProxy(win) && art != '0' ? art : null;
}

/**
* Returns the fixed size of the given element, or the fixed size of its nearest
* ancestor that has a fixed size, if the given element has none.
* @param {!Window} win
* @param {?Element} element
* @param {number=} maxDepth The maximum number of ancestors to check.
* @return {number} The width of the given element, or of the nearest ancestor
* with a fixed size, if the given element has none.
*/
export function getContainerWidth(win, element, maxDepth = 100) {
let el = element;
let depth = maxDepth;
// Find the first ancestor with a fixed size.
while (el && depth--) {
const layout = el.getAttribute('layout');
switch (layout) {
case Layout.FIXED:
return parseInt(el.getAttribute('width'), 10) || 0;
case Layout.RESPONSIVE:
case Layout.FILL:
case Layout.FIXED_HEIGHT:
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);
if (maxWidth || maxWidth == 0) {
return maxWidth;
}
el = el.parentElement;
break;
case Layout.CONTAINER:
// Container layout allows the container's size to be determined by
// the children within it, so in principle we can grow as large as the
// viewport.
const viewport = Services.viewportForDoc(dev().assertElement(element));
return viewport.getSize().width;
case Layout.NODISPLAY:
case Layout.FLEX_ITEM:
return 0;
default:
// If no layout is provided, we must use getComputedStyle.
return parseInt(computedStyle(win, el).width, 10) || 0;
}
}
return -1;
}

1 change: 1 addition & 0 deletions build-system/dep-check-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ exports.rules = [
'ads/google/a4a/**->src/experiments.js',
'ads/google/a4a/**->src/services.js',
'ads/google/a4a/utils.js->src/service/variable-source.js',
'ads/google/a4a/utils.js->src/layout.js',
// alp handler needs to depend on src files
'ads/alp/handler.js->src/dom.js',
'ads/alp/handler.js->src/config.js',
Expand Down
6 changes: 3 additions & 3 deletions examples/fluid.amp.html
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@
<h1>AMP Static Image DFP Reservation.</h1>
<p>Tests a static image ad in an AMP page.</p>
<div class="artificialfold"></div>
<div style="margin-left: 35%; margin-right: 15%; border: 1px red solid">
<amp-ad type=doubleclick data-slot="/30497360/native_v2/iu0/iu1/iu2" height="fluid"></amp-ad>
<div id="narrow" style="margin-left: 35%; margin-right: 15%; border: 1px red solid">
<amp-ad type=doubleclick data-slot="/30497360/native_v2/iu0/iu1/iu2" height="fluid" layout="fluid"></amp-ad>
</div>
<amp-ad type=doubleclick data-slot="/30497360/native_v2/iu7" height="fluid"></amp-ad>
<amp-ad type=doubleclick data-slot="/30497360/native_v2/iu7" height="fluid" layout="fluid"></amp-ad>
</body>
</html>
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
ValidAdContainerTypes,
addCsiSignalsToAmpAnalyticsConfig,
extractAmpAnalyticsConfig,
getContainerWidth,
getCsiAmpAnalyticsConfig,
getCsiAmpAnalyticsVariables,
getEnclosingContainerTypes,
Expand Down Expand Up @@ -119,6 +120,15 @@ const DOUBLECLICK_SRA_EXP_BRANCHES = {
SRA_NO_RECOVER: '21062235',
};

/** @const {string} */
const FLEXIBLE_AD_SLOTS_EXP = 'flexAdSlots';

/** @const @enum{string} */
const FLEXIBLE_AD_SLOTS_BRANCHES = {
CONTROL: '21063173',
EXPERIMENT: '21063174',
};

/**
* Map of pageview tokens to the instances they belong to.
* @private {!Object<string, !AmpAdNetworkDoubleclickImpl>}
Expand Down Expand Up @@ -267,6 +277,9 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
* @private {boolean}
*/
this.shouldSandbox_ = false;

/** @private {boolean} */
this.sendFlexibleAdSlotParams_ = false;
}

/**
Expand Down Expand Up @@ -363,10 +376,19 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
branches: Object.keys(DOUBLECLICK_SRA_EXP_BRANCHES).map(
key => DOUBLECLICK_SRA_EXP_BRANCHES[key]),
},
[FLEXIBLE_AD_SLOTS_EXP]: {
isTrafficEligible: () => true,
branches: Object.values(FLEXIBLE_AD_SLOTS_BRANCHES),
},
});
const setExps = this.randomlySelectUnsetExperiments_(experimentInfoMap);
Object.keys(setExps).forEach(expName =>
setExps[expName] && this.experimentIds.push(setExps[expName]));
if (setExps[FLEXIBLE_AD_SLOTS_EXP] &&
setExps[FLEXIBLE_AD_SLOTS_EXP] ==
FLEXIBLE_AD_SLOTS_BRANCHES.EXPERIMENT) {
this.sendFlexibleAdSlotParams_ = true;
}
}

/**
Expand Down Expand Up @@ -472,6 +494,17 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
this.win['ampAdGoogleIfiCounter']++;
const pageLayoutBox = this.isSinglePageStoryAd ?
this.element.getPageLayoutBox() : null;
let psz = null;
let msz = null;
if (this.sendFlexibleAdSlotParams_) {
const parentWidth = getContainerWidth(
this.win, this.element.parentElement);
let slotWidth = getContainerWidth(
this.win, this.element, 1 /* maxDepth */);
slotWidth = slotWidth == -1 ? parentWidth : slotWidth;
psz = `${parentWidth}x-1`;
msz = `${slotWidth}x-1`;
}
return Object.assign({
'iu': this.element.getAttribute('data-slot'),
'co': this.jsonTargeting &&
Expand All @@ -487,6 +520,10 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
'frc': Number(this.fromResumeCallback) || null,
'fluid': this.isFluidRequest_ ? 'height' : null,
'fsf': this.forceSafeframe ? '1' : null,
// Both msz/psz send a height of -1 because height expansion is
// disallowed in AMP.
'msz': msz,
'psz': psz,
'scp': serializeTargeting(
(this.jsonTargeting && this.jsonTargeting['targeting']) || null,
(this.jsonTargeting &&
Expand Down Expand Up @@ -1406,7 +1443,6 @@ AMP.extension(TAG, '0.1', AMP => {
AMP.registerElement(TAG, AmpAdNetworkDoubleclickImpl);
});


/** @visibleForTesting */
export function resetSraStateForTesting() {
sraRequests = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -810,6 +810,36 @@ describes.realWin('amp-ad-network-doubleclick-impl', realWinConfig, env => {
impl.getAdUrl(CONSENT_POLICY_STATE.UNKNOWN_NOT_REQUIRED).then(url => {
expect(url).to.not.match(/(\?|&)npa=(&|$)/);
}));

it('should include msz/psz if in experiment', () => {
sandbox.stub(impl, 'randomlySelectUnsetExperiments_').returns(
{flexAdSlots: '21063174'});
impl.setPageLevelExperiments();
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|&|$)/);
});
});

it('should not include msz/psz if not in flexAdSlots control', () => {
sandbox.stub(impl, 'randomlySelectUnsetExperiments_').returns(
{flexAdSlots: '21063173'});
impl.setPageLevelExperiments();
return impl.getAdUrl().then(url => {
expect(url).to.not.match(/(\?|&)msz=/);
expect(url).to.not.match(/(\?|&)psz=/);
expect(url).to.match(/(=|%2C)21063173(%2C|&|$)/);
});
});

it('should not include msz/psz if not in flexAdSlots experiment', () => {
return impl.getAdUrl().then(url => {
expect(url).to.not.match(/(\?|&)msz=/);
expect(url).to.not.match(/(\?|&)psz=/);
expect(url).to.not.match(/(=|%2C)2106317(3|4)(%2C|&|$)/);
});
});
});

describe('#getPageParameters', () => {
Expand Down

0 comments on commit 6ad1cb5

Please sign in to comment.