diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js index 573aee710cc64..36d7f83addc04 100644 --- a/build-system/tasks/presubmit-checks.js +++ b/build-system/tasks/presubmit-checks.js @@ -482,15 +482,17 @@ const forbiddenTerms = { // https://amp.dev/documentation/guides-and-tutorials/learn/a4a_spec 'src/services.js', 'src/service/cid-impl.js', - 'extensions/amp-user-notification/0.1/amp-user-notification.js', + 'extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js', 'extensions/amp-app-banner/0.1/amp-app-banner.js', 'extensions/amp-consent/0.1/consent-state-manager.js', + 'extensions/amp-user-notification/0.1/amp-user-notification.js', ], }, 'localStorage': { message: requiresReviewPrivacy, whitelist: [ 'extensions/amp-access/0.1/amp-access-iframe.js', + 'extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js', 'extensions/amp-script/0.1/amp-script.js', 'extensions/amp-web-push/0.1/amp-web-push-helper-frame.js', 'extensions/amp-web-push/0.1/amp-web-push-permission-dialog.js', diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js index 3316a27bff6cc..a423f2d92dcbc 100644 --- a/extensions/amp-a4a/0.1/amp-a4a.js +++ b/extensions/amp-a4a/0.1/amp-a4a.js @@ -372,7 +372,9 @@ export class AmpA4A extends AMP.BaseElement { return this.isRelayoutNeededFlag; } - /** @override */ + /** @override + @return {!Promise|undefined} + */ buildCallback() { this.creativeSize_ = { width: this.element.getAttribute('width'), diff --git a/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js b/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js index 6bd8ac07a0d1b..3e92105ba5052 100644 --- a/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js +++ b/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js @@ -76,6 +76,15 @@ const ADSENSE_BASE_URL = 'https://googleads.g.doubleclick.net/pagead/ads'; /** @const {string} */ const TAG = 'amp-ad-network-adsense-impl'; +/** @const {!{branch: string, control: string, experiment: string}} + @visibleForTesting +*/ +export const AD_SIZE_OPTIMIZATION_EXP = { + branch: 'adsense-ad-size-optimization', + control: '368226510', + experiment: '368226511', +}; + /** * Shared state for AdSense ad slots. This is used primarily for ad request url * parameters that depend on previous slots. @@ -161,6 +170,12 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A { * @private {boolean} */ this.shouldSandbox_ = false; + + /** + * Testing callback invoked when auto ad size settings are persisted in + * localstorage. + * @private {function()} */ + this.onAutoAdSizeSettingsStored_ = () => {}; } /** @@ -238,7 +253,9 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A { return getAmpAdRenderOutsideViewport(this.element) || 3; } - /** @override */ + /** @override + @return {!Promise|undefined}. + */ buildCallback() { super.buildCallback(); this.identityTokenPromise_ = Services.viewerForDoc(this.getAmpDoc()) @@ -246,26 +263,148 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A { .then(() => getIdentityToken(this.win, this.getAmpDoc(), super.getConsentPolicy()) ); - this.autoFormat_ = this.element.getAttribute('data-auto-format') || ''; - if (this.isResponsive_()) { - // Attempt to resize to the correct height. The width should already be - // 100vw, but is fixed here so that future resizes of the viewport don't - // affect it. - const viewportSize = this.getViewport().getSize(); - return this.attemptChangeSize( - AmpAdNetworkAdsenseImpl.getResponsiveHeightForContext_( - this.autoFormat_, - viewportSize, - this.element - ), - viewportSize.width - ).catch(() => {}); + return this.maybeUpgradeToResponsive().then(() => { + this.autoFormat_ = this.element.getAttribute('data-auto-format') || ''; + + if (this.isResponsive_()) { + // Attempt to resize to the correct height. The width should already be + // 100vw, but is fixed here so that future resizes of the viewport don't + // affect it. + const viewportSize = this.getViewport().getSize(); + return this.attemptChangeSize( + AmpAdNetworkAdsenseImpl.getResponsiveHeightForContext_( + this.autoFormat_, + viewportSize, + this.element + ), + viewportSize.width + ).catch(() => {}); + } + + // This should happen last, as some diversion criteria rely on some of the + // preceding logic (specifically responsive logic). + this.divertExperiments(); + this.maybeAddSinglePassExperiment(); + }); + } + + /** + * Upgrades the ad unit to responsive if there is an opt-in setting in localstorage. + * See https://github.com/ampproject/amphtml/issues/23568 for design. + * @return {!Promise} a promise that resolves when any upgrade is complete. + */ + maybeUpgradeToResponsive() { + if (!this.isInAdSizeOptimizationExperimentBranch()) { + return Promise.resolve(); + } + // If the ad unit is already responsive, there's nothing to do. + // Note that we can't use this.autoFormat_ here because it's not + // populated yet. + if (this.element.hasAttribute('data-auto-format')) { + return Promise.resolve(); } - // This should happen last, as some diversion criteria rely on some of the - // preceding logic (specifically responsive logic). - this.divertExperiments(); - this.maybeAddSinglePassExperiment(); + return ( + Services.storageForDoc(this.element) + .then(storage => storage.get(this.getAdSizeOptimizationStorageKey())) + .then(isAdSizeOptimizationEnabled => { + if (isAdSizeOptimizationEnabled) { + this.upgradeToResponsive(); + } + }) + // Do nothing if we fail to read localstorage. + .catch(() => { + dev().warn(TAG, 'Failed to look up publisher ad size settings.'); + }) + ); + } + + /** + * Actually upgrades the ad unit to responsive. + */ + upgradeToResponsive() { + this.element.setAttribute('height', ADSENSE_RSPV_WHITELISTED_HEIGHT); + this.element.setAttribute('width', '100vw'); + this.element.setAttribute('data-full-width', ''); + this.element.setAttribute('data-auto-format', 'rspv'); + + devAssert(this.isValidElement(), 'Upgraded element is not valid.'); + } + + /** + * Sets up a listener for a pingback of publisher settings in the ad response and + * writing such settings to localstorage. + * + * Note that we can have multiple listeners on the same page, which is okay because + * the settings for publishers should not change between different slots. + * + * See https://github.com/ampproject/amphtml/issues/23568 for design. + */ + attachSettingsListener() { + const listener = event => { + if (event['source'] != this.iframe.contentWindow) { + return; + } + const data = getData(event); + // data will look like this: + // { + // 'googMsgType': 'adsense-settings', + // 'adClient': 'ca-pub-123', + // 'enableAutoAdSize': '1' + // } + if (!!data && data['googMsgType'] != 'adsense-settings') { + return; + } + if (data['adClient'] != this.getAdClientId()) { + return; + } + const autoAdSizeStatus = data['enableAutoAdSize'] == '1'; + this.win.removeEventListener('message', listener); + Services.storageForDoc(this.element) + .then(storage => + storage + .set(this.getAdSizeOptimizationStorageKey(), autoAdSizeStatus) + .then(() => { + dev().info( + TAG, + `Saved publisher auto ad size setting: ${autoAdSizeStatus}` + ); + this.onAutoAdSizeSettingsStored_(); + }) + ) + // Do nothing if we fail to write to localstorage. + .catch(() => { + dev().warn(TAG, 'Failed to persist publisher auto ad size setting.'); + }); + }; + this.win.addEventListener('message', listener); + } + + /** + * Selects into the ad size optimization experiment. + * Note that this needs to be done before responsive sizing, so it must + * be separate from divertExperiments below. + * @return {boolean} + */ + isInAdSizeOptimizationExperimentBranch() { + const experimentInfoMap = /** @type {!Object} */ ({ + [[AD_SIZE_OPTIMIZATION_EXP.branch]]: { + isTrafficEligible: () => this.isResponsive_(), + branches: [ + [AD_SIZE_OPTIMIZATION_EXP.control], + [AD_SIZE_OPTIMIZATION_EXP.experiment], + ], + }, + }); + const setExps = randomlySelectUnsetExperiments(this.win, experimentInfoMap); + Object.keys(setExps).forEach(expName => + addExperimentIdToElement(setExps[expName], this.element) + ); + return ( + getExperimentBranch(this.win, AD_SIZE_OPTIMIZATION_EXP.branch) == + AD_SIZE_OPTIMIZATION_EXP.experiment + ); } /** @override */ @@ -307,6 +446,26 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A { ); } + /** + * @return {string} ad client ID for the current ad unit. + */ + getAdClientId() { + const adClientId = ( + this.element.getAttribute('data-ad-client') || '' + ).toLowerCase(); + if (!/^ca-/i.test(adClientId)) { + return `ca-${adClientId}`; + } + return adClientId; + } + + /** + * @return {string} ad size optimization storage key. + */ + getAdSizeOptimizationStorageKey() { + return `aas-${this.getAdClientId()}`; + } + /** @override */ getAdUrl(consentState) { if ( @@ -320,12 +479,7 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A { // validateData, from 3p/3p/js, after moving it someplace common. const startTime = Date.now(); const global = this.win; - let adClientId = this.element.getAttribute('data-ad-client'); - // Ensure client id format: lower case with 'ca-' prefix. - adClientId = adClientId.toLowerCase(); - if (adClientId.substring(0, 3) != 'ca-') { - adClientId = 'ca-' + adClientId; - } + const adClientId = this.getAdClientId(); const adTestOn = this.element.getAttribute('data-adtest') || isInManualExperiment(this.element); @@ -511,6 +665,9 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A { /** @override */ onCreativeRender(creativeMetaData) { super.onCreativeRender(creativeMetaData); + if (this.isInAdSizeOptimizationExperimentBranch()) { + this.attachSettingsListener(); + } this.isAmpCreative_ = !!creativeMetaData; if ( creativeMetaData && diff --git a/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js b/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js index 6af159d081fba..9e0cf68c30503 100644 --- a/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js +++ b/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js @@ -19,12 +19,13 @@ // always available for them. However, when we test an impl in isolation, // AmpAd is not loaded already, so we need to load it separately. import '../../../amp-ad/0.1/amp-ad'; -import {AmpA4A} from '../../../amp-a4a/0.1/amp-a4a'; -import {AmpAd} from '../../../amp-ad/0.1/amp-ad'; import { + AD_SIZE_OPTIMIZATION_EXP, AmpAdNetworkAdsenseImpl, resetSharedState, -} from '../amp-ad-network-adsense-impl'; // eslint-disable-line no-unused-vars +} from '../amp-ad-network-adsense-impl'; +import {AmpA4A} from '../../../amp-a4a/0.1/amp-a4a'; +import {AmpAd} from '../../../amp-ad/0.1/amp-ad'; // eslint-disable-line no-unused-vars import { AmpAdXOriginIframeHandler, // eslint-disable-line no-unused-vars } from '../../../amp-ad/0.1/amp-ad-xorigin-iframe-handler'; @@ -62,6 +63,7 @@ describes.realWin( let impl; let element; let isResponsiveStub; + let storageMock; beforeEach(() => { win = env.win; @@ -90,6 +92,18 @@ describes.realWin( impl = new AmpAdNetworkAdsenseImpl(element); impl.win['goog_identity_prom'] = Promise.resolve({}); isResponsiveStub = sandbox.stub(impl, 'isResponsive_'); + // Make tests faster by completing timeouts instantly. + sandbox.stub(Services, 'timerFor').returns({ + timeoutPromise: (unused, promise) => { + if (promise) { + return promise; + } + return Promise.reject(new Error('No token')); + }, + }); + return Services.storageForDoc(element).then(storage => { + storageMock = sandbox.mock(storage); + }); }); /** @@ -422,6 +436,49 @@ describes.realWin( ); expect(impl.iframe.id).to.equal('google_ads_iframe_3'); }); + + it('should write auto ad size data to localstorage', function*() { + storageMock + .expects('set') + .withExactArgs('aas-ca-adsense', true) + .returns(Promise.resolve()) + .once(); + + forceExperimentBranch( + impl.win, + AD_SIZE_OPTIMIZATION_EXP.branch, + AD_SIZE_OPTIMIZATION_EXP.experiment + ); + impl.iframe = { + contentWindow: window, + nodeType: 1, + style: {}, + }; + impl.element.setAttribute('data-ad-client', 'ca-adsense'); + + impl.size_ = {width: 123, height: 456}; + + impl.onCreativeRender(); + + const data = { + 'googMsgType': 'adsense-settings', + 'adClient': 'ca-adsense', + 'enableAutoAdSize': '1', + }; + let promiseResolver; + const savePromise = new Promise(resolve => { + promiseResolver = resolve; + }); + impl.onAutoAdSizeSettingsStored_ = () => { + promiseResolver(); + }; + + win.postMessage(data, '*'); + + yield savePromise; + + storageMock.verify(); + }); }); describe('centering', () => { @@ -878,6 +935,7 @@ describes.realWin( function constructImpl(config) { config.type = 'adsense'; + config['data-ad-client'] = 'ca-adsense'; element = createElementWithAttributes(doc, 'amp-ad', config); iframe = env.win.document.createElement('iframe'); element.appendChild(iframe); @@ -895,12 +953,20 @@ describes.realWin( return impl; } - it('should do nothing for non-responsive', () => { + it('should do nothing for non-responsive', function*() { const adsense = constructImpl({ width: '320', height: '150', }); - expect(adsense.buildCallback()).to.be.undefined; + env.sandbox + .stub(adsense, 'attemptChangeSize') + .returns(Promise.resolve()); + + const promise = adsense.buildCallback(); + expect(promise).to.exist; + yield promise; + + expect(adsense.attemptChangeSize).to.not.be.called; }); it('should schedule a resize for responsive', function*() { @@ -953,6 +1019,117 @@ describes.realWin( VIEWPORT_WIDTH ); }); + + describe('for publisher opted in to auto ad size optimization', () => { + beforeEach(() => { + storageMock + .expects('get') + .withExactArgs('aas-ca-adsense') + .returns(Promise.resolve(true)); + }); + + it('does nothing if experiment is disabled', function*() { + forceExperimentBranch( + impl.win, + AD_SIZE_OPTIMIZATION_EXP.branch, + AD_SIZE_OPTIMIZATION_EXP.control + ); + const adsense = constructImpl({ + width: '320', + height: '150', + }); + env.sandbox + .stub(adsense, 'attemptChangeSize') + .returns(Promise.resolve()); + + const promise = adsense.buildCallback(); + expect(promise).to.exist; + yield promise; + + expect(adsense.attemptChangeSize).to.not.be.called; + expect(adsense.element.hasAttribute('data-auto-format')).to.be.false; + }); + + it('does nothing if ad unit is responsive already', function*() { + forceExperimentBranch( + impl.win, + AD_SIZE_OPTIMIZATION_EXP.branch, + AD_SIZE_OPTIMIZATION_EXP.experiment + ); + const adsense = constructImpl({ + width: '100vw', + height: '100', + 'data-auto-format': 'mcrspv', + }); + env.sandbox + .stub(adsense, 'attemptChangeSize') + .returns(Promise.resolve()); + const promise = adsense.buildCallback(); + expect(promise).to.exist; + yield promise; + expect(adsense.attemptChangeSize).to.be.calledWith( + 1386, + VIEWPORT_WIDTH + ); + }); + + it('upgrades manual ad units to responsive if experiment is enabled', function*() { + forceExperimentBranch( + impl.win, + AD_SIZE_OPTIMIZATION_EXP.branch, + AD_SIZE_OPTIMIZATION_EXP.experiment + ); + const adsense = constructImpl({ + width: '320', + height: '150', + }); + env.sandbox + .stub(adsense, 'attemptChangeSize') + .returns(Promise.resolve()); + + const promise = adsense.buildCallback(); + expect(promise).to.exist; + yield promise; + + expect(adsense.element.getAttribute('data-auto-format')).to.be.equal( + 'rspv' + ); + expect(adsense.attemptChangeSize).to.be.calledWith( + 300, + VIEWPORT_WIDTH + ); + }); + }); + describe('for publisher not opted in to auto ad size optimization', () => { + beforeEach(() => { + storageMock + .expects('get') + .withExactArgs('aas-ca-adsense') + .returns(Promise.resolve(false)); + }); + + it('does not upgrade manual ad units to responsive if experiment is enabled', function*() { + forceExperimentBranch( + impl.win, + AD_SIZE_OPTIMIZATION_EXP.branch, + AD_SIZE_OPTIMIZATION_EXP.experiment + ); + const adsense = constructImpl({ + width: '320', + height: '150', + }); + env.sandbox + .stub(adsense, 'attemptChangeSize') + .returns(Promise.resolve()); + + const promise = adsense.buildCallback(); + expect(promise).to.exist; + yield promise; + + expect(adsense.attemptChangeSize).to.not.be.called; + expect(adsense.element.hasAttribute('data-auto-format')).to.be.false; + }); + }); }); describe('#onLayoutMeasure', () => { @@ -1288,5 +1465,24 @@ describes.realWin( expect(letCreativeTriggerRenderStart).to.equal(false); }); }); + + describe('#getAdClientId', () => { + it('should be same as data-ad-client if it has a ca- prefix', () => { + element.setAttribute('data-ad-client', 'ca-pub-123'); + expect(impl.getAdClientId()).to.equal('ca-pub-123'); + }); + it("should have a ca- prefix if data-ad-client doesn't", () => { + element.setAttribute('data-ad-client', 'pub-456'); + expect(impl.getAdClientId()).to.equal('ca-pub-456'); + }); + }); + describe('#getAdSizeOptimizationStorageKey', () => { + it('should be the data-ad-client attribute prefixed by aas-', () => { + element.setAttribute('data-ad-client', 'ca-pub-123'); + expect(impl.getAdSizeOptimizationStorageKey()).to.equal( + 'aas-ca-pub-123' + ); + }); + }); } ); diff --git a/spec/amp-localstorage.md b/spec/amp-localstorage.md index 32f3dbeac9592..281a9f613325a 100644 --- a/spec/amp-localstorage.md +++ b/spec/amp-localstorage.md @@ -54,6 +54,7 @@ The following AMP components and service are using the localStorage. ** Please add all future usage to the following list +- `` : Store publisher ad size opt in status. - `` : Store the user decision on dismiss banner - `` : Store the user decision on dismiss notification - `` : Store the user decision and granular information on consent