Nexxtv player extension#7816
Nexxtv player extension#7816erwinmombay merged 75 commits intoampproject:masterfrom 3qnexx:nexxtv-player
Conversation
…o nexxtv-player
|
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! corporation name: nexxtv GmbH |
aghassemi
left a comment
There was a problem hiding this comment.
Thanks for the PR. Please see the review comments below.
| src += `${encodeURIComponent(this.client_)}/`; | ||
| src += `${encodeURIComponent(this.mediaid_)}`; | ||
| src += `?start=${encodeURIComponent(this.start_)}`; | ||
| src += `&datamode=${encodeURIComponent(this.mode_)}&=1`; |
There was a problem hiding this comment.
Move this logic to a getVideoIframeSrc_ method that is called here and in preconnectCallback
| @@ -0,0 +1,219 @@ | |||
| /** | |||
| * Copyright 2016 The AMP HTML Authors. All Rights Reserved. | |||
| const iframe = this.element.ownerDocument.createElement('iframe'); | ||
| this.iframe_ = iframe; | ||
|
|
||
| this.forwardEvents([VideoEvents.PLAY, VideoEvents.PAUSE], iframe); |
There was a problem hiding this comment.
remove, not needed since the events are handled by message listener
|
|
||
| this.iframe_.src = src; | ||
|
|
||
| this.win.addEventListener('message', |
There was a problem hiding this comment.
Please use the helper util listen and assign the unlisten to an instance variable (e.g. this.unlistenMessage_) that can be later called in unlayoutCallback See below commect aboyt unlayoutCallback. listen helper can be imported from ../../../src/event-helper
|
|
||
| return this.loadPromise(this.iframe_) | ||
| .then(() => { | ||
| this.playerReadyPromise_; |
There was a problem hiding this comment.
the promise needs to be returned (swap the lines)
| # | ||
|
|
||
| tags: { # amp-nexxtv-player | ||
| html_format: AMP |
There was a problem hiding this comment.
/cc @honeybadgerdontcare for review of the validator file
| ``` | ||
|
|
||
| ## Attributes | ||
|
|
There was a problem hiding this comment.
add section for autoplay
**autoplay**
If this attribute is present, and the browser supports autoplay:
* the video is automatically muted before autoplay starts
* when the video is scrolled out of view, the video is paused
* when the video is scrolled into view, the video resumes playback
* when the user taps the video, the video is unmuted
* if the user has interacted with the video (e.g., mutes/unmutes, pauses/resumes, etc.), and the video is scrolled in or out of view, the state of the video remains as how the user left it. For example, if the user pauses the video, then scrolls the video out of view and returns to the video, the video is still paused.
| </tr> | ||
| <tr> | ||
| <td width="40%"><strong>Examples</strong></td> | ||
| <td><a href="https://ampbyexample.com/components/amp-youtube/">Annotated code example for amp-youtube</a></td> |
There was a problem hiding this comment.
this points to amp-youtube example. please remove the whole Example row. Later you can submit a PR to https://github.com/ampproject/amp-by-example to include your player there and update this file at that time
|
|
||
| ## Validation | ||
|
|
||
| See [amp-nexxtv-player rules](https://github.com/ampproject/amphtml/blob/master/extensions/amp-youtube/0.1/validator-amp-nexxtv-player.protoascii) in the AMP validator specification. |
There was a problem hiding this comment.
There was a problem hiding this comment.
This URL should be updated (note the amp-youtube in the path).
There was a problem hiding this comment.
Sorry please ignore this, you already updated the URL to not have amp-youtube.
| @@ -0,0 +1,127 @@ | |||
| /** | |||
There was a problem hiding this comment.
After you verify that autoplay works fine manually, please add your players to test-video-players.js and include the script in video-players.html and run gulp test --integration to run the test suite. These tests ensure the video-interface is implemented correctly by black-box testing it. This ensures methods such as play, pause, mute, etc.. actually work and the events actually propagate back from the iframed player.
| spec_name: "amp-nexxtv-player extension .js script" | ||
| satisfies: "amp-nexxtv-player extension .js script" | ||
| mandatory_parent: "HEAD" | ||
| unique_warning: true |
There was a problem hiding this comment.
Please make this unique: true. Warning is for older scripts that were grandfathered in before it was standard for this to be an error.
| error_message: "contents" | ||
| } | ||
| } | ||
| spec_url: "https://www.ampproject.org/docs/reference/extended/amp-nexxtv-player.html" |
There was a problem hiding this comment.
Please link to the md in your PR instead of ampproject since the ampproject link doesn't exist yet, but the md file will exist with the PR committed.
|
@aghassemi I've fixed gulp check-types, but build is still breaking with |
| name: "origin" | ||
| value_url: { | ||
| allow_empty: true | ||
| allowed_relative: true |
There was a problem hiding this comment.
@neko-fire The build isn't working because I said allowed_relative instead of allow_relative. Sorry about that. Just change line 76 to allow_relative: true
|
CLAs look good, thanks! |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
The build still breaks with the validator. Is there a way to test the validator faster for this extension locally or do I even need it? I've tried to create a build locally but it doesn't allow other OS than Linux/Mac. I'm running on Windows. |
|
Looking at the travis error it says the test html file is invalid. Go to https://travis-ci.org/ampproject/amphtml/builds/208564391 and scroll down to see the errors. I believe you can run |
|
Hi @honeybadgerdontcare. As I said I tried to run build.py I'm not sure what to do here. Is there any documentation of how to write a correct validator and make this run in Windows? |
|
You may want to try using amphtml-validation then. Windows support was added in v.1.0.15. |
|
For this specific error in Travis, here's what it is saying: So it was expecting only two errors in the html page but found many more. That either means the test file is incorrect or the protoascii is. |
|
@honeybadgerdontcare thanks a lot, I'll try with apmhtml-validation |
|
Hi, build went successfully through. Can we merge this PR? |
|
@neko-fire Merged. Thanks for contributing. |
| || 'https://embed.nexx.cloud/'; | ||
|
|
||
| let src = ''; | ||
| src += origin; |
There was a problem hiding this comment.
Eek! This has an XSS vulnerability (admittedly invalidated by the validation spec), and needs to have a runtime assertion. /to @alanorozco
| const origin = this.element.getAttribute('data-origin') | ||
| || 'https://embed.nexx.cloud/'; | ||
|
|
||
| let src = ''; |
There was a problem hiding this comment.
Nit, just initialize to origin.
| } | ||
|
|
||
| src += `${encodeURIComponent(client)}/`; | ||
| src += `${encodeURIComponent(mediaId)}`; |
There was a problem hiding this comment.
Nit: the template string is unnecessary.
|
|
||
| src += `${encodeURIComponent(client)}/`; | ||
| src += `${encodeURIComponent(mediaId)}`; | ||
| src += `?start=${encodeURIComponent(String(start))}`; |
There was a problem hiding this comment.
Nit: we can avoid the string cast by using '0' https://github.com/ampproject/amphtml/pull/7816/files#diff-785b3e6c303d2f26c285316e592444f9R100.
| iframe.setAttribute('frameborder', '0'); | ||
| iframe.setAttribute('allowfullscreen', 'true'); | ||
|
|
||
| this.element.appendChild(iframe); |
There was a problem hiding this comment.
Nit: this is a heavy operation for #buildCallback (it creates a Window context for the iframe). Let's move this into #layoutCallback.
|
|
||
| pauseCallback() { | ||
| if (this.iframe_) { | ||
| this.pause(); |
There was a problem hiding this comment.
Have we confirmed this works? If not, we need to add #unlayoutOnPause
| } | ||
|
|
||
| /** @override */ | ||
| unlayoutCallback() { |
There was a problem hiding this comment.
We need to "unresolve" playerReadyPromise_ be creating a new promise. We could probably just move it into #layoutCallback.
| } | ||
|
|
||
| sendCommand_(command) { | ||
| this.iframe_.contentWindow./*OK*/postMessage(JSON.stringify({ |
There was a problem hiding this comment.
This needs to test that iframe still has a contentWindow
| // VideoInterface Implementation | ||
| // only send in json format | ||
| play() { | ||
| this.playerReadyPromise_.then(() => { |
There was a problem hiding this comment.
Nit: if we move this into #sendCommand_, we can save a few bytes.
|
|
||
| if (data.cmd == 'onload') { | ||
| this.element.dispatchCustomEvent(VideoEvents.LOAD); | ||
| this.playerReadyResolver_(this.iframe_); |
There was a problem hiding this comment.
Bug: This will already be resolved by #layoutCallback. Are we supposed to wait until we receive the onload?
* initial setup for extension nexxtv-player * added nexxtv player logic * nexxtv player bugfixes, cleanup, tests, docs * added video interface, changed attribute start to seek-to * reworked video interface for nexxtv-player * added message handler for nexxtv player and tests * nexxtv player minor fixes & cleanup * nexxtv player changed indents from 4 spaces to 2 * added unlayoutCallback for nexxtv player * initial setup for extension nexxtv-player * added nexxtv player logic * nexxtv player bugfixes, cleanup, tests, docs * added video interface, changed attribute start to seek-to * reworked video interface for nexxtv-player * added message handler for nexxtv player and tests * nexxtv player minor fixes & cleanup * nexxtv player changed indents from 4 spaces to 2 * added unlayoutCallback for nexxtv player * fixed linting issues (ampproject#7673) * initial setup for extension nexxtv-player * added nexxtv player logic * nexxtv player bugfixes, cleanup, tests, docs * added video interface, changed attribute start to seek-to * reworked video interface for nexxtv-player * added message handler for nexxtv player and tests * nexxtv player minor fixes & cleanup * nexxtv player changed indents from 4 spaces to 2 * added unlayoutCallback for nexxtv player * initial setup for extension nexxtv-player * added nexxtv player logic * nexxtv player bugfixes, cleanup, tests, docs * added video interface, changed attribute start to seek-to * reworked video interface for nexxtv-player * added message handler for nexxtv player and tests * nexxtv player minor fixes & cleanup * fixed linting issues (ampproject#7673) * nexxtv player review fixes (ampproject#7816) * nexxtv player: updated validation rules (ampproject#7816) * nexxtv player validator ordered alphabetical, changed spec_url * fire amp-dom-update event on insert and replace (ampproject#7819) * temp * trigger `amp-dom-update` event on insert and replace * switch -amp-form to i-amp-form * Validator Rollup (ampproject#7844) * Revision bump. * Revision bump for amp-playbuzz changes in ampproject#7450 * Revision bump. * Allow filtering by HtmlFormat (AMP, AMP4ADS) in the code generator. * Revision bump. * Revision bump due to Github pull. * Code generation now driven by a variable LIGHT, which is broader than the previous GENERATE_DETAILED_ERRORS distinction. LIGHT implies filtered for specific format (AMP or AMP4ADS, and only amp.validator.validateSaxEvents, and no detailed errors. * Generate ValidatorRules.directAttrLists and globalAttrs and ampLayoutAttrs, reducing overhead by indirection. * Implement parallax effect extension (ampproject#7794) * Do not set hidden attribute on hide/collapse (ampproject#7879) * amp-bind: Support `Math` functions (ampproject#7797) * initial commit for Math in amp-bind * fix lint * ali's comment * avoid scope conflicts with built-in functions * remove Math functions from documentation * Measure 3P ad latency (ampproject#7902) * Update spec URLs from github to ampproject. (ampproject#7901) * Update github urls to ampproject urls * test .out updated * Too many slashes. * Moved Developing.md and Design_Principles.md (ampproject#7908) * Rename DEVELOPING.md to contributing/DEVELOPING.md * Move files to new folder * Renaming toggle to toggleVisibility because it was conflicting with sidebar and breaking it (ampproject#7855) * disable scrollRestoration auto for embed case (ampproject#7899) * Disable Fast Fetch for all ads when remote.html is used. (ampproject#7906) * Fix DoubleClick Fast Fetch bugs around categoryExclusions and tagForChildDirectedTreatment (ampproject#7843) * Fix incorrect parameter name for `tagForChildDirectedTreatment` in DoubleClick. * Added unit test for tagForChildDirectedTreatment. * Fixed the categoryExclusions bug in Fast Fetch DoubleClick. * Removed parens in arrow functions to satisfy linter * Adds two new experiment IDs (ampproject#7850) * Adds two new experiment IDs to distinguish "any externally triggered" experiment from "any internally triggered". Also updates tests to remove a number of assumptions that only a single eid will be populated. * Use return val to detect "externally selected"; test updates. * Updated network implementation guide. (ampproject#7862) * Updated network impl guide. * Changed 'validation' to 'verification' * Cid timeout error should not be dev error (ampproject#7911) * Fix amp-ad test (ampproject#7918) * Update amp-install-serviceworker.md (ampproject#7932) Fully escape javascript regex. * Update amp-cache-modifications.md (ampproject#7933) Add `data-no-service-worker-fallback-shell-url` as a URL to be rewritten as absolute. * amp-fx-parallax Don't use global (ampproject#7942) * Viewer integration: rewrite error message to indicate request name (ampproject#7923) * rewrite error message * update * ticks * Fix up links in DEVELOPING.md (ampproject#7944) We moved DEVELOPING.md to the contributing folder but didn't update many of the relative links accordingly. * Popin ad extension document updated. (ampproject#7674) * Add popin ad extension. * register popin. * Add resizeable attribute. * Remove resizable. * Implemented the render-start and no-content APIs. * Fixed lint. * Remove quatation. * Use tei@popin.cc * Rebase old commit . * Fixed document because tag was not closed. * I replaced the removed double quotes. * Add double quotes. * Support for bind expressions in mustache templates (ampproject#7602) * first pass at adding/removing subtrees * tests passing * Add ability to add and remove bindings in the subtree rooted at a specific node * lint fixes * merge conflict * ci issues * some cleanup * pr comments * pr comments, new method of removing bindings for node and its children * fix some lint warnings * test and lint fixes * mutation observer * update comments * add observer * naive implementation * cleanup * clean * cleanup bind impl * very simple example of bind in a template * pr comments * lint errors * pr comments * cleanup * lint errors * pr comments * pr comments and lint warnings * typo * comments * more lint warnings * pr comment * change README * cleanup * pr comments * pr comments 2 * lint warnings * don't use foreach on Nodelist * casting * more ci warnings * even more ci warnings * even more ci warnings * even more ci issues * even more ci comments * even more ci issues * pr comments * more pr comments * fix refactoring oversight * pr comments * pr comments * merge * pr comments * ci issues * pr comments * assertElement * pr comments * typo * updating sanitizer * fixing up sanitizer test * pr comments * pr comments * expanded on amp-mustache tests * pr comments * remove class from whitelist * restore whole sandbox * update mustache validator test and output * pr comments * nexxtv player gulp check-types fixes (ampproject#7816) * nexxtv player fix error gulp presumbit with postmessage (ampproject#7816) * added dependency check for nexxtv player (ampproject#7816) * nexxtv player minor fix in validator (ampproject#7816) * nexxtv player fixed validator (ampproject#7816) * fixed validator (ampproject#7816)
| * playlist-masked: playlist without possibility to skip or choose video | ||
| * album: is an audio playlist | ||
|
|
||
| **seek-to** |
There was a problem hiding this comment.
should "seek-to" be "start" like in the example above? Or should the example be corrected to "seek-to"?
Also, should the example be: "For example, the value 90 starts the video at 1:30min."
Just let me know as I'm cleaning up this documentation file.
There was a problem hiding this comment.
data-seek-to is correct, example is incorrect.
|
Hi, ampproject bot here! Here are a list of the owners that can approve your files. You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status /to cramforce dvoytenko jridgewell
/to lannka zhouyx
/to cramforce erwinmombay
/to choumx kmh287
For any issues please file a bug at https://github.com/google/github-owners-bot/issues |
* initial setup for extension nexxtv-player * added nexxtv player logic * nexxtv player bugfixes, cleanup, tests, docs * added video interface, changed attribute start to seek-to * reworked video interface for nexxtv-player * added message handler for nexxtv player and tests * nexxtv player minor fixes & cleanup * nexxtv player changed indents from 4 spaces to 2 * added unlayoutCallback for nexxtv player * initial setup for extension nexxtv-player * added nexxtv player logic * nexxtv player bugfixes, cleanup, tests, docs * added video interface, changed attribute start to seek-to * reworked video interface for nexxtv-player * added message handler for nexxtv player and tests * nexxtv player minor fixes & cleanup * nexxtv player changed indents from 4 spaces to 2 * added unlayoutCallback for nexxtv player * fixed linting issues (ampproject#7673) * initial setup for extension nexxtv-player * added nexxtv player logic * nexxtv player bugfixes, cleanup, tests, docs * added video interface, changed attribute start to seek-to * reworked video interface for nexxtv-player * added message handler for nexxtv player and tests * nexxtv player minor fixes & cleanup * nexxtv player changed indents from 4 spaces to 2 * added unlayoutCallback for nexxtv player * initial setup for extension nexxtv-player * added nexxtv player logic * nexxtv player bugfixes, cleanup, tests, docs * added video interface, changed attribute start to seek-to * reworked video interface for nexxtv-player * added message handler for nexxtv player and tests * nexxtv player minor fixes & cleanup * fixed linting issues (ampproject#7673) * nexxtv player review fixes (ampproject#7816) * nexxtv player: updated validation rules (ampproject#7816) * nexxtv player validator ordered alphabetical, changed spec_url * fire amp-dom-update event on insert and replace (ampproject#7819) * temp * trigger `amp-dom-update` event on insert and replace * switch -amp-form to i-amp-form * Validator Rollup (ampproject#7844) * Revision bump. * Revision bump for amp-playbuzz changes in ampproject#7450 * Revision bump. * Allow filtering by HtmlFormat (AMP, AMP4ADS) in the code generator. * Revision bump. * Revision bump due to Github pull. * Code generation now driven by a variable LIGHT, which is broader than the previous GENERATE_DETAILED_ERRORS distinction. LIGHT implies filtered for specific format (AMP or AMP4ADS, and only amp.validator.validateSaxEvents, and no detailed errors. * Generate ValidatorRules.directAttrLists and globalAttrs and ampLayoutAttrs, reducing overhead by indirection. * Implement parallax effect extension (ampproject#7794) * Do not set hidden attribute on hide/collapse (ampproject#7879) * amp-bind: Support `Math` functions (ampproject#7797) * initial commit for Math in amp-bind * fix lint * ali's comment * avoid scope conflicts with built-in functions * remove Math functions from documentation * Measure 3P ad latency (ampproject#7902) * Update spec URLs from github to ampproject. (ampproject#7901) * Update github urls to ampproject urls * test .out updated * Too many slashes. * Moved Developing.md and Design_Principles.md (ampproject#7908) * Rename DEVELOPING.md to contributing/DEVELOPING.md * Move files to new folder * Renaming toggle to toggleVisibility because it was conflicting with sidebar and breaking it (ampproject#7855) * disable scrollRestoration auto for embed case (ampproject#7899) * Disable Fast Fetch for all ads when remote.html is used. (ampproject#7906) * Fix DoubleClick Fast Fetch bugs around categoryExclusions and tagForChildDirectedTreatment (ampproject#7843) * Fix incorrect parameter name for `tagForChildDirectedTreatment` in DoubleClick. * Added unit test for tagForChildDirectedTreatment. * Fixed the categoryExclusions bug in Fast Fetch DoubleClick. * Removed parens in arrow functions to satisfy linter * Adds two new experiment IDs (ampproject#7850) * Adds two new experiment IDs to distinguish "any externally triggered" experiment from "any internally triggered". Also updates tests to remove a number of assumptions that only a single eid will be populated. * Use return val to detect "externally selected"; test updates. * Updated network implementation guide. (ampproject#7862) * Updated network impl guide. * Changed 'validation' to 'verification' * Cid timeout error should not be dev error (ampproject#7911) * Fix amp-ad test (ampproject#7918) * Update amp-install-serviceworker.md (ampproject#7932) Fully escape javascript regex. * Update amp-cache-modifications.md (ampproject#7933) Add `data-no-service-worker-fallback-shell-url` as a URL to be rewritten as absolute. * amp-fx-parallax Don't use global (ampproject#7942) * Viewer integration: rewrite error message to indicate request name (ampproject#7923) * rewrite error message * update * ticks * Fix up links in DEVELOPING.md (ampproject#7944) We moved DEVELOPING.md to the contributing folder but didn't update many of the relative links accordingly. * Popin ad extension document updated. (ampproject#7674) * Add popin ad extension. * register popin. * Add resizeable attribute. * Remove resizable. * Implemented the render-start and no-content APIs. * Fixed lint. * Remove quatation. * Use tei@popin.cc * Rebase old commit . * Fixed document because tag was not closed. * I replaced the removed double quotes. * Add double quotes. * Support for bind expressions in mustache templates (ampproject#7602) * first pass at adding/removing subtrees * tests passing * Add ability to add and remove bindings in the subtree rooted at a specific node * lint fixes * merge conflict * ci issues * some cleanup * pr comments * pr comments, new method of removing bindings for node and its children * fix some lint warnings * test and lint fixes * mutation observer * update comments * add observer * naive implementation * cleanup * clean * cleanup bind impl * very simple example of bind in a template * pr comments * lint errors * pr comments * cleanup * lint errors * pr comments * pr comments and lint warnings * typo * comments * more lint warnings * pr comment * change README * cleanup * pr comments * pr comments 2 * lint warnings * don't use foreach on Nodelist * casting * more ci warnings * even more ci warnings * even more ci warnings * even more ci issues * even more ci comments * even more ci issues * pr comments * more pr comments * fix refactoring oversight * pr comments * pr comments * merge * pr comments * ci issues * pr comments * assertElement * pr comments * typo * updating sanitizer * fixing up sanitizer test * pr comments * pr comments * expanded on amp-mustache tests * pr comments * remove class from whitelist * restore whole sandbox * update mustache validator test and output * pr comments * nexxtv player gulp check-types fixes (ampproject#7816) * nexxtv player fix error gulp presumbit with postmessage (ampproject#7816) * added dependency check for nexxtv player (ampproject#7816) * nexxtv player minor fix in validator (ampproject#7816) * nexxtv player fixed validator (ampproject#7816) * fixed validator (ampproject#7816)
NexxTV player extension
Displays a media stream from nexxOMNIA platform.
amp-nexxtv-player.md