<amp-video-iframe> ✨ #16088

alanorozco · 2018-06-15T22:33:59Z

Ready for review.

Includes integration library (/cc @erwinmombay for serving).

Validation rules coming later per experimental support.

Autoplay

… amp-video-iframe

…-iframe

cvializ · 2018-06-22T18:47:45Z

extensions/amp-video-iframe/amp-video-iframe.md

+  var myVideo = document.querySelector('#my-video');
+
+  // For video.js:
+  myIntegration.listenTo('videojs', myVideo);


What is the difference in terms of effort between these "simple integrations" and a full video component?

cvializ · 2018-06-25T23:24:04Z

extensions/amp-video-iframe/amp-video-iframe.md

+The document living inside your iframe must include a small library:
+
+```js
+<script src="https://cdn.ampproject.org/v0/amp-video-iframe-integration-0.1.js" defer>


Why defer, instead of the approach with async and global object with a push property? The shadowdoc API does this

amphtml/spec/amp-shadow-doc.md

Lines 30 to 37 in 5446958

<script async src="https://cdn.ampproject.org/shadow-v0.js"></script>



<script>

(window.AMP = window.AMP || []).push(function(AMP) {

// AMP APIs can be used now via "AMP" object.

});

</script>

e.g.

(window.AmpVideoIntegration = window.AmpVideoIntegration || []).push(onAmpIntegrationReady); function onAmpIntegrationReady(myIntegration) { }

That way boilerplate is reduced and the publisher doesn't need to listen for the event or to construct a new instance.

I like this approach a lot better!

…-iframe

codecov-io · 2018-06-29T04:44:50Z

Codecov Report

Merging #16088 into master will increase coverage by 0.89%.
The diff coverage is 76.51%.

@@            Coverage Diff             @@
##           master   #16088      +/-   ##
==========================================
+ Coverage   77.14%   78.04%   +0.89%     
==========================================
  Files         547      551       +4     
  Lines       39934    40280     +346     
==========================================
+ Hits        30807    31436     +629     
+ Misses       9127     8844     -283

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fefe7f...2cec4fa. Read the comment docs.

…-iframe

aghassemi · 2018-07-02T20:15:37Z

build-system/dep-check-config.js

@@ -37,6 +37,14 @@ let RuleConfigDef;
 // this in review.
 exports.rules = [
  // Global rules
+  {


great preemptive thinking.

aghassemi · 2018-07-02T20:17:42Z

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

+  postIntersection_(messageId) {
+    const {time, intersectionRatio} = this.element.getIntersectionChangeEntry();
+
+    // Only post ratio > 0 when in autoplay range to prevent internal autoplay


good compromise solution.

cvializ · 2018-07-02T22:39:55Z

extensions/amp-video-iframe/0.1/test/test-amp-video-iframe.js

+      `http://iframe.localhost:${port}/test/fixtures/served/video-iframe.html`);
+  }
+
+  function createVideoIframe(size = null) {


Does the null default value add anything? size is undefined by default, which is falsy, so the default value null looks redundant when there is only a falsy check.

Thanks! There are a few other places in this PR with the same pattern, but it's not super important. To me, it adds a bit of overhead when a reader has to evaluate whether the = null default value is significant or not.

Could you point to some other examples where the default value is extraneous, or the optional parameter pattern is followed?

Sure, these were part of this PR and it looks like undefined and null would give the same behavior in these situations, which makes the = null unnecessary:

amphtml/src/iframe-video.js

Lines 77 to 88 in 6ec9b05

export function createFrameFor(video, src, name = null, sandbox = null) {

const {element} = video;

const frame =

htmlFor(element)`<iframe frameborder=0 allowfullscreen></iframe>`;

if (name) {

frame.setAttribute('name', name);

}

if (sandbox) {

frame.setAttribute('sandbox', sandbox.join(' '));

}

amphtml/src/video-iframe-integration.js

Lines 309 to 322 in 6ec9b05

postToParent_(data, optCallback = null) {

const id = this.callCounter_++;

const completeData = Object.assign({id}, data);

if (!getMode(this.win_).test && this.win_.parent) {

this.win_.parent./*OK*/postMessage(completeData, '*');

}

if (optCallback) {

this.callbacks_[id] = optCallback;

}

return id;

}

Thanks! #16563

AmyLiversidge · 2018-11-28T13:19:37Z

Hi @alanorozco , i see is merged to master but it's not released yet? Could you clarify your release process please and when we can expect this to be live?

alanorozco · 2018-11-28T22:45:51Z

Hi @AmyLiversidge

The extension is ongoing validator release, see #19504 for updates.

If everything goes well you should be able to use this in production within the next couple of weeks.

AmyLiversidge · 2018-11-29T13:20:42Z

@alanorozco , great news, thanks for letting me know.

<amp-video-iframe>

fa46733

googlebot added the cla: yes label Jun 15, 2018

alanorozco and others added 6 commits June 18, 2018 09:59

Lint

c2fab95

Allow same origin

dc03579

Dep-check

dd47b50

Oops

5a9122d

Dupe decl

b43d1f9

Merge branch 'amp-video-iframe' of github.com:alanorozco/amphtml into…

7373825

… amp-video-iframe

alanorozco mentioned this pull request Jun 21, 2018

Intent To Implement: amp-eplayer component #15795

Closed

alanorozco added 6 commits June 21, 2018 09:26

Types

459d021

Tests

8364b81

Naming

c990b45

Basic library docs

38029ae

Merge branch 'master' of github.com:ampproject/amphtml into amp-video…

64378cf

…-iframe

Syntax in docs

9b53a93

alanorozco force-pushed the amp-video-iframe branch from 7dc5001 to 9b53a93 Compare June 21, 2018 19:42

alanorozco added 2 commits June 21, 2018 12:45

Simplify to 'canplay'

d1c995d

Defer loaded note

e9ca8de

alanorozco changed the title ~~(do not merge) <amp-video-iframe> ✨~~ <amp-video-iframe> ✨ Jun 25, 2018

alanorozco requested a review from aghassemi June 25, 2018 23:14

alanorozco assigned aghassemi Jun 25, 2018

cvializ reviewed Jun 25, 2018

View reviewed changes

alanorozco added 3 commits June 25, 2018 23:35

Import scheme

aebb207

Docs

9d4a58a

Basic tests

46980e1

alanorozco force-pushed the amp-video-iframe branch from b175ab5 to 46980e1 Compare June 27, 2018 23:35

alanorozco added 4 commits June 27, 2018 16:38

Merge branch 'master' of github.com:ampproject/amphtml into amp-video…

599c7be

…-iframe

Oops

08d8378

Clarify

fe0f196

Don't mention Video.js until integration is ready

1ea6651

alanorozco added 4 commits June 28, 2018 19:57

Style, dep check

233bae0

getMode().test checks

476f455

Oops

0a75cce

Format, typo

26514a0

alanorozco force-pushed the amp-video-iframe branch from 7b89c43 to f8a4331 Compare June 29, 2018 03:57

Fixes

23d0a86

alanorozco force-pushed the amp-video-iframe branch from f8a4331 to 23d0a86 Compare June 29, 2018 03:58

alanorozco added 2 commits June 28, 2018 21:14

Fix amp-iframe test

e1c545c

Test new intersection ratio scheme

c0d878a

alanorozco force-pushed the amp-video-iframe branch from 00af2a4 to c0d878a Compare June 29, 2018 04:24

Make entry point slightly smaller

ff54dcb

alanorozco added 3 commits June 29, 2018 07:51

GetMetadata()

62e8b50

Safe play/pause

c513fa3

Increase test coverage

f404457

alanorozco force-pushed the amp-video-iframe branch from ad29a38 to f404457 Compare June 29, 2018 17:00

alanorozco added 7 commits June 29, 2018 10:02

Merge branch 'master' of github.com:ampproject/amphtml into amp-video…

37936ef

…-iframe

Doc getMetadata

bb4ea2d

Increase test coverage

9bdbf30

Bump bundle size

3692740

Increase test coverage

45d0266

Remove generator

93f4735

<3 bunde-size 4e

2cec4fa

aghassemi approved these changes Jul 2, 2018

View reviewed changes

cvializ reviewed Jul 2, 2018

View reviewed changes

alanorozco merged commit 085e3c4 into ampproject:master Jul 3, 2018

alanorozco deleted the amp-video-iframe branch November 28, 2018 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

<amp-video-iframe> ✨ #16088

<amp-video-iframe> ✨ #16088

alanorozco commented Jun 15, 2018 •

edited

Loading

cvializ Jun 22, 2018 •

edited

Loading

cvializ Jun 25, 2018

alanorozco Jun 25, 2018

codecov-io commented Jun 29, 2018 •

edited

Loading

aghassemi Jul 2, 2018

aghassemi Jul 2, 2018

cvializ Jul 2, 2018

alanorozco Jul 3, 2018

cvializ Jul 3, 2018

alanorozco Jul 3, 2018

cvializ Jul 3, 2018 •

edited

Loading

alanorozco Jul 3, 2018

AmyLiversidge commented Nov 28, 2018

alanorozco commented Nov 28, 2018

AmyLiversidge commented Nov 29, 2018

	<script async src="https://cdn.ampproject.org/shadow-v0.js"></script>

	<!-- Wait for API to initialize -->
	<script>
	(window.AMP = window.AMP \|\| []).push(function(AMP) {
	// AMP APIs can be used now via "AMP" object.
	});
	</script>

	export function createFrameFor(video, src, name = null, sandbox = null) {
	const {element} = video;
	const frame =
	htmlFor(element)`<iframe frameborder=0 allowfullscreen></iframe>`;

	if (name) {
	frame.setAttribute('name', name);
	}

	if (sandbox) {
	frame.setAttribute('sandbox', sandbox.join(' '));
	}

	postToParent_(data, optCallback = null) {
	const id = this.callCounter_++;
	const completeData = Object.assign({id}, data);

	if (!getMode(this.win_).test && this.win_.parent) {
	this.win_.parent./OK/postMessage(completeData, '*');
	}

	if (optCallback) {
	this.callbacks_[id] = optCallback;
	}
	return id;
	}

<amp-video-iframe> ✨ #16088

<amp-video-iframe> ✨ #16088

Conversation

alanorozco commented Jun 15, 2018 • edited Loading

Ready for review.

Autoplay

cvializ Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 29, 2018 • edited Loading

Codecov Report

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

cvializ Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmyLiversidge commented Nov 28, 2018

alanorozco commented Nov 28, 2018

AmyLiversidge commented Nov 29, 2018

alanorozco commented Jun 15, 2018 •

edited

Loading

cvializ Jun 22, 2018 •

edited

Loading

codecov-io commented Jun 29, 2018 •

edited

Loading

cvializ Jul 3, 2018 •

edited

Loading