Closed
Bug 1255918
Opened 9 years ago
Closed 9 years ago
Disable YouTube Flash-to-HTML5 embed rewriter in Beta 46 (plugins.rewrite_youtube_embeds = false)
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
| 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)
|
1.15 KB,
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.
|
||
Comment 1•9 years ago
|
||
Regression (I think), but in any case, tracking this to make sure it gets into beta 46.
Keywords: regression
|
Assignee | |
Comment 2•9 years ago
|
||
Turns the pref off by default, fixes test to turn pref on/off as needed to make tests run.
Attachment #8730437 -
Flags: review?(cpeterson)
|
|
Assignee | |
Comment 3•9 years ago
|
||
Running it through try real quick, for safety sake.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f3857148c65
|
Reporter | |
Comment 4•9 years ago
|
||
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+
|
|
Assignee | |
Comment 5•9 years ago
|
||
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.
|
|
Assignee | |
Comment 6•9 years ago
|
||
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?
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8730437 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 7•9 years ago
|
||
Attachment #8730502 -
Flags: approval-mozilla-beta?
|
|
Assignee | |
Comment 8•9 years ago
|
||
Annnnd nevermind it broke on try. Fixing and putting up a new version (though can get approval while that's happening)
|
|
Assignee | |
Comment 9•9 years ago
|
||
The very, very burnt try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f3857148c65&selectedJob=18053877
|
|
Assignee | |
Comment 10•9 years ago
|
||
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.
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8730501 -
Flags: approval-mozilla-beta?
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8730502 -
Attachment is obsolete: true
Attachment #8730502 -
Flags: approval-mozilla-beta?
|
|
Assignee | |
Comment 11•9 years ago
|
||
Turning off both pref and test
Attachment #8730501 -
Attachment is obsolete: true
Attachment #8730810 -
Flags: review?(cpeterson)
|
|
Reporter | |
Comment 12•9 years ago
|
||
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+
|
|
Assignee | |
Comment 13•9 years ago
|
||
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 14•9 years ago
|
||
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+
|
||
Comment 15•9 years ago
|
||
|
||
Comment 16•9 years ago
|
||
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
|
||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•