Add amp-hulu element for Hulu videos. #5993

unbug · 2016-11-03T02:15:43Z

Intent to use amp-hulu element in Hulu website.

googlebot · 2016-11-03T02:15:47Z

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.

googlebot · 2016-11-03T03:32:27Z

CLAs look good, thanks!

cramforce · 2016-11-03T03:46:55Z

@unbug Out of curiosity: Are you working for Hulu?

unbug · 2016-11-03T03:49:44Z

@cramforce I am :)
Check out our profile https://github.com/hulu

aghassemi · 2016-11-03T15:31:47Z

@unbug, thanks for the PR.

Couple of questions before the code review:

1- Is any publisher allowed to use <amp-hulu> embeds or is it meant to only be used by Hulu's own AMP pages? ( 'https://secure.hulu.com` seems to be refusing origins other than http://*.hulu.com https://*.hulu.com *.optimizely.com)

2- Does the Hulu's iframe embed have a postMessage API that would allow <amp-hulu> to send commands such as pause, play, mute, unmute and receive events such as played, paused? We are requesting all new video players to implement our video-interface which would require such functionality.

unbug · 2016-11-04T01:41:53Z

@aghassemi Thanks for your time.

<amp-hulu/> currently only servers Hulu's own AMP pages, but we do have a white list domain for publishers.
Hulu's iframe embed doesn't have any postMessage API yet.

aghassemi · 2016-11-04T16:35:28Z

@unbug Any reason <amp-iframe> does not fit your needs? We normally don't allow components that are meant for a single site.

unbug · 2016-11-06T04:44:32Z

@aghassemi Thanks for your time :)

<amp-hulu/> will be used on Hulu's own sites first, and we have some partner publishers, they will try it too.
We don't use <amp-iframe> because we think <amp-hulu/> is more easer for us to maintain and update, and it's more easer for our partners to keep it up to date.
We just made a plan for the postMessage API, it's belong to Hulu Player Project, but we don't want to leave <amp-hulu/> behind, so we'd like to lunch <amp-hulu/> ASAP for our site team and partners.

jridgewell · 2016-11-07T20:58:20Z

extensions/amp-hulu/0.1/amp-hulu.js

+    iframe.setAttribute('allowfullscreen', 'true');
+    iframe.src = 'https://secure.hulu.com/dash/mobile_embed.html?eid=' + encodeURIComponent(eid);
+    this.applyFillContent(iframe);
+    iframe.width = width;


Setting the width and height on the iframe shouldn't be necessary as it's "fill-content".

aghassemi · 2016-11-07T21:53:00Z

@unbug

will be used on Hulu's own sites first, and we have some partner publishers, they will try it too.

That's fine but keep in mind that AMP pages can be served from caches such as cdn.ampproject.org. Whitelisting cdn.ampproject.org would allow any AMP page to embed <amp-hulu>, if that's not desired:

We are working on allowing publishers to get their own subdomain e.g. publisher.cdn.ampproject.org but that's not launched yet.
For now, to accurately validate access based on origin, you would need to rely on the __amp_source_origin param in addition to the referrer. So:
1. pass the your iframe src to getCorsUrl in url.js that would add a new query string param __amp_source_origin with the true origin of the AMP document to your Url.
2. Have hulu's server-side code allow access if referrer is cdn.ampproject.org AND __amp_source_origin's value is your whitelisted publisher.
3. Note that relying on __amp_source_origin is safe if referrer is cdn.ampproject.org since AMP validation does not allow __amp_source_origin to be manually set by anyonem, so AMP pages can't hack
  your access protection by adding __amp_source_origin to any src or href (they won't pass validation)

We just made a plan for the postMessage API, it's belong to Hulu Player Project,

Good to hear, please implement the video-interface as soon as you have the necessary hooks. Implementing the video-interface will automatically enable features such as AMP-managed autoplay for your player.

I will send out the code review soon.

aghassemi

Additional requests:
1- Validation file (validator-amp-hulu.protoascii) is missing. Please take a look at <amp-youtube> or other amp components for reference.

2- amp-hulu needs to be added to gulpfile.js to be built.

3- Please only submit when cdn.ampproject.org as referrer works as per previous comment.

aghassemi · 2016-11-07T21:56:41Z

extensions/amp-hulu/0.1/amp-hulu.js

+
+  /** @override */
+  createdCallback() {
+    this.preconnect.url('https://secure.hulu.com/dash/mobile_embed.html');


move to preconnectCallback override.

aghassemi · 2016-11-07T21:57:25Z

extensions/amp-hulu/0.1/amp-hulu.js

+
+  /** @override */
+  createdCallback() {
+    this.preconnect.url('https://secure.hulu.com/dash/mobile_embed.html');


just preconnect to https://secure.hulu.com, the path segment is not useful.

'https://secure.hulu.com` is refusing origins other than http://.hulu.com https://.hulu.com *.optimizely.com, but any request's referer is https://secure.hulu.com/dash/mobile_embed.html will not.
Should we preconnect to https://secure.hulu.com/dash/mobile_embed.html?

that's fine then, I did not realize 'https://secure.hulu.com` and tps://secure.hulu.com/dash/mobile_embed.html have different access policies.

aghassemi · 2016-11-07T21:58:16Z

extensions/amp-hulu/0.1/amp-hulu.js

+import {user} from '../../../src/log';
+
+class AmpHulu extends AMP.BaseElement {
+


add constructor where iframe_ is defined.

/** @param {!AmpElement} element */ constructor(element) { super(element); /** @private {?Element} */ this.iframe_ = null; }

aghassemi · 2016-11-07T21:59:34Z

extensions/amp-hulu/0.1/amp-hulu.js

+    iframe.width = width;
+    iframe.height = height;
+    this.element.appendChild(iframe);
+    /** @private {?Element} */


remove this type annotation (since it will be annotated in the constructor per request above))

aghassemi · 2016-11-07T22:01:26Z

extensions/amp-hulu/0.1/amp-hulu.js

+  /** @override */
+  pauseCallback() {
+    if (this.iframe_ && this.iframe_.contentWindow) {
+      this.iframe_.contentWindow./*OK*/postMessage('pause', '*');


does this actually work? I thought there is no postMessage API. If this does not work yet, please remove the pauseCallaback and set

/** @override */ unlayoutOnPause() { return true; }

until you have postMessage support.

aghassemi · 2016-11-07T22:08:34Z

extensions/amp-hulu/amp-hulu.md

+    <td class="col-fourty"><strong><a href="https://www.ampproject.org/docs/guides/responsive/control_layout.html">Supported Layouts</a></strong></td>
+    <td>fill, fixed, fixed-height, flex-item, nodisplay, responsive</td>
+  </tr>
+  <tr>


remove it instead of a TODO, add back when example is there.

aghassemi · 2016-11-07T22:10:43Z

extensions/amp-hulu/amp-hulu.md

+    <td>(TODO) <a href="https://ampbyexample.com/components/amp-hulu/">Annotated code example for amp-hulu</a></td>
+  </tr>
+</table>
+


Please mention the restrictions (in bold warning) and instructions on how publishers can signup for whitelisting.

aghassemi · 2016-11-07T22:11:08Z

extensions/amp-hulu/amp-hulu.md

+## Example
+
+```html
+<amp-hulu width="400" height="400"


add layout="responsive" to the example

aghassemi · 2016-11-07T22:25:26Z

extensions/amp-hulu/0.1/amp-hulu.js

+
+  /** @override */
+  layoutCallback() {
+    const width = this.element.getAttribute('width');


as per @jridgewell's comment, no need to read the width/height values.

aghassemi · 2016-11-07T22:25:49Z

extensions/amp-hulu/amp-hulu.md

+## Example
+
+```html
+<amp-hulu width="400" height="400"


Please use correct aspect ratio for this video ( width="412" height="231" ?)

unbug · 2016-11-08T03:12:39Z

@aghassemi Thanks for your time :)

AMP pages can be served from caches such as cdn.ampproject.org

I just talked to my team about the Content-Security-Policy issue. Yes, 'https://secure.hulu.com` is refusing origins other than http://*.hulu.com https://*.hulu.com *.optimizely.com, but any request's referer is https://secure.hulu.com/dash/mobile_embed.html will not.

But I don't understand for https://secure.hulu.com/dash/mobile_embed.html why any AMP pages can't be served from caches such as cdn.ampproject.org?
And why other websites can't use <amp-hulu>?
If we implement the video-interface will all these issues gone?

aghassemi · 2016-11-08T19:06:31Z

But I don't understand for https://secure.hulu.com/dash/mobile_embed.html why any AMP pages can't be served from caches such as cdn.ampproject.org?
And why other websites can't use ?

I was under the impression that Hulu only allows certain origins to be able to embed Hulu videos, if that's not the case and any origin can embed Hulu videos, then it is not an issue at all.

erwinmombay · 2016-11-09T17:30:22Z

extensions/amp-hulu/0.1/validator-amp-vine.protoascii

+# limitations under the license.
+#
+
+tags: {  # amp-hulu


@Gregable @powdercloud @honeybadgerdontcare can somebody review this? seems to be causing a failure on the validator tests

Please rename the file to validator-amp-hulu.protoascii.

aghassemi · 2016-11-09T17:49:53Z

@unbug Please let me know when this is ready for review again. Thanks!

honeybadgerdontcare · 2016-11-09T17:50:12Z

extensions/amp-hulu/0.1/validator-amp-vine.protoascii

+  attrs: {
+    name: "src"
+    mandatory: true
+    value_regex: "https://cdn\\.ampproject\\.org/v0/amp-hulu-(latest|0\\.1).js"


Please escape the last "." e.g. \\.js

honeybadgerdontcare · 2016-11-09T17:51:15Z

extensions/amp-hulu/0.1/validator-amp-vine.protoascii

+tags: {  # <amp-hulu>
+  html_format: AMP
+  html_format: AMP4ADS
+  tag_name: "AMP-VINE"


The tag_name should be AMP-HULU not AMP-VINE

honeybadgerdontcare · 2016-11-09T17:51:40Z

extensions/amp-hulu/0.1/validator-amp-vine.protoascii

+  tag_name: "AMP-VINE"
+  disallowed_ancestor: "AMP-SIDEBAR"
+  also_requires_tag: "amp-hulu extension .js script"
+  # data-* is generally allowed, but it's not generally mandatory.


No need for this comment here.

honeybadgerdontcare · 2016-11-09T17:52:33Z

extensions/amp-hulu/0.1/validator-amp-vine.protoascii

+  disallowed_ancestor: "AMP-SIDEBAR"
+  also_requires_tag: "amp-hulu extension .js script"
+  # data-* is generally allowed, but it's not generally mandatory.
+  attrs: { name: "data-eid" mandatory: true }


nit, please expand to multi-line.

attrs: {
name: "data-eid"
mandatory: true
}

honeybadgerdontcare · 2016-11-09T17:54:14Z

extensions/amp-hulu/0.1/validator-amp-vine.protoascii

+    supported_layouts: FIXED
+    supported_layouts: FIXED_HEIGHT
+    supported_layouts: FLEX_ITEM
+    supported_layouts: NODISPLAY


Somewhat surprised to see nodisplay. Is that intentional? Wouldn't a video player want to be seen?

honeybadgerdontcare · 2016-11-09T17:57:55Z

gulpfile.js

@@ -106,6 +106,7 @@ declareExtension('amp-viz-vega', '0.1', true);
 declareExtension('amp-google-vrview-image', '0.1', false, 'NO_TYPE_CHECK');
 declareExtension('amp-viewer-integration', '0.1', false);
 declareExtension('amp-youtube', '0.1', false);
+declareExtension('amp-hulu', '0.1', false, 'NO_TYPE_CHECK');


nit, please put this in alphabetical order (even though some are not, lets try to keep it consistent).

unbug · 2016-11-10T01:40:59Z

Thanks @honeybadgerdontcare , I was wordering why it can't pass the travis. I had no idea about the validator rules, so I just followed vine's file, and it failed, so I talked to @erwinmombay in slack, it's not ready to reviewed yet. You're very helping, I'll make it right and re-summit again :)

unbug · 2016-11-10T02:14:49Z

@aghassemi It's ready, thanks 👍

honeybadgerdontcare · 2016-11-10T03:49:02Z

@unbug validator rules look good. thanks!

aghassemi

Thanks @unbug. Two small requests and then we should be good to merge this.

aghassemi · 2016-11-10T16:33:54Z

extensions/amp-hulu/0.1/amp-hulu.js

+    iframe.src = 'https://secure.hulu.com/dash/mobile_embed.html?eid=' + encodeURIComponent(eid);
+    this.applyFillContent(iframe);
+    this.element.appendChild(iframe);
+    /** @private {?Element} */


remove the type annotation, it is redundant at this point.

aghassemi · 2016-11-10T16:37:30Z

gulpfile.js

@@ -72,6 +72,7 @@ declareExtension('amp-form', '0.1', true);
 declareExtension('amp-fresh', '0.1', true);
 declareExtension('amp-fx-flying-carpet', '0.1', true);
 declareExtension('amp-gfycat', '0.1', false);
+declareExtension('amp-hulu', '0.1', false, 'NO_TYPE_CHECK');


Please remove NO_TYPE_CHECK and allow the component to be type checked. Please run gulp check-types locally to ensure it passes type checking.

jridgewell · 2016-11-10T18:53:45Z

extensions/amp-hulu/0.1/amp-hulu.js

+
+  /** @override */
+  preconnectCallback() {
+    this.preconnect.url('https://secure.hulu.com/dash/mobile_embed.html');


Should this be Preconnect#preload? The path won't be useful for #url.

@unbug Signature of this method is preconnectCallback(onLayout) and onLayout needs to be passed to this.preconnect.url as the last arg.

@jridgewell I had the same question but apparently https://secure.hulu.com/dash/mobile_embed.html and https://secure.hulu.com/ return different Content-Security-Policy response headers. I don't know if preconnect logic still keeps the connections if Content-Security-Policy is not met.

Why not #preload then? That'll get the right security headers and we won't have this question again. 😁

@unbug let's do @jridgewell's suggestion for preload but you need to have the eid= appended to the Url before preloading. I suggest the following:

1- introduce a new instance variable this.eid_ in the cosntrcutor.
2- in buildCallback do the this.eid_ = user().assert(this.element.getAttribute('data-eid'), 'The data-eid attribute is required for <amp-hulu> %s', this.element); instead of in the layoutCallback as it does now.
3- Create a new getVideoIframeSrc_() which returns the Url with correct eid query string appended.
4- Use getVideoIframeSrc_() in preconnectCallback and layoutCallback

jridgewell · 2016-11-10T18:54:49Z

extensions/amp-hulu/0.1/amp-hulu.js

+  }
+
+  /** @override */
+  unlayoutOnPause() {


We need an #unlayoutCallback that removes the iframe! See https://github.com/ampproject/amphtml/blob/master/extensions/amp-brightcove/0.1/amp-brightcove.js#L122-L128.

unbug · 2016-11-11T03:17:35Z

@aghassemi @jridgewell Sorry for so much troubles, 👍 and thanks for your patience. I hope this time everything is ready :)

use es6 syntax use const instead of let added validator file remove size expect code reviewed some updated added amp-hulu reviewd validator code reviewed, remove NO_TYPE_CHECK, added getVideoIframeSrc_ syntax fixed

jridgewell

LGTM

jridgewell · 2016-11-11T16:36:36Z

extensions/amp-hulu/0.1/amp-hulu.js

+  /** @return {string} */
+  getVideoIframeSrc_() {
+    dev().assert(this.eid_);
+    return `https://secure.hulu.com/dash/mobile_embed.html?eid=${encodeURIComponent(this.eid_ || '')}`;


The || '' will never occur, since we've asserted #eid is truthy.

aghassemi · 2016-11-11T16:42:10Z

@unbug LGTM.
Please keep in mind to implement the video-interface as soon as you have the necessary postMessage hooks. Implementing the video-interface will automatically enable features such as AMP-managed autoplay for your player.

unbug · 2016-11-11T19:02:10Z

@aghassemi Thanks, my team already created a ticket forvideo-interface, we'll keep that in mind :)
Anyway when I'll see https://cdn.ampproject.org/v0/amp-hulu-0.1.js online?

unbug force-pushed the amp-hulu branch from f2b8bb6 to 7bbfdef Compare November 3, 2016 03:32

cramforce assigned aghassemi Nov 3, 2016

unbug force-pushed the amp-hulu branch from 3e2e655 to be64121 Compare November 3, 2016 04:39

aghassemi added NEEDS REVIEW labels Nov 3, 2016

jridgewell reviewed Nov 7, 2016

View reviewed changes

aghassemi suggested changes Nov 7, 2016

View reviewed changes

unbug force-pushed the amp-hulu branch from 644c542 to f6d14b8 Compare November 9, 2016 03:01

erwinmombay reviewed Nov 9, 2016

View reviewed changes

honeybadgerdontcare reviewed Nov 9, 2016

View reviewed changes

unbug force-pushed the amp-hulu branch from f6d14b8 to 9a4f9bf Compare November 10, 2016 01:47

aghassemi suggested changes Nov 10, 2016

View reviewed changes

jridgewell requested changes Nov 10, 2016

View reviewed changes

unbug force-pushed the amp-hulu branch from 9a4f9bf to 9ceb2db Compare November 11, 2016 03:13

create amp-hulu

40dc6a8

use es6 syntax use const instead of let added validator file remove size expect code reviewed some updated added amp-hulu reviewd validator code reviewed, remove NO_TYPE_CHECK, added getVideoIframeSrc_ syntax fixed

unbug force-pushed the amp-hulu branch from 9ceb2db to 40dc6a8 Compare November 11, 2016 03:20

jridgewell approved these changes Nov 11, 2016

View reviewed changes

aghassemi approved these changes Nov 11, 2016

View reviewed changes

aghassemi merged commit e4900fc into ampproject:master Nov 11, 2016

dreamofabear pushed a commit to dreamofabear/amphtml that referenced this pull request Nov 15, 2016

amp-hulu video player (ampproject#5993)

8977824

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

amp-hulu video player (ampproject#5993)

d960afc

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

amp-hulu video player (ampproject#5993)

598eedb

		import {user} from '../../../src/log';

		class AmpHulu extends AMP.BaseElement {

Add amp-hulu element for Hulu videos. #5993

Add amp-hulu element for Hulu videos. #5993

Conversation

unbug commented Nov 3, 2016 • edited

googlebot commented Nov 3, 2016

googlebot commented Nov 3, 2016

cramforce commented Nov 3, 2016

unbug commented Nov 3, 2016 • edited

aghassemi commented Nov 3, 2016

unbug commented Nov 4, 2016 • edited

aghassemi commented Nov 4, 2016

unbug commented Nov 6, 2016

Choose a reason for hiding this comment

aghassemi commented Nov 7, 2016

aghassemi 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

unbug commented Nov 8, 2016 • edited

aghassemi commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi commented Nov 9, 2016

honeybadgerdontcare Nov 9, 2016 • 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

unbug commented Nov 10, 2016

unbug commented Nov 10, 2016

honeybadgerdontcare commented Nov 10, 2016

aghassemi 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

aghassemi Nov 10, 2016 • 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

unbug commented Nov 11, 2016

jridgewell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi commented Nov 11, 2016

unbug commented Nov 11, 2016 • edited

unbug commented Nov 3, 2016 •

edited

unbug commented Nov 3, 2016 •

edited

unbug commented Nov 4, 2016 •

edited

unbug commented Nov 8, 2016 •

edited

honeybadgerdontcare Nov 9, 2016 •

edited

aghassemi Nov 10, 2016 •

edited

unbug commented Nov 11, 2016 •

edited