Powr player component by chadladensack · Pull Request #19043 · ampproject/amphtml

chadladensack · 2018-10-30T13:17:50Z

This is an amp-powr-player component to implement a Powr player as outlined in #18960.

alanorozco · 2018-10-30T19:12:16Z

Hi @chadladensack. Thanks for opening your PR again. I see you're still making changes so please ping me here once this is good to go!

Gregable · 2018-10-30T20:11:50Z

Validator changes look good.

chadladensack · 2018-10-30T20:41:07Z

Hey @alanorozco, when I opened this new PR, the travis build was failing due to a node version update that existed in the master branch. The only difference between this PR and the original was the master merge in 128ebc4.

We don't anticipate any modifications to the PR. If you notice anything during the review, let me know and I can jump on them right away.

alanorozco

Thanks! Looks good, only a few change requests.

alanorozco · 2018-10-30T21:39:31Z

extensions/amp-powr-player/0.1/amp-powr-player.js

+    this.muted_ = false;
+
+    /** @private {?boolean}  */
+    this.hasAmpSupport_ = false;


This one took me a second to figure out :) Consider renaming to frameHasAmpSupport_.

I guess the amp-powr-player component would always have AMP support lol. +1 for the rename.

alanorozco · 2018-10-30T21:40:24Z

extensions/amp-powr-player/0.1/amp-powr-player.js

