Extension amp-3q-player added #7999

JuliusThms · 2017-03-06T19:22:22Z

Extension amp-3q-player added for embedding Video from 3qsdn

googlebot · 2017-03-06T19:22:25Z

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!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

JuliusThms · 2017-03-06T19:25:46Z

I signed it!

googlebot · 2017-03-06T19:25:49Z

CLAs look good, thanks!

JuliusThms · 2017-03-07T11:49:33Z

Is there someone who can help with the validator? It's not passing the validation at the end.
@honeybadgerdontcare It would be a great pleasure to me to have your support

honeybadgerdontcare · 2017-03-08T19:28:33Z

@JuliusThms Thanks for contributing!

It looks like there may be some filenaming changes that need to be made and I'll point out some things for validation. The filenames should reflect the name of your component which is 3q-player so for instance in the examples folder it should be 3q-player.amp.html.

The validator issue can be found at the bottom of the travis build:
https://travis-ci.org/ampproject/amphtml/builds/208554798

The issue is that the .out file has no contents. It should have the results of if the .html file passes or fails and if fails what are the errors. So if there were no errors then the .out file should have one line saying PASS.

Failures:
1) ValidatorFeatures amp-3q-player/0.1/test/validator-amp-3q.html
  Message:
    AssertionError: 
    amp-3q-player/0.1/test/validator-amp-3q.out:1:0
    expected:
    
    saw:
    FAIL
    amp-3q-player/0.1/test/validator-amp-3q.html:41:4 The mandatory attribute 'data-id' is missing in tag 'amp-3q-player'. (see https://www.ampproject.org/docs/reference/extended/amp-3q-player.html) [AMP_TAG_PROBLEM]
  Stack:
    AssertionError: 
    amp-3q-player/0.1/test/validator-amp-3q.out:1:0
    expected:
    
    saw:
    FAIL
    amp-3q-player/0.1/test/validator-amp-3q.html:41:4 The mandatory attribute 'data-id' is missing in tag 'amp-3q-player'. (see https://www.ampproject.org/docs/reference/extended/amp-3q-player.html) [AMP_TAG_PROBLEM]
        at ValidatorTestCase.run (/home/travis/build/ampproject/amphtml/validator/dist/validator_test_minified.js:902:398)
        at Object.<anonymous> (/home/travis/build/ampproject/amphtml/validator/dist/validator_test_minified.js:904:248)
2) ValidatorFeatures amp-3q-player/0.1/test/validator-amp-3q.html
  Message:
    AssertionError: 
    amp-3q-player/0.1/test/validator-amp-3q.out:1:0
    expected:
    
    saw:
    FAIL
  Stack:
    AssertionError: 
    amp-3q-player/0.1/test/validator-amp-3q.out:1:0
    expected:
    
    saw:
    FAIL
        at ValidatorTestCase.run (/home/travis/build/ampproject/amphtml/validator/dist/validator-light_test_minified.js:797:486)
        at Object.<anonymous> (/home/travis/build/ampproject/amphtml/validator/dist/validator-light_test_minified.js:798:323)
4201 specs, 2 failures

honeybadgerdontcare · 2017-03-08T19:29:07Z

examples/3q.amp.html

@@ -0,0 +1,23 @@
+<!doctype html>


Please rename to 3q-player.amp.html.

Please rename.

honeybadgerdontcare · 2017-03-08T19:29:47Z

extensions/amp-3q-player/0.1/test/test-amp-3q.js

@@ -0,0 +1,54 @@
+/**


Please rename to test-amp-3q-player.js.

honeybadgerdontcare · 2017-03-08T19:29:58Z

extensions/amp-3q-player/0.1/test/test-amp-3q.js

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


s/2016/2017

honeybadgerdontcare · 2017-03-08T19:30:08Z

extensions/amp-3q-player/0.1/amp-3q-player.js

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


s/2016/2017

honeybadgerdontcare · 2017-03-08T19:30:22Z

extensions/amp-3q-player/0.1/test/validator-amp-3q.html

@@ -0,0 +1,47 @@
+<!--
+  Copyright 2016 The AMP HTML Authors. All Rights Reserved.


s/2016/2017

honeybadgerdontcare · 2017-03-08T19:30:31Z

extensions/amp-3q-player/0.1/validator-amp-3q-player.protoascii

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


s/2016/2017

honeybadgerdontcare · 2017-03-08T19:30:41Z

extensions/amp-3q-player/amp-3q-player.md

@@ -0,0 +1,54 @@
+<!---
+Copyright 2016 The AMP HTML Authors. All Rights Reserved.


s/2016/2017

honeybadgerdontcare · 2017-03-08T19:31:21Z

extensions/amp-3q-player/0.1/test/validator-amp-3q.html

@@ -0,0 +1,47 @@
+<!--


Please rename to validator-amp-3q-player.html.

honeybadgerdontcare · 2017-03-08T19:33:32Z

extensions/amp-3q-player/0.1/validator-amp-3q-player.protoascii

+      error_message: "contents"
+    }
+  }
+  spec_url: "https://www.ampproject.org/docs/reference/extended/amp-3q-player.html"


ampproject recently changed the urls, while this will redirect to the new one, lets just point to the new location instead, https://www.ampproject.org/docs/reference/components/media/amp-3q-player.

honeybadgerdontcare · 2017-03-08T19:33:39Z

extensions/amp-3q-player/0.1/validator-amp-3q-player.protoascii

+    mandatory: true
+  }
+  attr_lists: "extended-amp-global"
+  spec_url: "https://www.ampproject.org/docs/reference/extended/amp-3q-player.html"


ampproject recently changed the urls, while this will redirect to the new one, lets just point to the new location instead, https://www.ampproject.org/docs/reference/components/media/amp-3q-player.

honeybadgerdontcare · 2017-03-08T19:35:04Z

extensions/amp-3q-player/0.1/validator-amp-3q-player.protoascii

+  satisfies: "amp-3q-player"
+  requires: "amp-3q-player extension .js script"
+  attrs: {
+    name: "data-id"


Just an FYI: It is possible to enforce certain values for this in addition to making it mandatory. This can be done with value_regex or value_regex_casei. Up to you if you want to try and implement that type of regex.

Ill do the next days.

honeybadgerdontcare · 2017-03-08T19:37:31Z

extensions/amp-3q-player/0.1/test/validator-amp-3q.html

+      layout="responsive">
+    </amp-3q-player>
+
+    <!-- invalid, needs data-id -->


In addition to the .out file being empty, this is also a reason the travis build is failing. This test file will fail. So the .out file should start with FAIL followed by the errors in the file. According to the travis output we saw earlier it was expecting to see:

FAIL amp-3q-player/0.1/test/validator-amp-3q.html:41:4 The mandatory attribute 'data-id' is missing in tag 'amp-3q-player'. (see https://www.ampproject.org/docs/reference/extended/amp-3q-player.html) [AMP_TAG_PROBLEM]

This would go into the .out file. Note that after you update the spec_url, that the above would reflect the spec_url. So if you copy/paste this, please also update the spec_url here.

honeybadgerdontcare · 2017-03-08T19:39:38Z

Note I made some comments on files that were renamed, so you may want to look at outdated changes to see those comments. Let me know if you have other questions.

JuliusThms · 2017-03-08T19:40:00Z

@honeybadgerdontcare Many Thanks for your review. I commited the changes.

honeybadgerdontcare · 2017-03-08T19:45:49Z

extensions/amp-3q-player/0.1/validator-amp-3q-player.protoascii

+  spec_name: "amp-3q-player extension .js script"
+  satisfies: "amp-3q-player extension .js script"
+  mandatory_parent: "HEAD"
+  requires: "amp-3q-player"


Instead of requires here lets use this instead: extension_unused_unless_tag_present: "amp-3q-player". This also means we get to remove the satisfies: "amp-3d-player" below.

jridgewell · 2017-04-26T21:41:48Z

Sorry by stuck in loading I meant the inner video player is stuck, the iframe is loaded fine

Yes. But is this a message that is sent by the player and received by AMP (if so, it's our code's problem), or is this one sent by AMP to the player (if so, it's 3q's). From reading your comment, I was under the impression is was this

JuliusThms · 2017-04-26T21:43:49Z

@jridgewell Yes, you we're right. We changed the player so it's getting not fired too early. We have to wait some hours until the change is deployed.

aghassemi · 2017-04-26T22:15:20Z

@JuliusThms In the ideal situation, player sends the ready message as soon as possible but then it internally queues up commands (like play, mute, etc..) it receives until it is fully ready to execute them. This way user gets to see the player earlier. This is an optimization you can do later in the player without changing amp-3q-player.

aghassemi · 2017-04-27T18:58:52Z

@JuliusThms Current tests failures are the unit tests and not related to video-interface integration tests. They should be simple to fix and are probably due to the all the new code changes.

JuliusThms · 2017-04-27T22:47:57Z

@aghassemi Ok, i'll take a look

JuliusThms · 2017-05-02T17:01:13Z

@aghassemi Changed the unit tests, should be running fine now.

JuliusThms · 2017-05-02T18:50:30Z

@aghassemi The tests are passing, are we ready to go?

aghassemi · 2017-05-02T18:48:48Z

extensions/amp-3q-player/0.1/validator-amp-3q-player.protoascii

+  html_format: AMP
+  tag_name: "AMP-3Q-PLAYER"
+  requires: "amp-3q-player extension .js script"
+  attrs: {


add autoplay

attrs: { name: "autoplay" value: "" }

aghassemi · 2017-05-02T18:50:21Z

extensions/amp-3q-player/0.1/amp-3q-player.js

+    if (this.unlistenMessage_) {
+      this.unlistenMessage_();
+    }
+    return true;


please also add:

this.playerReadyPromise_ = new Promise(resolve => { this.playerReadyResolver_ = resolve; });

before returning.

aghassemi · 2017-05-02T18:52:55Z

@JuliusThms This is looking amazing! Thanks for taking the time to fully implement the video-interface. Two more small requests and then we can finally merge. Sorry for long review process, but this is turning out to be one of the best video players in AMP.

JuliusThms · 2017-05-02T19:01:28Z

@aghassemi I made the changes. Many thanks for the time you invest. It's a pleasure to contribute to this great project, and thanks for the good feedback!

aghassemi · 2017-05-02T19:29:37Z

Merged!

aghassemi · 2017-05-02T19:30:10Z

will take about three weeks until code is fully in production (including validator changes)

JuliusThms · 2017-05-02T19:30:49Z

@aghassemi Great! Let me know if i can do something!

JuliusThms pushed a commit to JuliusThms/amphtml that referenced this pull request Mar 6, 2017

- Fixes Pull ampproject#7999

1ad6321

JuliusThms pushed a commit to JuliusThms/amphtml that referenced this pull request Mar 6, 2017

Added dep config ampproject#7999

63d7dd5

JuliusThms pushed a commit to JuliusThms/amphtml that referenced this pull request Mar 6, 2017

Fix ampproject#7999

3fd3ee9

JuliusThms pushed a commit to JuliusThms/amphtml that referenced this pull request Mar 6, 2017

Fix ampproject#7999

b3faea0

JuliusThms pushed a commit to JuliusThms/amphtml that referenced this pull request Mar 6, 2017

Fix ampproject#7999

97e5ee5

JuliusThms pushed a commit to JuliusThms/amphtml that referenced this pull request Mar 6, 2017

Link fixes ampproject#7999

3a3ad3c

JuliusThms pushed a commit to JuliusThms/amphtml that referenced this pull request Mar 7, 2017

Fix Validator ampproject#7999

5f57535

JuliusThms pushed a commit to JuliusThms/amphtml that referenced this pull request Mar 7, 2017

Test Validation ampproject#7999

9e68145

honeybadgerdontcare reviewed Mar 8, 2017

View reviewed changes

JuliusThms pushed a commit to JuliusThms/amphtml that referenced this pull request Mar 8, 2017

File rename changes ampproject#7999

4e9a322

honeybadgerdontcare reviewed Mar 8, 2017

View reviewed changes

JuliusThms pushed a commit to JuliusThms/amphtml that referenced this pull request Mar 8, 2017

Required changes made ampproject#7999

a28e802

honeybadgerdontcare reviewed Mar 8, 2017

View reviewed changes

Julius Thomas added 2 commits April 27, 2017 00:48

Merge remote-tracking branch 'upstream/master'

79d6f9b

Build ampproject#7999

e05336f

Julius Thomas added 3 commits May 2, 2017 12:23

Merge remote-tracking branch 'upstream/master'

3c2c138

Merge remote-tracking branch 'upstream/master'

fb8d62a

Unit Test ampproject#7999

2fe4b22

Unit Test Fix ampproject#7999

684bcac

aghassemi suggested changes May 2, 2017

View reviewed changes

Requested Changes ampproject#7999

97a2db8

aghassemi approved these changes May 2, 2017

View reviewed changes

aghassemi merged commit 25395cb into ampproject:master May 2, 2017

KenneyE pushed a commit to spotxchange/amphtml that referenced this pull request May 3, 2017

Extension amp-3q-player added (ampproject#7999)

6f6703c

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels May 11, 2017

honeybadgerdontcare mentioned this pull request May 12, 2017

Pull in new components: amp-3q-player, amp-facebook-like, amp-ima-video & amp-timeago ampproject/amp.dev#483

Closed

3 tasks

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels May 15, 2017

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

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

		@@ -0,0 +1,47 @@
		<!--
		Copyright 2016 The AMP HTML Authors. All Rights Reserved.

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

		@@ -0,0 +1,54 @@
		<!---
		Copyright 2016 The AMP HTML Authors. All Rights Reserved.

Extension amp-3q-player added #7999

Extension amp-3q-player added #7999

Conversation

JuliusThms commented Mar 6, 2017

googlebot commented Mar 6, 2017

JuliusThms commented Mar 6, 2017

googlebot commented Mar 6, 2017

JuliusThms commented Mar 7, 2017 • edited

honeybadgerdontcare commented Mar 8, 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

honeybadgerdontcare commented Mar 8, 2017

JuliusThms commented Mar 8, 2017

Choose a reason for hiding this comment

jridgewell commented Apr 26, 2017

JuliusThms commented Apr 26, 2017

aghassemi commented Apr 26, 2017

aghassemi commented Apr 27, 2017

JuliusThms commented Apr 27, 2017

JuliusThms commented May 2, 2017 • edited

JuliusThms commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi commented May 2, 2017

JuliusThms commented May 2, 2017 • edited

aghassemi commented May 2, 2017

aghassemi commented May 2, 2017

JuliusThms commented May 2, 2017

JuliusThms commented Mar 7, 2017 •

edited

JuliusThms commented May 2, 2017 •

edited

JuliusThms commented May 2, 2017 •

edited