Adding a4a support to TripleLift amp-ads by szach · Pull Request #6884 · ampproject/amphtml

szach · 2017-01-05T01:30:53Z

googlebot · 2017-01-05T01:30:55Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

keithwrightbos

Test coverage?

keithwrightbos · 2017-01-05T14:03:06Z

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

+import {AmpA4A} from '../../amp-a4a/0.1/amp-a4a';
+import {base64UrlDecodeToBytes} from '../../../src/utils/base64';
+
+const AMP_SIGNATURE_HEADER = 'X-AmpAdSignature';


Add /** @type {string} */ here and below

keithwrightbos · 2017-01-05T14:03:29Z

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

+  /**
+   * @param {!Element} element
+   */
+  constructor(element) {


Given you're only calling super, why is this necessary?

This was actually in our template code as a placeholder, to show where initialization code could go. I'm adding comments to the template code that this is an optional block and can be removed if there is no content.

keithwrightbos · 2017-01-05T14:04:15Z

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

+
+  /** @override */
+  extractCreativeAndSignature(responseText, responseHeaders) {
+    var signature = null;


replace var with let

keithwrightbos · 2017-01-05T14:08:37Z

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

+
+  /** @override */
+  isValidElement() {
+    return this.element.hasAttribute('src') && this.isAmpAdElement();


you should check for the src attribute within isA4A predicate as isValidElement returning false will cause the ad request to be dropped meaning it does not fallback to 3p

I would suggest at least the same check as you have in your current 3p implementation (https://github.com/ampproject/amphtml/blob/master/ads/triplelift.js):
validateSrcPrefix('https://ib.3lift.com/', src);

keithwrightbos · 2017-01-05T14:12:05Z

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

+
+  /** @override */
+  getAdUrl() {
+    return this.element.getAttribute('src').replace(TRIPLELIFT_BASE_URL, TRIPLELIFT_BASE_A4A_URL);


Do you want to verify that the src attribute has the TRIPLELIFT_BASE_URL to start? I noticed that you're 3p implementation includes validateSrcPrefix('https://ib.3lift.com/'. Without it, this could allow a callout to ANY XHR CORS end point (e.g. src to doubleclick). This is supplied by the publisher so what if it includes javascript(sendMeYourCookies())?

I would add check in the config use A4A predicate as well as isValidElement. Both will ensure proper value well before getAdUrl would execute.

keithwrightbos · 2017-01-05T14:12:46Z

extensions/amp-ad-network-triplelift-impl/0.1/triplelift-a4a-config.js

+ */
+export function tripleliftIsA4AEnabled(win, element) {
+  const a4aRequested = element.getAttribute('data-use-a4a');
+  return !!a4aRequested;


Why not just
return !!element.getAttribute('data-use-a4a');

keithwrightbos · 2017-01-11T17:15:39Z

I have reopened the PR. Sorry I did not see notifications because the PR was closed.
@lannka & @jridgewell to look ASAP given we are hoping to get into today's canary cut

googlebot · 2017-01-11T19:55:44Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

keithwrightbos · 2017-01-11T19:58:16Z

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

+
+  /** @override */
+  isValidElement() {
+    return this.isAmpAdElement();


Let's add check for src attribute and starting with base url you expect just to be safe.

I was being overly cautious... we do not need to include it

keithwrightbos · 2017-01-11T19:59:21Z

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

+    sandbox.restore();
+  });
+
+  describe('#isValidElement', () => {


Can we add a negative test here (create instance of amp-ad-network-triplelift element instead of amp-ad) as well as checks regarding src.

googlebot · 2017-01-11T20:33:07Z

CLAs look good, thanks!

lannka · 2017-01-12T00:48:06Z

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

+import {createElementWithAttributes} from '../../../../src/dom';
+import {createIframePromise} from '../../../../testing/iframe';
+
+describe.only('triplelift-a4a-config', () => {


remove only

jasti assigned keithwrightbos and taymonbeal Jan 5, 2017

keithwrightbos requested changes Jan 5, 2017

View reviewed changes

szach closed this Jan 5, 2017

keithwrightbos mentioned this pull request Jan 6, 2017

Cloudflare ad network example #6882

Merged

keithwrightbos reopened this Jan 11, 2017

keithwrightbos requested changes Jan 11, 2017

View reviewed changes

szach force-pushed the feature/triplelift-a4a branch 2 times, most recently from 745bc5b to f2f1af4 Compare January 11, 2017 20:33

keithwrightbos approved these changes Jan 11, 2017

View reviewed changes

keithwrightbos requested a review from lannka January 11, 2017 20:34

szach force-pushed the feature/triplelift-a4a branch 3 times, most recently from 9519e59 to 0b5bc58 Compare January 11, 2017 22:09

lannka approved these changes Jan 12, 2017

View reviewed changes

lannka self-assigned this Jan 12, 2017

Adding a4a support to TripleLift amp-ads

072ba30

szach force-pushed the feature/triplelift-a4a branch from 0b5bc58 to 072ba30 Compare January 12, 2017 01:15

lannka merged commit 0027e1d into ampproject:master Jan 12, 2017

jridgewell pushed a commit to jridgewell/amphtml that referenced this pull request Jan 31, 2017

Adding a4a support to TripleLift amp-ads (ampproject#6884)

79b071d

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

Adding a4a support to TripleLift amp-ads (ampproject#6884)

9834e19

Conversation

szach commented Jan 5, 2017

Uh oh!

googlebot commented Jan 5, 2017

Uh oh!

keithwrightbos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithwrightbos commented Jan 11, 2017

Uh oh!

googlebot commented Jan 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jan 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants