Closed Bug 1255918 Opened 8 years ago Closed 8 years ago

Disable YouTube Flash-to-HTML5 embed rewriter in Beta 46 (plugins.rewrite_youtube_embeds = false)

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Tracking Flags:
Tracking Status
firefox45 --- unaffected
firefox46 + verified
firefox47 --- unaffected
firefox48 --- unaffected

People

(Reporter: cpeterson, Assigned: qdot)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

Patch 1 (v3) - Disable youtube flash rewriting on Beta 46
(deleted), patch
cpeterson
: review+
lizzard
: approval-mozilla-beta+
Details | Diff | Splinter Review 
[Tracking Requested - why for this release]:

The YouTube Flash embed rewriter was enabled in FF 46 (bug 769117), but it breaks some embedded videos whose URLs have invalid query strings (bug 1240471). That bug was fixed in 47 but won't be uplifted to 46. Thus, we should disable the YouTube Flash embed rewriter in 46, as per bug 1240471 comment 51.
Regression (I think), but in any case, tracking this to make sure it gets into beta 46.
tracking-firefox46: ? → +
Keywords: regression
Turns the pref off by default, fixes test to turn pref on/off as needed to make tests run.
Attachment #8730437 - Flags: review?(cpeterson)
Comment on attachment 8730437 [details] [diff] [review]
Patch 1 (v1) - Disable youtube flash rewriting on Beta 46

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

LGTM

::: dom/base/test/test_bug769117.html
@@ +40,4 @@
>             SimpleTest.finish();
>           });
>       }
> +     addLoadEvent(() => { onLoad(); });

Why did you need to move onLoad() from <body> to addLoadEvent()? Is this change something we'll want to preserve when rewrite_youtube_embeds is re-enabled in 47?
Attachment #8730437 - Flags: review?(cpeterson) → review+
Yeah, I need to promote this test change up to 47 and central. 

The problem is the pref being off when we start the test in this case. The rewrite happens whenever the node is processed by the HTML parser, meaning the embed node in the body won't be processed if the pref isn't on before load time. However, we only want to run the test /after/ the embed has been loaded. So this just makes sure everything is ordered the correct way.

I'll split this into two patches and request approval-beta on both, but will only land the pref-turning-off patch to beta, while landing the test to aurora (with a=TEST-ONLY) and central also.
Approval Request Comment
[Feature/regressing bug #]: Bug 769117
[User impact if declined]: Rewrite can fail in some cases due to malformed queries (see bug 1240471)
[Describe test coverage new/current, TreeHerder]: Try run
[Risks and why]: None
[String/UUID change made/needed]: None
Attachment #8730501 - Flags: approval-mozilla-beta?
Attachment #8730437 - Attachment is obsolete: true
Annnnd nevermind it broke on try. Fixing and putting up a new version (though can get approval while that's happening)
I'm backing the patch in this bug down to just skip the mochitest altogether. Filing a followup to fix the test, but that'll live in 47+. Will post new patch when it has cleared try.
Attachment #8730501 - Flags: approval-mozilla-beta?
Attachment #8730502 - Attachment is obsolete: true
Attachment #8730502 - Flags: approval-mozilla-beta?
Turning off both pref and test
Attachment #8730501 - Attachment is obsolete: true
Attachment #8730810 - Flags: review?(cpeterson)
Comment on attachment 8730810 [details] [diff] [review]
Patch 1 (v3) - Disable youtube flash rewriting on Beta 46

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

LGTM for 46
Attachment #8730810 - Flags: review?(cpeterson) → review+
Comment on attachment 8730810 [details] [diff] [review]
Patch 1 (v3) - Disable youtube flash rewriting on Beta 46

Approval Request Comment
[Feature/regressing bug #]: Bug 769117
[User impact if declined]: Some youtube flash rewrites can fail due to patches from bug 1240471 not being uplifted
[Describe test coverage new/current, TreeHerder]: Try
[Risks and why]: None
[String/UUID change made/needed]: None
Attachment #8730810 - Flags: approval-mozilla-beta?
Comment on attachment 8730810 [details] [diff] [review]
Patch 1 (v3) - Disable youtube flash rewriting on Beta 46

Sounds like a good temporary solution for 46. This should end up in beta 4.
Attachment #8730810 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/5ff7037205e4
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Depends on: 1258053
I confirm that on Firefox 46.0b4 build 2, the plugins.rewrite_youtube_embeds pref is disabled.
Also, the embed videos are played with Flash player.
The tests were performed on Windows 10 x86, Mac OS X 10.11 and Ubuntu 14.04 x86.
Status: RESOLVED → VERIFIED
status-firefox46: fixed → verified
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: