Add some A4A ad request parameters #6643
Conversation
| clientId, referrer) { | ||
| const slotId = a4a.element.getAttribute('data-amp-slot-index'); | ||
| const slotNumber = Number(slotId); |
There was a problem hiding this comment.
What happens if data-amp-slot-index isn't coercible to a number? I think you'll get NaN back? Is that okay? Also, what happens if it's something that does coerce to a number, but really shouldn't? (Like null or [''], both of which coerce to 0.)
There was a problem hiding this comment.
This set by code in amp-ad, to a number. So I would prefer to simply remove the coerce.
| {name: 'adf', value: domFingerprint(adElement)}, | ||
| {name: 'c', value: makeCorrelator(clientId, documentInfo.pageViewId)}, | ||
| {name: 'c', value: getCorrelator(global, clientId)}, |
There was a problem hiding this comment.
Aside: Most other AMP code seems to use win for the owner window. global looks like a convention inherited from inside Google? Any chance we could change this to win everywhere in here, just for greater consistency with AMP code?
| {name: 'output', value: 'html'}, | ||
| {name: 'nhd', value: iframeDepth}, | ||
| {name: 'eid', value: adElement.getAttribute('data-experiment-id')}, | ||
| {name: 'iu', value: a4a.element.getAttribute('data-ad-slot')}, |
There was a problem hiding this comment.
This is a required parameter, yes? Is there some user-level warning or error somewhere if it's omitted? I don't recall if we use it in the A4A network impls, but in the ads/google/doubleclick.js code, there's a call to 3p/3p.validateData that might be relevant.
There was a problem hiding this comment.
I'll add a TODO. Yes, validateData is the right thing to use. So it ought to be moved somewhere to be used by both 3p and A4A. And someone needs to figure out which are required. (I used to know....)
| * @param {!Window} win The window for which we read the browser dimensions. | ||
| * @return {?{width: number, height: number}} | ||
| */ | ||
| function browserViewportSize(win) { |
There was a problem hiding this comment.
Is this going to be different than you'd get from a4a.getViewport().getSize()?
There was a problem hiding this comment.
Used getSize(), so function is now gone.
| outerHeight, | ||
| innerWidth, | ||
| innerHeight].join(); | ||
| }; |
There was a problem hiding this comment.
There is a test-utils.js for this file. Seems like a lot of these methods should have tests there (and/or revise/extend the tests that already exist.)
There was a problem hiding this comment.
I added a test for this function, and a TODO for others that I am not touching in this PR.
| */ | ||
|
|
||
| import {AmpAdNetworkDoubleclickImpl} from '../amp-ad-network-doubleclick-impl'; | ||
| import {AmpAdUIHandler} from '../../../amp-ad/0.1/amp-ad-ui'; // eslint-disable-line no-unused-vars |
There was a problem hiding this comment.
Why do you need to disable this lint check here, and below?
There was a problem hiding this comment.
No longer needed. Removed.
| import {utf8Encode} from '../../../../src/utils/bytes'; | ||
| import * as sinon from 'sinon'; | ||
|
|
||
| describe('amp-ad-network-doubleclick-impl', () => { |
There was a problem hiding this comment.
New hotness! You can now make this:
describes.sandboxed('amp-ad-network-doubleclick-impl', {}, () => {
// Omit let sandbox and sinon.sandbox.create() and sandbox.restore().
// Rest of test stuff
});|
|
||
| beforeEach(() => { | ||
| sandbox = sinon.sandbox.create(); | ||
| doubleclickImplElem = document.createElement('amp-ad'); |
There was a problem hiding this comment.
If you're going to do document.createElement, worth doing it inside an isolated iframe, I think. Can also use src/dom#createElementWithAttributes as shorthand. Try nesting the following inside the outer describes.sandboxed:
describes.fakeWin('some descriptive name', {}, env => {
let win;
let doc;
beforeEach(() => {
win = env.win;
doc = win.document;
doubleclickImplElem = createElementWithAttributes(doc, 'amp-ad', {
type: 'doubleclick',
data-ad-client: 'adsense',
width: /* ??? */,
height: /* ??? */,
});
// Do rest of beforeEach initialization.
});
// Test cases and so on.
});| describe('#extractCreativeAndSignature', () => { | ||
| it('without signature', () => { | ||
| return utf8Encode('some creative').then(creative => { | ||
| return expect(doubleclickImpl.extractCreativeAndSignature( |
| '&top=https?%3A%2F%2Flocalhost%3A9876%2F%3Fid%3D[0-9]+' + | ||
| '(&loc=https?%3A%2F%2[a-zA-Z0-9.:%]+)?' + | ||
| '&ref=https?%3A%2F%2Flocalhost%3A9876%2F%3Fid%3D[0-9]+')); | ||
| }); |
| @@ -65,19 +65,18 @@ export function isGoogleAdsA4AValidEnvironment(win, element) { | |||
| * @param {!../../../extensions/amp-a4a/0.1/amp-a4a.AmpA4A} a4a | |||
| * @param {string} baseUrl | |||
| * @param {number} startTime | |||
| * @param {number} slotNumber | |||
| * @param {!Array<!./url-builder.QueryParameterDef>} queryParams | |||
| * @param {!Array<!./url-builder.QueryParameterDef>} unboundedQueryParams | |||
There was a problem hiding this comment.
Can we have some documentation on the difference between queryParams and unboundedQueryParams?
There was a problem hiding this comment.
I'm not changing this, and didn't write it it originally, but sure.
I added doc.
| * @param {!Array<!./url-builder.QueryParameterDef>} queryParams | ||
| * @param {!Array<!./url-builder.QueryParameterDef>} unboundedQueryParams | ||
| * @return {!Promise<string>} | ||
| */ | ||
| export function googleAdUrl( | ||
| a4a, baseUrl, startTime, slotNumber, queryParams, unboundedQueryParams) { | ||
| a4a, baseUrl, startTime, queryParams, unboundedQueryParams) { | ||
| /** @const {!Promise<string>} */ | ||
| const referrerPromise = viewerForDoc(a4a.getAmpDoc()).getReferrerUrl(); |
There was a problem hiding this comment.
Would it be possible/possible to recover from either of these promises failing? Does it occur in the wild? I'm trying to be much more explicit about possible error conditions and how we would handle/recover
There was a problem hiding this comment.
I'll add a TODO.
| @@ -106,16 +105,17 @@ export function extractGoogleAdCreativeAndSignature( | |||
| * @param {!../../../extensions/amp-a4a/0.1/amp-a4a.AmpA4A} a4a | |||
| * @param {string} baseUrl | |||
| * @param {number} startTime | |||
| * @param {number} slotNumber | |||
| * @param {!Array<!./url-builder.QueryParameterDef>} queryParams | |||
| * @param {!Array<!./url-builder.QueryParameterDef>} unboundedQueryParams | |||
| * @param {(string|undefined)} clientId | |||
| * @param {string} referrer | |||
| * @return {string} | |||
| */ | |||
| function buildAdUrl( | |||
There was a problem hiding this comment.
Is there any real value in googleAdUrl and buildAdUrl being separate functions? I believe buildAdUrl can be private
There was a problem hiding this comment.
Will merge googleAdUrl and buildAdUrl. Style guide says not to use private:
Exported functions may be defined directly on the exports object, or else declared locally and exported separately. Non-exported functions are encouraged and should not be declared @Private.
| * @param {!Window} win The window for which we read the browser dimensions. | ||
| * @return {?{width: number, height: number}} | ||
| */ | ||
| function browserViewportSize(win) { |
| /* | ||
| * Browser viewport size, if we are in an iframe. | ||
| * @param {!Window} win The window for which we read the browser dimensions. | ||
| * @return {?{width: number, height: number}} |
There was a problem hiding this comment.
Can this be explicit what is returned for various error conditions?
| * @param {!../../../src/service/viewport-impl.Viewport} viewport | ||
| * @return {string} | ||
| */ | ||
| function additionalDimensions(win, viewport) { |
There was a problem hiding this comment.
Style guide says no. (See above.)
| try { | ||
| screenX = win.screenX; | ||
| screenY = win.screenY; | ||
| } catch (e) {} |
There was a problem hiding this comment.
I'm curious how this or references below could ever throw?
There was a problem hiding this comment.
Don't know. This was copied from code elsewhere. If you think it can't throw, I'm happy to remove the try/catch.
Original comment:
// On some UAs (particularly UCWEB), these can throw NS_ERROR_FAILUREs.
| {name: 'ref', value: referrer}, | ||
| ] | ||
| ); | ||
| dtdParam.value = elapsedTimeWithCeiling(Date.now(), startTime); |
There was a problem hiding this comment.
Setting dtd here means that it does not include the time required to execute buildUrl. Instead, I suggest:
const url = buildUrl(baseUrl, allQueryParams, MAX_URL_LENGTH - 10, {name: 'trunc', value: '1'});
return url += '&dtd=' + elapsedTimeWithCeiling(Date.now(), startTime);
| const slotNumber = a4a.element.getAttribute('data-amp-slot-index'); | ||
| const win = a4a.win; | ||
| const documentInfo = documentInfoForDoc(a4a.element); | ||
| if (!win.gaGlobal) { |
There was a problem hiding this comment.
win.gaGlobal = win.gaGlobal || {vid: clientId, hid: documentInfo. pageViewId};
| const viewportRect = viewport.getRect(); | ||
| const iframeDepth = iframeNestingDepth(win); | ||
| const viewportSize = viewport.getSize(); | ||
| const adElement = a4a.element; |
There was a problem hiding this comment.
You reference a4a.element above in a4a.element.getAttribute('data-amp-slot-index'), might as well move this up and use it there
| let depth = 0; | ||
| while (win != win.parent) { | ||
| win = win.parent; | ||
| while (w != w.parent) { |
There was a problem hiding this comment.
Let's add a loop iteration count to ensure this will always exit
| function secondWindowFromTop(global) { | ||
| let secondFromTop = global; | ||
| function secondWindowFromTop(win) { | ||
| let secondFromTop = win; | ||
| while (secondFromTop.parent != secondFromTop.parent.parent) { |
| @@ -97,19 +96,18 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A { | |||
|
|
|||
| /** @override */ | |||
| getAdUrl() { | |||
| // TODO: Check for required and allowed parameters. Probably use | |||
| // validateData, from 3p/3p/js, after noving it someplace common. | |||
There was a problem hiding this comment.
nit: noving -> moving
| const slotRect = this.getIntersectionElementLayoutBox(); | ||
| const visibilityState = viewerForDoc(this.getAmpDoc()) | ||
| .getVisibilityState(); | ||
| const adTestOn = this.element.getAttribute('data-adtest') || | ||
| isInManualExperiment(this.element); | ||
| const format = `${slotRect.width}x${slotRect.height}`; | ||
| const slotId = this.element.getAttribute('data-amp-slot-index'); | ||
| const adk = this.adKey_(format); | ||
| this.uniqueSlotId_ = slotId + adk; |
There was a problem hiding this comment.
Let's add a comment here that data-amp-slot-index is populated within amp-ad (had to do some splunking to ensure it was always set) as well as a dev().assert(slotId != undefined);
| import {createIframePromise} from '../../../../testing/iframe'; | ||
| import {upgradeOrRegisterElement} from '../../../../src/custom-element'; | ||
|
|
||
| function createAdsenseImplElement(attributes, opt_doc, opt_tag) { | ||
| const doc = opt_doc || document; | ||
| const tag = opt_tag || 'amp-ad'; | ||
| const adsenseImplElem = doc.createElement(tag); | ||
| adsenseImplElem.setAttribute('type', 'adsense'); | ||
| const element = doc.createElement(tag); |
There was a problem hiding this comment.
Can use createElementWithAttributes in src/dom
Remodularize, to put common parameters in utils.js. Fix correlator. Add test for A4A AdSense getAdUrl. Add A4A DoubleClick unit test, including getAdUrl.
Split addAttributesToElement out from createElementWithAttributes.
Leave a TODO to fix the tests so they set up enough of the environment that the assertion will be true.
* master: (310 commits) Update csa.md to remove non-required parameters (ampproject#6902) Add notes about requesting ads ATF and link to demo (ampproject#7037) Remove whitelist for lightbox scrollable validator (ampproject#7034) Delegate submit events until amp-form is loaded (ampproject#6929) Moves closure sha384 into a new extension amp-crypto-polyfill for lazy load (ampproject#7006) Refactor observables in viewer-impl into a map object (ampproject#7004) resizing of margins (ampproject#6824) Use URL replacer from embed for pixel (ampproject#7029) adds support for Gemius analytics (ampproject#6558) Avoid duplicating server-layout (ampproject#7021) Laterpay validator config (ampproject#6974) Validator rollup (ampproject#7023) skeleton for amp-tabs (ampproject#7003) Upgrade post-css and related packages to latest (ampproject#7020) handle unload (ampproject#7001) viewer-integr.js -> amp-viewer-integration (ampproject#6989) dev().info()->dev().fine() (ampproject#7017) Turned on experiment flag (ampproject#6774) Unlaunch ios-embed-wrapper for iOS8 to avoid scroll freezing issues (ampproject#7018) Add some A4A ad request parameters (ampproject#6643) ...
Remodularize, to put common parameters in utils.js.
Fix correlator.
Add test for A4A AdSense getAdUrl.
Add A4A DoubleClick unit test, including getAdUrl.