amp-ima-video: add support for IMASdkSettings as JSON. #10525

shawnbuso · 2017-07-19T14:22:52Z

Adds support for providing IMASdkSettings as a <script type="application/json"> child note of amp-ima-video.

shawnbuso · 2017-07-19T14:23:19Z

validator/validator-main.protoascii

@@ -3145,6 +3145,19 @@ tags: {
    }
  }
 }


This is a guess based on the above config - let me know if this needs to be different.

@shawnbuso You'll also want the cdata section as we don't allow html comments within script tags.

@Gregable as fyi for validation review.

Cool, thanks - updated.

aghassemi · 2017-07-19T19:36:18Z

ads/google/imaVideo.js

@@ -330,6 +333,9 @@ export function imaVideo(global, data) {
      videoPlayer.appendChild(htmlToElement(child));
    });
  }
+  if (data.imaSettings) {
+    imaSettings = JSON.parse(data.imaSettings);


please use tryParseJson (already imported)

aghassemi · 2017-07-19T19:38:27Z

ads/google/imaVideo.js

@@ -378,12 +384,37 @@ export function imaVideo(global, data) {
          false);
    });

+    // Handle settings that need to be set before the AdDisplayContainer is
+    // created.
+    if (imaSettings.locale) {


please use imaSettings['locale'] format everywhere since tryParseJson returns a JsonObject and we like to enable a Closure Compiler optimization which will obfuscate properties

Good catch, thanks - done.

aghassemi · 2017-07-19T19:40:57Z

ads/google/imaVideo.js

+      if (!skippedSettings.includes(setting)) {
+        // Change e.g. 'ppid' to 'setPpid'.
+        const methodName =
+            'set' + setting.charAt(0).toUpperCase() + setting.slice(1);


use camelCaseToTitleCase from style.js

aghassemi · 2017-07-19T19:42:42Z

ads/google/imaVideo.js

+        // Change e.g. 'ppid' to 'setPpid'.
+        const methodName =
+            'set' + setting.charAt(0).toUpperCase() + setting.slice(1);
+        adsLoader.getSettings()[methodName](imaSettings[setting]);


add safety check to prevent exception if user-provided setting happens to be wrong.

aghassemi · 2017-07-19T19:43:32Z

extensions/amp-ima-video/0.1/amp-ima-video.js

@@ -93,6 +94,13 @@ class AmpImaVideo extends AMP.BaseElement {
      this.element.setAttribute(
          'data-child-elements', JSON.stringify(children));
    }
+
+    // Handle IMASetting JSON
+    const scriptElement = this.element.getElementsByTagName('script')[0];


childElementsByTag from dom.js

aghassemi · 2017-07-19T19:44:01Z

extensions/amp-ima-video/0.1/amp-ima-video.js

+
+    // Handle IMASetting JSON
+    const scriptElement = this.element.getElementsByTagName('script')[0];
+    if (scriptElement.type == 'application/json') {


null check as <script> is optional also use isJsonScriptTag from dom.js

aghassemi

LGTM. Small nit and there is a lint error. good to merge when tests pass.

aghassemi · 2017-07-20T16:33:00Z

extensions/amp-ima-video/0.1/amp-ima-video.js

+    const scriptElement = childElementsByTag(this.element, 'SCRIPT')[0];
+    if (scriptElement && isJsonScriptTag(scriptElement)) {
+      this.element.setAttribute(
+          'data-ima-settings', scriptElement./*REVIEW*/innerHTML);


shawnbuso · 2017-07-20T18:54:41Z

@Gregable I think something is wrong with my validator change - the validator build is failing (see travis logs). Any tips? I need it to be set up to allow <script type="application/json">...</script> as a child of my tag.

Gregable · 2017-07-21T20:18:16Z

validator/validator-main.protoascii

+  spec_name: "amp-ima-video > script[type=application/json]"
+  unique: true
+  mandatory_parent: "AMP-IMA-VIDEO"
+  attrs: {


Let's add a dispatch key attribute. This will look like:

attrs: {
name: "amp-ima-video-config"
value: ""
dispatch_key: NAME_DISPATCH
}

This will force the validator to only consider this tagspec for matching if that attribute is present on the tag it's matching. Therefore this tagspec won't produce errors for generic <script> tags found in the document.

Users will need to add that attribute to their script tag, if that's OK.

In the future, we can improve the validator matching and relax this requirement.

Can the dispatch key be on type? seems to work for amp-analytics: https://github.com/ampproject/amphtml/blob/master/extensions/amp-analytics/validator-amp-analytics.protoascii#L35

shawnbuso · 2017-07-28T17:52:58Z

Bump :) I think I have the approvals I need and comments should be resolved.

honeybadgerdontcare · 2017-07-28T18:18:08Z

@shawnbuso github is showing the branch has conflicts in amp-ima-video.js that need to be resolved.

aghassemi · 2017-07-28T18:43:13Z

validator/validator-main.protoascii

+    name: "amp-ima-video-settings"
+    mandatory: true
+    value: ""
+    dispatch_key: NAME_DISPATCH


@Gregable @shawnbuso I think dispatch_key can to go "type" attr so we don't need the extra amp-ima-video-settings attribute.

/cc @honeybadgerdontcare

It looks like that broke things again - I changed it and re-pushed so we can check out the logs.

What @aghassemi suggested would be a different value for dispatch_key when on type. It would be dispatch_key: NAME_VALUE_PARENT_DISPATCH

shawnbuso · 2017-07-28T19:20:38Z

Whoops, my mistake - merge conflicts resolved.

aghassemi

one request and can merge when tests pass. thanks!

aghassemi · 2017-07-29T03:29:46Z

extensions/amp-ima-video/amp-ima-video.md

@@ -65,6 +66,12 @@ VAST-compliant ad response (for examples, see
  <source src="foo.mp4" type="video/mp4">
  <source src="foo.webm" type="video/webm">
  <track label="English subtitles" kind="subtitles" srclang="en" src="subtitles.vtt">
+  <script type="application/json" amp-ima-video-settings>


remove amp-ima-video-settings now that it is gone from validator

plammy · 2017-08-15T13:35:09Z

Hi guys,
who can I test this? I have dev-channel and amp-ima-video experimental features enabled. I have try to set the location to Germany and encrease the number of redirects to 8 but I don't see any changes.

Here is my code:
<amp-ima-video id="myVideo" width="640" height="360" data-tag="https://pubads.g.doubleclick.net/gampad/ads?sz=640x480&iu=/124319096/external/ad_rule_samples&ciu_szs=300x250&ad_rule=1&impl=s&gdfp_req=1&env=vp&output=vmap&unviewed_position_start=1&cust_params=deployment%3Ddevsite%26sample_ar%3Dpremidpost&cmsid=496&vid=short_onecue&correlator=" data-poster="https://ampbyexample.com/img/ima-poster.jpg"> <script type="application/json"> { "locale": "de", "numRedirects": 8 } </script> <source src="https://s0.2mdn.net/4253510/google_ddm_animation_480P.mp4" type="video/mp4"> </amp-ima-video>

Thanks

aghassemi · 2017-08-16T21:43:27Z

Hi @plammy , I just gave your code a try and worked for me (well, at least the locale:"de" part that I could see and verify). We just did a prod push yesterday, that may explain why I see a different result. Could you give it another try?

Thanks,
Ali

plammy · 2017-08-18T11:17:40Z

@aghassemi Yes thank you. Now it works fine for me :)

omeryounus · 2017-11-20T09:04:50Z

@shawnbuso when this release will be available for prod use? Is there any way to use in prod?

aghassemi · 2017-11-21T00:59:24Z

@omeryounus Adding <meta name="amp-experiments-opt-in" content="amp-ima-video"> to your page will enable the experiment for that page, but please note that component is still considered experimental and therefore no guarantee on backward compability in regards to changes until it is officially launched.

omeryounus · 2017-11-21T18:38:55Z

thanks @aghassemi working fine...

amp-ima-video: add support for IMASdkSettings as JSON.

0a9faa7

googlebot added the cla: yes label Jul 19, 2017

shawnbuso commented Jul 19, 2017

View reviewed changes

jridgewell requested a review from aghassemi July 19, 2017 17:50

jridgewell added the Category: Audio&Video label Jul 19, 2017

Added html comment blacklist for json config.

4a2e7e2

aghassemi reviewed Jul 19, 2017

View reviewed changes

aghassemi suggested changes Jul 19, 2017

View reviewed changes

Resolved review comments.

00c52a7

aghassemi approved these changes Jul 20, 2017

View reviewed changes

shawnbuso added 4 commits July 20, 2017 12:53

Fixed line length lint error.

e7be41f

Fixed check-types errors.

6661b2f

Merge branch 'master' of github.com:ampproject/amphtml into settings

f941490

Flip REVIEW to OK for innerHTML.

da5568c

Gregable reviewed Jul 21, 2017

View reviewed changes

shawnbuso added 2 commits July 21, 2017 16:26

Added dispatch key for validator.

20d6c32

Remove unique qualification for amp-ima-video-settings.

9035ce7

Gregable approved these changes Jul 21, 2017

View reviewed changes

aghassemi suggested changes Jul 28, 2017

View reviewed changes

shawnbuso added 2 commits July 28, 2017 14:49

Merge branch 'master' into settings

73a8dea

Remove duplicate import and update validator.

d05fe5b

aghassemi reviewed Jul 29, 2017

View reviewed changes

Updated validator and readme.

008034b

aghassemi approved these changes Aug 2, 2017

View reviewed changes

aghassemi merged commit 3455879 into ampproject:master Aug 2, 2017

shawnbuso deleted the settings branch August 4, 2017 14:16

ampprojectbot added the PR use: In Beta / Experimental label Aug 10, 2017

@@ @@ -3145,6 +3145,19 @@ tags: { @@
                   }
                 }
               }

amp-ima-video: add support for IMASdkSettings as JSON. #10525

amp-ima-video: add support for IMASdkSettings as JSON. #10525

Conversation

shawnbuso commented Jul 19, 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

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

aghassemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnbuso commented Jul 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnbuso commented Jul 28, 2017

honeybadgerdontcare commented Jul 28, 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

shawnbuso commented Jul 28, 2017

aghassemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plammy commented Aug 15, 2017

aghassemi commented Aug 16, 2017

plammy commented Aug 18, 2017

omeryounus commented Nov 20, 2017

aghassemi commented Nov 21, 2017

omeryounus commented Nov 21, 2017

shawnbuso commented Jul 20, 2017 •

edited