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

RESOLVED FIXED in Firefox 43, Firefox OS v2.5

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: qdot, Assigned: qdot)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox43 fixed, firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Created attachment 8680848 [details] [diff] [review]
Patch 1 (v1) - Add youtube object embedding to youtube embed telemetry
Assignee: nobody → kyle
Attachment #8680848 - Flags: review?(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. :)
Attachment #8680848 - Flags: review?(cpeterson) → review+
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.
Summary: Add support for tracking object tag embeds for youtube videos → Add support for tracking object tag embeds and enablejsapi for youtube videos
Created attachment 8681459 [details] [diff] [review]
Patch 1 (v2) - Add youtube object embedding and enablejsapi to youtube embed telemetry
Attachment #8680848 - Attachment is obsolete: true
Attachment #8681459 - Flags: review?(cpeterson)
Attachment #8681459 - Attachment description: Patch 1 (v1) - Add youtube object embedding and enablejsapi to youtube embed telemetry → Patch 1 (v2) - Add youtube object embedding and enablejsapi to youtube embed telemetry
Added the enablejsapi checking too.
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.
Attachment #8681459 - Flags: review?(cpeterson) → review+
Created attachment 8681550 [details] [diff] [review]
Patch 1 (v3) - Add youtube object embedding and enablejsapi to youtube embed telemetry; r=cpeterson

Fixed nits
Attachment #8681459 - Attachment is obsolete: true
Need checkin on this as I don't have a registered ssh key handy. :/
Keywords: checkin-needed

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0084543c175b
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0084543c175b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Comment 11

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0084543c175b
status-b2g-v2.5: --- → fixed
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
Attachment #8681550 - Flags: approval-mozilla-beta?
Attachment #8681550 - Flags: approval-mozilla-aurora?
status-firefox43: --- → affected
status-firefox44: --- → affected
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.
Attachment #8681550 - Flags: approval-mozilla-beta?
Attachment #8681550 - Flags: approval-mozilla-beta+
Attachment #8681550 - Flags: approval-mozilla-aurora?
Attachment #8681550 - Flags: approval-mozilla-aurora+
Kyle, Could you consider writing a test for this if possible? Thanks
Flags: needinfo?(kyle)

Comment 15

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/8a6cc7dda4d4
status-firefox44: affected → fixed

Comment 16

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/be9dcc3b97b3
status-firefox43: affected → fixed

Comment 17

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/8a6cc7dda4d4
Clearing ni? here as testing question moved to bug 1229971
Flags: needinfo?(kyle)
You need to log in before you can comment on or make changes to this bug.