Integrate Google AdSense for Search and Adsense for Shopping (on the Custom Search Ads protocol) with AMP #6518

megankj-zz · 2016-12-07T17:49:49Z

This change allows publishers to request Google AdSense for Search and AdSense for Shopping on the Custom Search Ads protocol. We introduce a new AMP ad type, called "csa".

#2659

megankj-zz · 2016-12-08T17:46:47Z

@lannka Can you take a look at this?

megankj-zz · 2016-12-08T17:47:35Z

@jasti FYI

jscud

FWIW I've reviewed this code, though I'm not a project owner.

lannka

Thanks for the PR!

I saw some methods with a hundred lines. Could you please extract out some functions for better readability?

lannka · 2016-12-08T17:50:57Z

ads/google/csa.js

+ * @param {Object} container HTML element of the CSA container
+ * @param {number} height Height to resize, in pixels
+ */
+function resizeCsa(container, height) {


move private method to the bottom

lannka · 2016-12-08T17:51:37Z

ads/google/csa.js

+ * @param {*} backfillPageOptions AFS page options (if necessary)
+ * @param {*} backfillAdblockOptions AFS ad unit options (if necessary)
+ * @return {function(string,boolean)} The callback function
+ */


add annotation @visibleForTesting if it's public only for testing purpose.

lannka · 2016-12-08T17:54:23Z

ads/google/csa.js

+ * This is needed for an iOS bug found in versions 10.0.1 and below that
+ * doesn't properly reflow the iframe upon orientation change.
+ */
+window.addEventListener('orientationchange', function() {


is it necessary to have this call outside your main function?

megankj-zz · 2016-12-12T18:56:23Z

@lannka Any other feedback?

jridgewell · 2016-12-13T20:50:24Z

ping @lannka

lannka · 2016-12-15T18:49:15Z

ads/google/csa.js

+  // Get parent width in case we want to override
+  const width = global.document.body.clientWidth;
+
+  validateData(data, [], ['afshPageOptions',


nitpick: please break line before 'afshPageOptions', and after 'ampSlotIndex'

lannka · 2016-12-15T18:58:05Z

ads/google/csa.js

+      afshPageOptions['source'] = 'amp';
+      afshPageOptions['referer'] = window.context.referrer;
+    } catch (e) {}
+  }


you can leverage Object.assign

import {tryParseJson} from 'src/json.js'; const afshPageOptions = Object.assign( {source: 'amp', referer: global.context.referrer}, tryParseJson(data['afshPageOptions']));

you might need to run gulp dep-check to see if there's dependency errors. adding json.js to the whitelist should fix that.

tryParseJson isn't gauranteed to return an Object, so gulp check-types complains when I do this

actual parameter 1 of Object.assign does not match formal parameter found : (*|undefined) required: Object tryParseJson(data['afsPageOptions']),

lannka · 2016-12-15T19:00:18Z

ads/google/csa.js

+   * This is needed for an iOS bug found in versions 10.0.1 and below that
+   * doesn't properly reflow the iframe upon orientation change.
+   */
+  window.addEventListener('orientationchange', function() {


use arrow function

lannka · 2016-12-15T19:00:25Z

ads/google/csa.js

+    // Save the height of the container before the event listener triggers
+    const oldHeight = document.getElementById('csacontainer').style.height;
+
+    setTimeout(function() {


lannka · 2016-12-15T19:00:56Z

ads/google/csa.js

+                          'ampSlotIndex']);
+
+  // Add the ad container to the document
+  const d = global.document.createElement('div');


can you give it a better name, like containerDiv?

lannka · 2016-12-15T19:15:41Z

ads/google/csa.js

+ * @param {Node} container HTML element of the CSA container
+ * @param {number} containerH Height to resize the CSA container, in pixels
+ */
+function createOverflow(overflowH, fullH, container, containerH) {


nit: please swap the position of createOverFlow and resizeCsa. Easier for reader to follow the logic.

lannka · 2016-12-15T19:18:10Z

ads/google/csa.js

+      global._googCsa('plas', afshPageOptions, afshAdblockOptions);
+    }
+  });
+


Why do you want me to remove this? This is an ad call.

i meant remove the blank line :-)

lannka · 2016-12-15T19:18:54Z

ads/google/csa.js

+ * @return {function(string,boolean)} The callback function
+ * @visibleForTesting
+ */
+export function generateCallback(backfillPageOptions, backfillAdblockOptions) {


you will want to pass in the global object

lannka · 2016-12-15T19:19:27Z

ads/google/csa.js

+
+/**
+ * Create a callback function to resize the iframe and/or request backfill
+ * @param {*} backfillPageOptions AFS page options (if necessary)


If I annotate is as anything other than *, gulp check-types complains

found : (*|undefined) required: Object tryParseJson(data['afshPageOptions'])

lannka · 2016-12-15T19:22:21Z

ads/google/csa.js

+   * @param {string} containerName
+   * @param {boolean} adsLoaded
+   */
+  const resizeIframe = function(containerName, adsLoaded) {


instead of return a function, we typically do:

afshAdblockOptions['adLoadedCallback'] = resizeIframe.bind(null, afsPageOptions, afsAdblockOptions); function resizeIframe(backfillPageOptions, backfillAdblockOptions, containerName, adsLoaded) { ... }

megankj-zz

Latest round of changes are done, but this code doesn't pass check-types because JSON.parse isn't guaranteed to return an Object.

change some variable names, mostly condense code

global.innerHeight doesn't work because I need the height of the AMP container at the time of requesting the ads before the iframe is resized. Different browsers have different behaviors (some bust the iframe, some don't). Changing to global.innerHeight broke the overflow implementation on iOS.

iOS9 appears to have a race condition and sometimes shows the overflow on top of the ads, even though ALL containers and iframe appear to have the right height set . Adding this to see if it fixes the issue. This doesn’t appear to be a problem for iOS10 or Android.

This reverts commit 2040f76.

I’m adding this because on ios9, the ad content is visible underneath the overflow element, even though the height of the container and iframe are being set correctly.

megankj-zz · 2016-12-17T01:53:37Z

There is some weirdness I can't explain with iOS9 (9.3.5) that shows the overflow element on top of the adblock. When I inspect it, the heights for the AMP ad, AMP iframe, CSA div, and CSA iframe are all set correctly. I tried setting overflow-y: 'hidden' in the code and that didn't fix the problem either. If I scroll down and back up, the problem fixes itself. If I play around with the CSS properties (and don't actually change anything, just set it back to what it was before) it fixes itself.

Not sure what's going on here but I'll investigate more on Monday.

…mplementation

megankj-zz · 2016-12-19T21:37:36Z

On iOS9, when a resize is requested sometimes the CSA iframe busts and then returns to it's full height even though we have set the height via CSS. I was able to stop this behavior by setting the iframe width to "100%", but you can still see a flicker of the ad expanding and contracting. For some reason this only happens some of the time on iOS9 and not at all on iOS10 or Android. Perhaps this is related to the iframe reflow issue that required me to add an event listener for orientation change.

One possible way to stop the flicker is to make the overflow background a solid color and not transparent, but that locks partners into a particular styling which we probably don't want to do.

Test page:
http://ampcsa.herokuapp.com/examples/afstop.amp.min.html

lannka

If you had iOS9 particular bug that is hard to fix, I suggest you add a TODO and open an issue. We can get this PR in first.

lannka · 2016-12-19T22:54:59Z

ads/google/csa.js

+    });
+
+    // If resize was denied, show overflow element and resize container
+    global.context.onResizeDenied(function(requestedHeight) {


will there be a problem when resizeIframe() get called multiple times? Seems we're going to register multiple listener here, which could create multiple overflow elements?

Resize iframe is only called multiple times if the partner wants to make a new request for (AFS) ads if the first (AFSh) request didn't return anything. In this case, we won't have made an overflow or created any listeners because adsLoaded in resizeIframe will be false.

The resizeDenied and resizeSuccess functions check for existence of overflow before creating a new one and/or resizing.

Even though, I think logically it's better to move this logic out of the resize handler, to make sure we only register once.

lannka · 2016-12-19T22:56:13Z

ads/google/csa.js

+  const pageOptions = {source: 'amp', referer: global.context.referrer};
+  const adblockOptions = {container: containerId};
+  const afshPageOptions = Object.assign(
+      Object(tryParseJson(data['afshPageOptions'])), pageOptions);


did you try swap the argument position? That will help you get rid of Object().

Object.assign(pageOptions, tryParseJson(data['afshPageOptions']));

Ah, never mind. that will mutate pageOptions.

Switching the order does not get rid of the check-types error. Furthermore, if a partner passes in "source" or "referrer" they would end up overriding what we set if we do it in this order.

lannka · 2016-12-19T22:58:29Z

ads/google/csa.js

+  global.addEventListener('orientationchange', () => {
+    // Save the height of the container before the event listener triggers
+    const oldHeight = containerDiv.style.height;
+    setTimeout(() => {


global.setTimeout

lannka · 2016-12-19T22:58:53Z

ads/google/csa.js

+   */
+  global.addEventListener('orientationchange', () => {
+    // Save the height of the container before the event listener triggers
+    const oldHeight = containerDiv.style.height;


could you extract out the whole listener to a function?

lannka · 2016-12-19T22:59:59Z

ads/google/csa.js

+
+  // Only call for ads once the script has loaded
+  loadScript(global, 'https://www.google.com/adsense/search/ads.js', () => {
+    if (data['afsPageOptions'] != null && data['afshPageOptions'] == null) {


could you extract out the load callback to a function?

lannka · 2016-12-20T18:04:47Z

ads/google/csa.js

+    });
+
+    // If resize was denied, show overflow element and resize container
+    global.context.onResizeDenied(function(requestedHeight) {


Even though, I think logically it's better to move this logic out of the resize handler, to make sure we only register once.

lannka · 2016-12-20T18:06:27Z

ads/google/csa.js

+  }
+
+  // Event listener needed for iOS9 bug
+  global.addEventListener('orientationchange', () => {


global.addEventListener('orientationchange', orientationChangeHandler.bind(null, global, containerDiv));

lannka · 2016-12-20T18:06:49Z

ads/google/csa.js

+
+  // Only call for ads once the script has loaded
+  loadScript(global, 'https://www.google.com/adsense/search/ads.js', () => {
+    requestCsaAds(global, data, afsPage, afsAd, afshPage, afshAd);


use bind here as well

lannka · 2016-12-20T18:08:21Z

ads/google/csa.js

+let currentAmpHeight = null;
+
+// Enum to hold different ad types
+const ADTYPE = {


/** @enum {string} */

lannka · 2016-12-20T18:08:59Z

ads/google/csa.js

+// Enum to hold different ad types
+const ADTYPE = {
+  UNSUPPORTED: 0,
+  AFS: 1,


can you add some description about the enum value?

lannka · 2016-12-20T18:10:17Z

test/functional/ads/test-csa.js

+import * as sinon from 'sinon';
+import * as _3p from '../../../3p/3p';
+
+describe('amp-ad-csa-impl', () => {


seems this describe wrapper can be omitted?

…resizeIframe other minor refactoring as well

megankj-zz · 2016-12-20T22:13:36Z

I addressed your comments and tried to remove unnecessary code to make it more compact (and in some cases fit on one line). Let me know if I should reorder the helper functions or do anything else to clean it up.

lannka

Some more nitpicks

lannka · 2016-12-20T22:09:44Z

ads/google/csa.js

+
+/**
+ * Enum for different AdSense Products
+ * UNSUPPORTED: Value if we can't determine which product to request


nitpick, you can inline descriptions.

/** Value if we can't determine which product to request */ UNSUPPORTED: 0,

lannka · 2016-12-20T22:12:10Z

ads/google/csa.js

+      Object(tryParseJson(data['afsAdblockOptions'])), adblockOptions);
+
+  // Special case for AFSh when "auto" is the requested width
+  if (afshAd != null && afshAd['width'] == 'auto') {


seems afshAd is guaranteed not null

lannka · 2016-12-20T22:13:53Z

ads/google/csa.js

+ * @param {?Object} afsP The parsed AFS page options object
+ * @param {?Object} afsA The parsed AFS adblock options object
+ * @param {?Object} afshP The parsed AFSh page options object
+ * @param {?Object} afshA The parsed AFSh adblock options object


seems none of the arguments can be null

lannka · 2016-12-20T22:14:52Z

ads/google/csa.js

+ */
+function requestCsaAds(global, data, afsP, afsA, afshP, afshA) {
+  const type = getAdType(data);
+  if (type == ADTYPE.AFS) {


is switch more appropriate here?

lannka · 2016-12-20T22:16:33Z

ads/google/csa.js

+ * AFSHBACKFILL: AdSense for Shopping, backfilled with AdSense for Search
+ * @enum {number}
+ */
+const ADTYPE = {


lannka · 2016-12-20T22:16:41Z

ads/google/csa.js

+  UNSUPPORTED: 0,
+  AFS: 1,
+  AFSH: 2,
+  AFSHBACKFILL: 3,


AFSH_BACKFILL

lannka · 2016-12-20T22:17:43Z

test/functional/ads/test-csa.js

+ * AFSHBACKFILL: AdSense for Shopping, backfilled with AdSense for Search
+ * @enum {number}
+ */
+const ADTYPE = {


can you just import this enum?

lannka

LGTM with one more comment. Please also resolve merge conflicts.

lannka · 2016-12-21T04:57:41Z

ads/google/csa.js

+      global._googCsa('plas', afshP, afshA);
+      break;
+    default:
+      return;


nit: return is not needed

lannka · 2016-12-21T05:06:09Z

ads/google/csa.js

+ * @param {boolean} ads If ads were returned or not
+ * @visibleForTesting
+ */
+export function resizeIframe(global, bkflPage, bkflAd, conName, ads) {


shall we just split this function into 2?

function resizeIframe(global, containerName, hasAd) {} function backfill(global, backfillPage, backfillAd) {}

it gives not just shorter argument list, but also avoided the recursive call of resizeIframe.

Then the above listener code becomes:

afshA['adLoadedCallback'] = (containerName, hasAd) => { if (hasAd) { resizeIframe(global, containerName, hasAd); } else { backfill(global, afsP, afsA) } }

# Conflicts: # ads/google/csa.js

# Conflicts: # 3p/integration.js # ads/_config.js # ads/google/adsense.md # examples/ads.amp.html # extensions/amp-ad/amp-ad.md

lannka · 2016-12-21T22:34:29Z

ads/google/csa.js

+ * @param {?Object} page Backfill AFS page options (if necessary)
+ * @param {?Object} adblock Backfill AFS ad unit options (if necessary)
+ */
+function backfillAds(global, page, adblock) {


this little function is only used once, you can inline.

…Custom Search Ads protocol) with AMP (ampproject#6518) * Initial commit for CSA AMP ad integration * update ads example * assign throwaway variable so compiler doesn't complain * fix lint errors * define _googCsa in backfill function * Add link to CSA docs in AdSense docs * fix test error * fix type error * feedback from jeff * update css example * create overflow on resize denied if it doesn't exist * fix from jeff's comments * initial stab at unit tests * first stab at unit tests * add test for backfill * lint issues * create helper functions and move to bottom * hongfei's feedback * use Object.assign and tryparsejson * update tests * code review changes change some variable names, mostly condense code * change global.innerHeight back to initial height global.innerHeight doesn't work because I need the height of the AMP container at the time of requesting the ads before the iframe is resized. Different browsers have different behaviors (some bust the iframe, some don't). Changing to global.innerHeight broke the overflow implementation on iOS. * force dom reflow after appending overflow iOS9 appears to have a race condition and sometimes shows the overflow on top of the ads, even though ALL containers and iframe appear to have the right height set . Adding this to see if it fixes the issue. This doesn’t appear to be a problem for iOS10 or Android. * Revert "force dom reflow after appending overflow" This reverts commit 2040f76. * hide overflow of CSA iframe I’m adding this because on ios9, the ad content is visible underneath the overflow element, even though the height of the container and iframe are being set correctly. * Set iframe width to 100% to prevent ios9 from breaking the overflow implementation * move code into functions * Move resize handlers to own functions, register in CSA function, not resizeIframe other minor refactoring as well * nitpicks * add forgotten breaks to switch statement * Create two callback functions, one for backfill and one without. * Merge remote-tracking branch 'ampproject/master' into csa-integration # Conflicts: # ads/google/csa.js * Inline backfill ads function * Merge with adsense.md accidentally replaced HC link * travis errors * more travis errors

megankj added 16 commits November 3, 2016 18:14

Initial commit for CSA AMP ad integration

9141d53

update ads example

743b5c7

assign throwaway variable so compiler doesn't complain

3aa6786

fix lint errors

2d8301c

define _googCsa in backfill function

61b027f

Add link to CSA docs in AdSense docs

66b2794

fix test error

ae8e9a4

fix type error

321b74a

feedback from jeff

dbac823

update css example

fb23abf

create overflow on resize denied if it doesn't exist

2dc8c7f

fix from jeff's comments

968d93a

initial stab at unit tests

30b88ca

first stab at unit tests

02e5f43

add test for backfill

25d7cac

lint issues

b38e6a4

jscud approved these changes Dec 8, 2016

View reviewed changes

lannka self-assigned this Dec 8, 2016

lannka added 3P WG: monetization labels Dec 8, 2016

lannka requested changes Dec 8, 2016

View reviewed changes

create helper functions and move to bottom

6148cc2

lannka requested changes Dec 15, 2016

View reviewed changes

hongfei's feedback

2803f73

megankj-zz commented Dec 16, 2016

View reviewed changes

use Object.assign and tryparsejson

8a0588f

megankj added 5 commits December 16, 2016 13:54

code review changes

0edfe45

change some variable names, mostly condense code

Revert "force dom reflow after appending overflow"

733eea8

This reverts commit 2040f76.

hide overflow of CSA iframe

368aec7

I’m adding this because on ios9, the ad content is visible underneath the overflow element, even though the height of the container and iframe are being set correctly.

Set iframe width to 100% to prevent ios9 from breaking the overflow i…

594787c

…mplementation

lannka reviewed Dec 19, 2016

View reviewed changes

move code into functions

f152a6b

lannka reviewed Dec 20, 2016

View reviewed changes

Move resize handlers to own functions, register in CSA function, not …

8ad6f35

…resizeIframe other minor refactoring as well

lannka reviewed Dec 20, 2016

View reviewed changes

megankj added 2 commits December 20, 2016 14:38

nitpicks

8183a36

add forgotten breaks to switch statement

579a4a5

lannka approved these changes Dec 21, 2016

View reviewed changes

megankj added 3 commits December 21, 2016 13:56

Create two callback functions, one for backfill and one without.

e06b1e3

Merge remote-tracking branch 'ampproject/master' into csa-integration

ad7721d

# Conflicts: # ads/google/csa.js

Merge branch 'ampproject/master' into csa-integration

d2fcd09

# Conflicts: # 3p/integration.js # ads/_config.js # ads/google/adsense.md # examples/ads.amp.html # extensions/amp-ad/amp-ad.md

lannka approved these changes Dec 21, 2016

View reviewed changes

megankj added 4 commits December 21, 2016 14:40

Inline backfill ads function

2ada432

Merge with adsense.md accidentally replaced HC link

3f3617f

travis errors

d30ff10

more travis errors

ad9b7e5

lannka merged commit cd8cf89 into ampproject:master Dec 22, 2016

Integrate Google AdSense for Search and Adsense for Shopping (on the Custom Search Ads protocol) with AMP #6518

Integrate Google AdSense for Search and Adsense for Shopping (on the Custom Search Ads protocol) with AMP #6518

Conversation

megankj-zz commented Dec 7, 2016 • edited Loading

megankj-zz commented Dec 8, 2016

megankj-zz commented Dec 8, 2016

jscud left a comment

Choose a reason for hiding this comment

lannka left a 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

megankj-zz commented Dec 12, 2016

jridgewell commented Dec 13, 2016

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

megankj-zz left a comment

Choose a reason for hiding this comment

megankj-zz commented Dec 17, 2016

megankj-zz commented Dec 19, 2016

lannka left a 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

megankj-zz commented Dec 20, 2016

lannka left a 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

lannka left a 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

megankj-zz commented Dec 7, 2016 •

edited

Loading