✨ Implement ad size optimization for AdSense. #24375

riklund · 2019-09-05T16:42:48Z

riklund · 2019-09-05T16:43:49Z

@keithwrightbos @lannka @charliereams @zhouyx

keithwrightbos · 2019-09-05T16:58:23Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+    // Note: this must happen before this.autoFormat_ is populated
+    // since it may change the outcome.
+    if (this.isInAdSizeOptimizationExperimentBranch()) {
+      upgradePromise = this.maybeUpgradeToResponsive();


Can you move the isInAdSizeOptimizationExperimentBranch check into maybeUpgradeToResponsive saving having to create the upgradePromise variable? Given the function name implies it may do nothing, it seems like a cleaner divide:

'''return this.maybeUpgradeToResponsive().then(() => {...```

Sure, done.
I think I wanted the code to be obvious in terms of not changing behavior when the experiment wasn't active, but both ways work.

keithwrightbos · 2019-09-05T16:59:58Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+   * Actually upgrades the ad unit to responsive.
+   */
+  upgradeToResponsive() {
+    this.element.setAttribute('height', ADSENSE_RSPV_WHITELISTED_HEIGHT);


Might this cause reflow? Is it ok to change directly or do you need to use attemptSizeChange?

I'm a bit unsure. It seems at some point styles are set with css on the element which means these attributes don't actually matter for layout any more, but not entirely sure if that has happened yet in buildCallback?

We do need to change the values in the attributes, because otherwise the element validation (as it currently works) will fail (isValidElement will throw later on).

OK please add @lannka for review

AMP layout should have happened before this. But could you pls double confirm locally? (for example print logs that measures the height before / after this call.

Done! Added console.error at the top/bottom of method printing
this.win.getComputedStyle(this.element, null).height
and it's the same value for both calls.

keithwrightbos · 2019-09-05T17:00:29Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+    this.element.setAttribute('data-full-width', '');
+    this.element.setAttribute('data-auto-format', 'rspv');
+
+    devAssert(this.isValidElement(), 'Upgraded element is not valid.');


Why would this assert fail? What about the change could make it invalid?

For example if we didn't set height/width to the exact values we do above.

devAssert will be stripped out from production binaries. is that what you want?

Yes. This is intended to prevent accidentally breaking the feature during future development.

keithwrightbos · 2019-09-05T17:01:40Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+      if (data['adClient'] != this.getAdClientId()) {
+        return;
+      }
+      const autoAdSizeStatus = !!data['enableAutoAdSize'];


if you truly expect it to the boolean true then better to do: const autoAdSizeStatus = data['enableAutoAdSize'] === true;

Looking at some ad response, it seems it's actually '1' - updated.

keithwrightbos · 2019-09-05T17:02:25Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+      this.win.removeEventListener('message', listener);
+      Services.storageForDoc(this.element)
+        .then(storage => {
+          return storage


Nit: you don't nee the braces and return: then(storage => storage.set...

keithwrightbos · 2019-09-05T17:02:47Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+        .then(storage => {
+          return storage
+            .set(this.getAdSizeOptimizationStorageKey(), autoAdSizeStatus)
+            .then(unused => {


Instead of unused just pass empty parens: then(() => {

keithwrightbos · 2019-09-05T17:07:16Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

@@ -629,6 +785,9 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A {

  /** @override */
  letCreativeTriggerRenderStart() {
+    if (this.isInAdSizeOptimizationExperimentBranch()) {


Can you add this in onCreativeRender? It's a more explicit location of when creative rendering occurs as this function is more around analytics triggers and controlling if render should start.

keithwrightbos · 2019-09-05T17:11:39Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+   * @return {string} ad client ID for the current ad unit.
+   */
+  getAdClientId() {
+    const adClientId = (


let adClientId = (this.element.getAttribute('data-ad-client') || '').toLowerCase();
if (!/^ca-/i.test(adClientId)) adClientId = ca-${adClientId};
return adClientId;

keithwrightbos · 2019-09-05T17:12:11Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+   * @return {string} ad size optimization storage key.
+   */
+  getAdSizeOptimizationStorageKey() {
+    return 'aas-' + this.getAdClientId();


return aas-${this.getAdClientId()};

keithwrightbos · 2019-09-06T14:39:11Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+    /**
+     * Testing callback invoked when auto ad size settings are persisted in
+     * localstorage.
+     * {function()} */


Missing @Private

keithwrightbos · 2019-09-06T14:39:41Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

-    // preceding logic (specifically responsive logic).
-    this.divertExperiments();
-    this.maybeAddSinglePassExperiment();
+    return (


Why the paren?

It gets automatically added by the linter:
gulp lint --local_changes --fix
Not sure if there's a way around that.

keithwrightbos · 2019-09-06T14:40:25Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+   * Actually upgrades the ad unit to responsive.
+   */
+  upgradeToResponsive() {
+    this.element.setAttribute('height', ADSENSE_RSPV_WHITELISTED_HEIGHT);


OK please add @lannka for review

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

lannka · 2019-09-06T17:53:48Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+   * Actually upgrades the ad unit to responsive.
+   */
+  upgradeToResponsive() {
+    this.element.setAttribute('height', ADSENSE_RSPV_WHITELISTED_HEIGHT);


AMP layout should have happened before this. But could you pls double confirm locally? (for example print logs that measures the height before / after this call.

lannka · 2019-09-06T17:55:33Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+    this.element.setAttribute('data-full-width', '');
+    this.element.setAttribute('data-auto-format', 'rspv');
+
+    devAssert(this.isValidElement(), 'Upgraded element is not valid.');


devAssert will be stripped out from production binaries. is that what you want?

lannka · 2019-09-06T17:56:15Z

extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js

@@ -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*() {


new test should use async + await

extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js

riklund · 2019-09-30T12:23:21Z

Merged the changes together with the previously landed refactoring PR

PTAL.

extensions/amp-ad-network-adsense-impl/0.1/test/test-responsive-state.js

See ampproject#23568.

rsimha

Changes to build-system/tasks/presubmit-checks.js LGTM.

lannka · 2019-10-01T19:00:39Z

for the OWNERS bot check, @cramforce to approve the amp-localstorage.md change

googlebot added the cla: yes label Sep 5, 2019

keithwrightbos requested changes Sep 5, 2019

View reviewed changes

riklund force-pushed the formatopia-1 branch 2 times, most recently from d07ddc7 to 772658e Compare September 6, 2019 12:14

riklund requested a review from keithwrightbos September 6, 2019 12:21

amp-bundle-size bot requested a review from dreamofabear September 6, 2019 13:03

keithwrightbos approved these changes Sep 6, 2019

View reviewed changes

keithwrightbos requested a review from lannka September 6, 2019 14:42

riklund force-pushed the formatopia-1 branch 3 times, most recently from 9f4dec0 to 76789a5 Compare September 6, 2019 17:28

lannka requested changes Sep 6, 2019

View reviewed changes

riklund force-pushed the formatopia-1 branch 2 times, most recently from 5e85f9c to 099ce8a Compare September 9, 2019 13:33

dreamofabear removed their request for review September 9, 2019 19:59

riklund force-pushed the formatopia-1 branch from 099ce8a to 447deb5 Compare September 30, 2019 12:21

riklund requested review from lannka and keithwrightbos September 30, 2019 12:22

lannka approved these changes Sep 30, 2019

View reviewed changes

extensions/amp-ad-network-adsense-impl/0.1/test/test-responsive-state.js Outdated Show resolved Hide resolved

amp-owners-bot bot requested a review from rsimha September 30, 2019 22:39

riklund force-pushed the formatopia-1 branch from 447deb5 to bd0ff12 Compare October 1, 2019 09:21

Implement ad size optimization for AdSense.

f6aa6a7

See ampproject#23568.

riklund force-pushed the formatopia-1 branch from bd0ff12 to f6aa6a7 Compare October 1, 2019 09:24

rsimha approved these changes Oct 1, 2019

View reviewed changes

lannka requested a review from cramforce October 1, 2019 19:00

cramforce approved these changes Oct 1, 2019

View reviewed changes

lannka merged commit 79878af into ampproject:master Oct 1, 2019

✨ Implement ad size optimization for AdSense. #24375

✨ Implement ad size optimization for AdSense. #24375

Conversation

riklund commented Sep 5, 2019

riklund commented Sep 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riklund commented Sep 30, 2019

rsimha left a comment

Choose a reason for hiding this comment

lannka commented Oct 1, 2019