1218952 - Add support for tracking object tag embeds and enablejsapi for youtube videos

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Youtube's documentation recommends using the SWFObject library for embedding flash videos. SWFObject generated an object node for embedding, meaning our current telemetry doesn't pick it up.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Add youtube object embedding to youtube embed telemetry

Comment 2

[Chris Peterson [:cpeterson]]

Comment on attachment 8680848 [details] [diff] [review]
Patch 1 (v1) - Add youtube object embedding to youtube embed telemetry

Review of attachment 8680848 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM as per our email discussion.

::: dom/base/nsObjectLoadingContent.cpp
@@ +1480,3 @@
>    NS_ASSERTION(thisContent, "Must be an instance of content");
>  
>    // We're only interested in switching out embed tags

This comment is now incorrect. :)

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Actually, this is still checking for /all/ embed and object tags, not just ones that use embedjsapi. So I'll need one more revision of the patch.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - Add youtube object embedding and enablejsapi to youtube embed telemetry

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Added the enablejsapi checking too.

Comment 6

[Chris Peterson [:cpeterson]]

Comment on attachment 8681459 [details] [diff] [review]
Patch 1 (v2) - Add youtube object embedding and enablejsapi to youtube embed telemetry

Review of attachment 8681459 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

::: dom/base/nsObjectLoadingContent.cpp
@@ +1509,1 @@
>      return true;

Personally, I find double negative conditional like `!= kNotFound` hard to read. If you changed to `== kNotFound`, then this early return could return false like every other early return in this function. Then the only `return true` needed would be the end of the function.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - Add youtube object embedding and enablejsapi to youtube embed telemetry; r=cpeterson

Fixed nits

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Need checkin on this as I don't have a registered ssh key handy. :/

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0084543c175b

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0084543c175b

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0084543c175b

Comment 12

[Chris Peterson [:cpeterson]]

Comment on attachment 8681550 [details] [diff] [review]
Patch 1 (v3) - Add youtube object embedding and enablejsapi to youtube embed telemetry; r=cpeterson

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: We will have to wait longer before we have enough telemetry to know whether it is safe and worthwhile to rewrite embedded YouTube videos to use HTML5 video instead of Flash. This will help us reduce the use of Flash.
[Describe test coverage new/current, TreeHerder]: Patch has been on central for two days.
[Risks and why]: Very low risk because we are already reporting this YOUTUBE_EMBED_SEEN telemetry in Aurora 44 and Beta 43. This patch reduces how much telemetry we report.
[String/UUID change made/needed]: N/A

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8681550 [details] [diff] [review]
Patch 1 (v3) - Add youtube object embedding and enablejsapi to youtube embed telemetry; r=cpeterson

Anything to kill flash! Taking it.
Should be in 43 beta 2.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Kyle, Could you consider writing a test for this if possible? Thanks

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8a6cc7dda4d4

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/be9dcc3b97b3

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/8a6cc7dda4d4

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Clearing ni? here as testing question moved to bug 1229971