+      this.playing_ = false;
+    }
+
+    if (redispatch(element, eventType, {


nit: define as top-level constant so the dictionary doesn't get recreated each time a message gets received.

alanorozco · 2018-10-30T21:43:08Z

extensions/amp-powr-player/0.1/amp-powr-player.js

+        '<amp-powr-player> %s',
+        el);
+
+    const src = 'https://player.powr.com/iframe.html?' +


Nit: use addParamsToUrl:

const srcPrefix = 'https://player.powr.com/iframe.html'; const srcParams = dict({ 'account': account, 'player': this.playerId_, }); if (video) { srcParams['video'] = video; } if (terms) { srcParams['terms'] = terms; } const src = addParamsToUrl(srcPrefix, srcParams);

Solid idea. Makes it a lot easier to follow.

alanorozco · 2018-10-31T23:56:10Z

Thanks for your changes! Looks pretty solid to me.

/to @aghassemi for a second pair of eyes and we should be good to go!

aghassemi

Looks very close to finish line. A few requests. Thanks!

aghassemi · 2018-11-01T16:42:51Z

extensions/amp-powr-player/0.1/amp-powr-player.js

@@ -0,0 +1,446 @@
+/**
+ * Copyright 2015 The AMP HTML Authors. All Rights Reserved.


aghassemi · 2018-11-01T16:46:39Z

extensions/amp-powr-player/0.1/amp-powr-player.js

+        ' Ensure it has the iframe plugin.', this.playerId_);
+    }, 3000);
+
+    this.playerReadyResolver_(this.iframe_);


This is resolving the ready promise too early. You wanna resolve inside your onReady. Maybe this was meant to be inside the timer above?

Good catch. Moved the resolve into onReady_.

aghassemi · 2018-11-01T16:48:14Z

extensions/amp-powr-player/0.1/amp-powr-player.js

+    this.playerReadyResolver_ = deferred.resolve;
+
+    // Warn if the player does not have video interface support
+    this.readyTimeout_ = Services.timerFor(window).delay(() => {


timer should start after the iframe is loaded. I think <1000ms between iframe loaded and readyMessage received should be enough to detect whether player is supporting the interface or not.

This readyTimeout_ was meant to prompt the publisher to configure their powr player to use the required iframe plugin.

We recently modified the amp-powr-player iframe src ( https://player.powr.com/iframe.html) to automatically include the player iframe plugin, rendering readyTimeout_ obsolete. I'll be removing it shortly.

aghassemi · 2018-11-01T16:50:53Z

extensions/amp-powr-player/0.1/test/test-amp-powr-player.js

@@ -0,0 +1,233 @@
+/**
+ * Copyright 2015 The AMP HTML Authors. All Rights Reserved.


all dates to 2018

aghassemi · 2018-11-01T16:51:36Z

extensions/amp-powr-player/validator-amp-powr-player.protoascii

@@ -0,0 +1,62 @@
+#


@honeybadgerdontcare

aghassemi · 2018-11-01T16:52:13Z

extensions/amp-powr-player/validator-amp-powr-player.protoascii

+    name: "data-terms"
+    mandatory_oneof: "['data-video', 'data-terms']"
+  }
+  attrs: { name: "[data-referrer]" }


no need to list data- unless mandatory or wanna enforce pattern for the value.

This one would be necessary since it's an amp-bind attribute and not just a data- attribute value.

Just to make sure we're all on the same page, I will leave data-referrer in the validation since it's an amp-bind attribute.

honeybadgerdontcare

validation looks good with some change requests.

honeybadgerdontcare · 2018-11-01T17:40:58Z

extensions/amp-powr-player/validator-amp-powr-player.protoascii

@@ -0,0 +1,62 @@
+#
+# Copyright 2016 The AMP HTML Authors. All Rights Reserved.


Please make sure years in the licenses are the current year. In this case it should be 2018 not 2016.

honeybadgerdontcare · 2018-11-01T17:41:37Z

extensions/amp-powr-player/validator-amp-powr-player.protoascii

+    mandatory_oneof: "['data-video', 'data-terms']"
+    value_regex: "[0-9a-zA-Z-]+"
+  }
+  attrs: {


nit: please list attributes alphabetically by name, IOW data-terms should go before data-video.

honeybadgerdontcare · 2018-11-01T17:42:20Z

extensions/amp-powr-player/validator-amp-powr-player.protoascii

+    name: "data-terms"
+    mandatory_oneof: "['data-video', 'data-terms']"
+  }
+  attrs: { name: "[data-referrer]" }


This one would be necessary since it's an amp-bind attribute and not just a data- attribute value.

…ssing the external resource.

aghassemi

LGTM

* cl/219531121 Revision bump for #17481 * cl/219850294 Allow `http` scheme for links in email spec * cl/219866890 Revision bump for #19043 * cl/219867113 Revision bump for #18981 * cl/219877087 Make ESlint happy. * cl/219882876 Fix lint for #19096

✨

* cl/219531121 Revision bump for ampproject#17481 * cl/219850294 Allow `http` scheme for links in email spec * cl/219866890 Revision bump for ampproject#19043 * cl/219867113 Revision bump for ampproject#18981 * cl/219877087 Make ESlint happy. * cl/219882876 Fix lint for ampproject#19096

Added powr-player component

6128dfc

googlebot added the cla: yes label Oct 30, 2018

chadladensack mentioned this pull request Oct 30, 2018

Powr player component #18977

Closed

Merge branch 'master' into powr-player

128ebc4

chadladensack force-pushed the powr-player branch from 7705f24 to 128ebc4 Compare October 30, 2018 18:28

alanorozco requested changes Oct 30, 2018

View reviewed changes

chadladensack added 3 commits October 31, 2018 14:35

Update powr-player for ease of maintenance.

10e6295

Fix JSDoc type

1218300

Merge branch 'master' into powr-player

7e2af51

alanorozco approved these changes Oct 31, 2018

View reviewed changes

aghassemi suggested changes Nov 1, 2018

View reviewed changes

honeybadgerdontcare approved these changes Nov 1, 2018

View reviewed changes

chadladensack added 2 commits November 1, 2018 14:55

Update amp-powr-player after peer review.

9a88c4c

Resolve the player ready state since the test isnt fetching and proce…

ad86ac6

…ssing the external resource.

aghassemi approved these changes Nov 1, 2018

View reviewed changes

aghassemi merged commit d605aff into ampproject:master Nov 2, 2018

twifkak added a commit to twifkak/amphtml that referenced this pull request Nov 2, 2018

cl/219866890 Revision bump for ampproject#19043

67d9b34

twifkak added a commit to twifkak/amphtml that referenced this pull request Nov 2, 2018

cl/219866890 Revision bump for ampproject#19043

9e91ae0

twifkak mentioned this pull request Nov 2, 2018

Validator rollup #19120

Merged

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Nov 14, 2018

Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018

✨ amp-powr-player component (ampproject#19043)

49b295c

✨

		@@ -0,0 +1,446 @@
		/**
		* Copyright 2015 The AMP HTML Authors. All Rights Reserved.

		@@ -0,0 +1,233 @@
		/**
		* Copyright 2015 The AMP HTML Authors. All Rights Reserved.

		@@ -0,0 +1,62 @@
		#
		# Copyright 2016 The AMP HTML Authors. All Rights Reserved.

Conversation

chadladensack commented Oct 30, 2018

Uh oh!

alanorozco commented Oct 30, 2018

Uh oh!

Gregable commented Oct 30, 2018

Uh oh!

chadladensack commented Oct 30, 2018

Uh oh!

alanorozco 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!

alanorozco commented Oct 31, 2018

Uh oh!

aghassemi 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!

Choose a reason for hiding this comment

Uh oh!

honeybadgerdontcare 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!

aghassemi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants