Closed Bug 1218952 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox43 --- fixed
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: qdot, Assigned: qdot)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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
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+
Need checkin on this as I don't have a registered ssh key handy. :/
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0084543c175b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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?
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)
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.