Add NETLETIX to amp-ads plugins #9211

mschwage · 2017-05-08T20:22:19Z

We want to enable amp-ads for our publishers.
Thanks for your support!
(note we closed the existing PR and repopened to rebase).

googlebot · 2017-05-08T20:22:22Z

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.

mschwage · 2017-05-08T20:25:32Z

I signed it!
CLA was signed for corporation Netletix GmbH

googlebot · 2017-05-09T08:54:05Z

CLAs look good, thanks!

mschwage · 2017-05-09T15:44:43Z

@zhouyx Could you review this PR? Thank you!

alanorozco

Thank you for your PR!

I raised some minor concerns, mostly about style.

Please resolve before approval.

alanorozco · 2017-05-09T20:17:02Z

ads/netletix.js

+    mandatory_data: ['nxkey','nxunit','nxwidth','nxheight'],
+    data,
+  };
+


Please only set one empty newline.

alanorozco · 2017-05-09T20:18:00Z

ads/netletix.js

+    + '&site=' + encodeURIComponent(nxsite)
+    + '&ord=' + rand;
+  const receiveNxAction = function(event)
+  {


Please place the opening bracket on the same line as the assignment.

alanorozco · 2017-05-09T20:19:21Z

ads/netletix.js

+          global.context.noContentAvailable();
+          break;
+        case 'nx-identifier':
+          window.context.reportRenderedEntityIdentifier(


Is there a reason why you're using window.context on some lines and global.context in others?

If not, please use only global.context.

(If you're seeing that the behavior caused by window.context is different than global.context, please let us know. It could be a bug.)

alanorozco · 2017-05-09T20:19:53Z

ads/netletix.md

+      data-nxunit="/60343726/netzathleten_.de"
+      data-nxwidth="300"
+      data-nxheight="250"
+      >


Please set closing > on the same line as the last attribute.

alanorozco · 2017-05-09T20:24:00Z

ads/netletix.js

+ * @param {!Window} global
+ * @param {!Object} data
+ */
+/** @type {{nxasync:string}} */


These @type declarations set here don't do anything and will be ignored by the Closure compiler 🙂 Please remove.

alanorozco · 2017-05-09T20:29:44Z

ads/netletix.js

+  const nxheight = (data.nxheight ? data.nxheight : 'fluid');
+  const nxv = (data.nxv ? data.nxv : '0002');
+  const nxsite = (data.nxsite ? data.nxsite : 'none');
+  const url = ls + '/pb/'


Please use assertHttpsUrl and addParamsToUrl from the ../src/url package.

Three other suggestions:

Inline the intermediate variable expressions in the params object

Use JS-style short-circuiting for default values

Set module-level constants for your defaults

If you employ all of these, the URL assignment snippet should look like this:

const NX_URL_HOST = 'https://call.adadapter.netzathleten-media.de'; const DEFAULT_NX_KEY = 'default'; const DEFAULT_NX_UNIT = 'default'; const DEFAULT_NX_WIDTH = 'fluid'; const DEFAULT_NX_HEIGHT = 'fluid'; const DEFAULT_NX_V = '0002'; const DEFAULT_NX_SITE = 'none'; // ... export function netletix(global, data) { // ... const url = assertHttpsUrl(addParamsToUrl( NX_URL_HOST + '/pb/' + encodeURIComponent(data.nxkey || DEFAULT_NX_KEY), { unit: data.nxunit || DEFAULT_NX_UNIT, width: data.nxwidth || DEFAULT_NX_WIDTH, height: data.nxheight || DEFAULT_NX_HEIGHT, v: data.nxv || DEFAULT_NX_V, site: data.nxsite || DEFAULT_NX_SITE, ord: Math.round(Math.random() * 100000000), })); // ... }

alanorozco · 2017-05-09T20:31:34Z

extensions/amp-ad/amp-ad.md

@@ -31,7 +31,7 @@ limitations under the License.
  </tr>
  <tr>
    <td width="40%"><strong>Required Script</strong></td>
-    <td><code>&lt;script async custom-element="amp-ad" src="https://cdn.ampproject.org/v0/amp-ad-0.1.js">&lt;/script></code> Note: amp-ad may still work without this script, but we highly recommend it for future compatibility</td>
+    <td><code>&lt;script async custom-element="amp-ad" src="https:&sol;&sol;cdn.ampproject.org&sol;v0&sol;amp-ad-0.1.js">&lt;/script></code> Note: amp-ad may still work without this script, but we highly recommend it for future compatibility</td>


Please undo HTML character entity changes.

Thanks for your review - apologies for having you do that many comments - so my turn again ;)
Just a sidenote on this change: i could not get CI through without that change. CI saw them as dead links; will revert nonetheless.

alanorozco · 2017-05-09T20:39:29Z

ads/netletix.js

+    + '&v=' + encodeURIComponent(nxv)
+    + '&site=' + encodeURIComponent(nxsite)
+    + '&ord=' + rand;
+  const receiveNxAction = function(event)


(Nitpicking, optional)

We usually employ fat arrow functions when defining callbacks.

For consistency, it would be preferrable if you inlined the callback handler as a fat-arrow function like this:

window.addEventListener('message', event => { // ... });

alanorozco · 2017-05-09T21:16:46Z

ads/netletix.js

+          global.context.renderStart(renderconfig);
+          if (event.data.width != nxwidth ||
+              event.data.height != nxheight) {
+            window.context.requestResize(event.data.width, event.data.height);


Overlooked this.

What are the possible values of event.data.width and event.data.height?

I'm a little confused since it seems that they're expected to be strings. It's fine if they're pixel values.

Please use parseInt() in the requestResize parameters as they're expected to be numbers.

Both will now already be supplied as number so no need for parseInt here but nxwidth still remains String so will parseInt when comparing. Thanks!

alanorozco · 2017-05-10T16:17:11Z

ads/netletix.js

+  validateData(data,
+      global._netletix_amp.mandatory_data, global._netletix_amp.allowed_data);
+  data.nxwidth = data.nxwidth || DEFAULT_NX_WIDTH;
+  data.nxheight = data.nxheight || DEFAULT_NX_HEIGHT;


Please don't mutate passed parameters. There's no need to short-circuit this here since it's done in the param object below.

Unfortunately i could not get around not being able to get "gulp check-types" pass without explicitely (re)setting those for use in the window.eventlistener. Not explicitely restating currently gives me "Property nxwidth never defined on Object" error.

alanorozco · 2017-05-10T16:18:40Z

ads/netletix.js

+          global.context.renderStart(renderconfig);
+          if (event.data.width &&
+              event.data.height &&
+              event.data.width != parseInt(global.context.data.nxwidth, 10) ||


I'm a little confused as to what you're doing here?

My previous comment meant that requestResize() needs number parameters and the values passed to the data object are strings.

We just wanted to compare actual size to size returned from our delivered ad to resize if required. Currently Event.data is number, data.nx... is string, so you are right if we compare we have to do it the other way around since otherwise the default 'fluid' is definitely "problematic". Apologies.

alanorozco · 2017-05-10T16:19:38Z

ads/netletix.js

+    }), data.ampSlotIndex
+  );
+  window.addEventListener('message', event => {
+    if (event.data.type && startsWith(String(event.data.type), 'nx-')) {


Please use dev().assertString() instead of type coercion.

alanorozco · 2017-05-10T16:19:56Z

ads/netletix.js

+      site: data.nxsite || DEFAULT_NX_SITE,
+      ord: Math.round(Math.random() * 100000000),
+    }), data.ampSlotIndex
+  );


Please push closing paren ) to previous line.

alanorozco · 2017-05-10T16:20:35Z

ads/netletix.js

+/**
+ * @param {!Window} global
+ * @param {!Object} data
+ */


This JSdoc block should be directly above the netletix function signature.

alanorozco · 2017-05-10T16:21:51Z

ads/netletix.js

+            event.data.identifier
+          );
+          break;
+        case 'nx-info':


Please remove empty case.

alanorozco · 2017-05-10T16:23:03Z

ads/netletix.js

+        case 'nx-identifier':
+          global.context.reportRenderedEntityIdentifier(
+            event.data.identifier
+          );


Please push back param to previous line.

If it doesn't fit within the char limit, use 4 extra spaces for indentation and close on the same line.

mschwage · 2017-05-10T18:48:55Z

@alanorozco thanks so far and once again apologies for the number of iterations! Hope i got it right this time.

alanorozco · 2017-05-11T00:12:26Z

ads/netletix.js

+    }), data.ampSlotIndex);
+  window.addEventListener('message', event => {
+    if (event.data.type &&
+      startsWith(dev().assertString(event.data.type), 'nx-')) {


Double indent startsWith line

alanorozco · 2017-05-11T00:13:04Z

ads/netletix.js

+          global.context.renderStart(renderconfig);
+          if (event.data.width &&
+              event.data.height) {
+            if (dev().assertString(event.data.width) != data['nxwidth'] ||


Why are you asserting string here?

Why are you using bracket instead of dot syntax for data.nxwidth?

Switched back to dot syntax and included used parameters in a single if clause. Thanks for pointing me in the right direction.

alanorozco · 2017-05-11T00:13:53Z

ads/netletix.js

+              event.data.height) {
+            if (dev().assertString(event.data.width) != data['nxwidth'] ||
+               dev().assertString(event.data.height) != data['nxheight']) {
+              global.context.requestResize(event.data.width,


If the values in event.data.width and event.data.height are expected to be strings, they should be parseInt'd for requestResize

ah, my bad i misunderstood your comment. both are numbers.

alanorozco · 2017-05-11T00:14:37Z

ads/netletix.js

+            'height': event.data.height,
+          };
+          global.context.renderStart(renderconfig);
+          if (event.data.width &&


If you're immediately nesting an if-condition, please only use one conjuncted expression.

alanorozco · 2017-05-11T00:14:58Z

ads/netletix.js

+  validateData(data,
+      global._netletix_amp.mandatory_data, global._netletix_amp.allowed_data);
+  const url = assertHttpsUrl(addParamsToUrl(
+    NX_URL_FULL + encodeURIComponent(data.nxkey || DEFAULT_NX_KEY),


Double-indent function parameters.

mschwage · 2017-05-11T11:55:26Z

Tried to incorporate your comments @alanorozco let me know what you think. Thanks again for your efforts!

mschwage · 2017-05-15T08:14:33Z

@alanorozco all changes should be made and CI finished OK, can you have a look? thanks!

alanorozco

Sorry for the delay! I got unexpectedly sick. Ready to continue with this. Only a couple of comments.

alanorozco · 2017-05-15T18:28:30Z

ads/netletix.js

+            global.context.data.nxwidth &&
+            global.context.data.nxheight) &&
+            (event.data.width.toString() != global.context.data.nxwidth ||
+            event.data.height.toString() != global.context.data.nxheight)) {


Could you please explain what you are trying to do with all these conditions?

I believe you can just do

if (event.data.width && event.data.height && (event.data.width != data.width || event.data.height != data.height)) { // ... }

Except for type conversions between event.data variables I can agree - but if we don't include data.nxwidth & data.nxheight as conditions "gulp check-types" in our local test fails - that was the main reason. Any idea on how to overcome that (or is this something we only see when testing locally?)

Could switch to your pattern by using a local const
const nxh = (data.nxheight || DEFAULT_NX_HEIGHT); const nxw = (data.nxwidth || DEFAULT_NX_WIDTH);
and then using them within the conditions - hope that approach is ok?

alanorozco · 2017-05-15T18:28:57Z

ads/netletix.js

+            global.context.data.nxheight) &&
+            (event.data.width.toString() != global.context.data.nxwidth ||
+            event.data.height.toString() != global.context.data.nxheight)) {
+            global.context.requestResize(event.data.width,


Please +double indent argument params.

alanorozco · 2017-05-15T18:29:37Z

ads/netletix.js

+      ord: Math.round(Math.random() * 100000000),
+    }), data.ampSlotIndex);
+  window.addEventListener('message', event => {
+    if (event.data.type &&


Please +double indent second line of the condition content:

if (event.data.type && startsWith(dev().assertString(event.data.type), 'nx-')) { //... }

…rtString, conjuncted if vs. nested

mschwage · 2017-05-16T12:22:02Z

Hey, @alanorozco: hope all is fine again and no worries you are still faster in reviewing than us in adapting... So highly appreciate your efforts! Hope you are OK with the solution to the conditions as outlined above - can you have a look? thanks!

alanorozco

Looks good! Just a few minor style concerns and you should be golden :)

alanorozco · 2017-05-16T16:06:51Z

ads/netletix.js

+            'height': event.data.height,
+          };
+          global.context.renderStart(renderconfig);
+          const nxh = (data.nxheight || DEFAULT_NX_HEIGHT);


I see your problem with check-types and I think using intermediate variables is an OK solution.

One minor request: could you define these two intermediate variables at the function level (i.e. at the beginning of the function, right after validateData)?

alanorozco · 2017-05-16T16:08:52Z

ads/netletix.js

+          if (event.data.width &&
+              event.data.height &&
+              (event.data.width != nxh ||
+              event.data.height != nxw)) {


Two things:

Minor request: please fold these in one line when it fits.

It looks like you're comparing width with nxh and height with nxw and I think it should be the other way around.

if (event.data.width && event.data.height && (event.data.width != nxw || event.data.height != nxh)) { // ... }

Ups, you are right in re to nxh/w thanks for highlighting!

alanorozco · 2017-05-16T16:09:11Z

ads/netletix.js

+              event.data.height &&
+              (event.data.width != nxh ||
+              event.data.height != nxw)) {
+            global.context.requestResize(event.data.width,


Please fold function call in one line as it fits in 80 chars.

alanorozco · 2017-05-16T16:14:48Z

ads/netletix.js

+      v: data.nxv || DEFAULT_NX_V,
+      site: data.nxsite || DEFAULT_NX_SITE,
+      ord: Math.round(Math.random() * 100000000),
+    }), data.ampSlotIndex);


Two minor requests:

Please add one empty newline in between major blocks.

Please indent the url var assignment in a way that reflects the function composition.

Example:

export function netletix(global, data) { /*eslint "google-camelcase/google-camelcase": 0*/ global._netletix_amp = ... validateData(...); const url = assertHttpsUrl( addParamsToUrl( NX_URL_FULL + encodeURIComponent(data.nxkey || DEFAULT_NX_KEY), { //... }), data.ampSlotIndex); window.addEventListener('message', event => { //... }); if (data.async && data.async.toLowerCase() === 'true') { // ... } else { // ... } }

mschwage · 2017-05-17T07:08:44Z

Thanks for your comments @alanorozco - looking forward to your thoughts!

alanorozco · 2017-05-17T16:00:17Z

Thank you for your patience working on this!

The PR will be merged now. Please wait during the upcoming weeks for release.

googlebot added the cla: no label May 8, 2017

jridgewell added the WG: monetization label May 8, 2017

googlebot added cla: yes and removed cla: no labels May 9, 2017

zhouyx added this to Backlog in 3P Ads & Analytics Support May 9, 2017

lannka requested a review from alanorozco May 9, 2017 20:06

lannka assigned alanorozco May 9, 2017

alanorozco requested changes May 9, 2017

View reviewed changes

alanorozco reviewed May 9, 2017

View reviewed changes

lannka moved this from Backlog to In Review in 3P Ads & Analytics Support May 9, 2017

alanorozco requested changes May 10, 2017

View reviewed changes

alanorozco requested changes May 11, 2017

View reviewed changes

alanorozco requested changes May 15, 2017

View reviewed changes

Martin Schwager added 10 commits May 16, 2017 13:36

amp-ads for NETLETIX

3128680

changed //to <td><code><script async custom-element=amp-ad src=https:

905cb32

changed indexOf to startWith as suggested by gulp presubmit

8852654

deleted unused variable according to lint

2d026cf

fixed potentiyl type issue named by check-types

de70e47

trigger googlebot

f21e61d

removed trailing spaces

9d72be6

adapted as commented by alanorozco

589d2a7

adapted based on additional comments from alanorozco

9ce7ef9

changed idents, switched to . notation, removed wrongly inserted asse…

d66c4f7

…rtString, conjuncted if vs. nested

Martin Schwager added 2 commits May 16, 2017 13:36

missed one

014e16a

adapted based on comments by alanorozco

f594b49

alanorozco reviewed May 16, 2017

View reviewed changes

Martin Schwager added 2 commits May 17, 2017 08:06

style changes and correction of nxh/w mixup as pointed out by alanorozco

25e3009

changed indent level

e4dc7a5

alanorozco approved these changes May 17, 2017

View reviewed changes

alanorozco merged commit c4ad5a7 into ampproject:master May 17, 2017

lannka moved this from In Review to Done in 3P Ads & Analytics Support May 17, 2017

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels May 23, 2017

Add NETLETIX to amp-ads plugins #9211

Add NETLETIX to amp-ads plugins #9211

Conversation

mschwage commented May 8, 2017

googlebot commented May 8, 2017

mschwage commented May 8, 2017

googlebot commented May 9, 2017

mschwage commented May 9, 2017

alanorozco left a comment • edited

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

alanorozco May 9, 2017 • edited

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

mschwage commented May 10, 2017

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

mschwage commented May 11, 2017

mschwage commented May 15, 2017

alanorozco 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

mschwage commented May 16, 2017

alanorozco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanorozco May 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanorozco May 16, 2017 • edited

Choose a reason for hiding this comment

mschwage commented May 17, 2017

alanorozco commented May 17, 2017

alanorozco left a comment •

edited

alanorozco May 9, 2017 •

edited

alanorozco May 16, 2017 •

edited

alanorozco May 16, 2017 •

edited