Last Comment Bug 765106 - Video controls are using the desktop binding
: Video controls are using the desktop binding
Status: RESOLVED FIXED
: regression
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 15 Branch
: All All
: -- normal (vote)
: Firefox 16
Assigned To: Wesley Johnston (:wesj)
:
Mentors:
Depends on:
Blocks: 760696
  Show dependency treegraph
 
Reported: 2012-06-14 17:48 PDT by Wesley Johnston (:wesj)
Modified: 2012-07-10 00:02 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Patch (739 bytes, text/plain)
2012-06-14 17:48 PDT, Wesley Johnston (:wesj)
mbrubeck: review+
Details
Patch v2 (2.81 KB, patch)
2012-06-15 11:31 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Screenshot of error on Native Fennec (41.95 KB, image/png)
2012-06-15 11:32 PDT, Wesley Johnston (:wesj)
no flags Details
Patch v2.1 (3.41 KB, patch)
2012-06-15 11:35 PDT, Wesley Johnston (:wesj)
gavin.sharp: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Wesley Johnston (:wesj) 2012-06-14 17:48:00 PDT
Created attachment 633336 [details]
Patch

Desktop recently started using -moz-any(video,audio) to attach their video controls binding. -moz-any is more specific than the video,audio we use, breaking ours.
Comment 2 Matt Brubeck (:mbrubeck) 2012-06-14 18:18:28 PDT
It looks like this regression is on Aurora too.
Comment 3 Wesley Johnston (:wesj) 2012-06-14 18:20:38 PDT
Comment on attachment 633336 [details]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 659402
User impact if declined: Video controls do not behave correctly (fade in and out) and have the wrong layout on mobile
Testing completed (on m-c, etc.): Landed on mc Jun14
Risk to taking this patch (and alternatives if risky):  Low risk. Gives us back our old beahvior.
String or UUID changes made by this patch:
Comment 4 Matt Brubeck (:mbrubeck) 2012-06-14 22:26:20 PDT
Sorry, I backed this out on inbound because it appeared to cause failures in mochitest-2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c9d11e009b7

https://tbpl.mozilla.org/php/getParsedLog.php?id=12684783&tree=Mozilla-Inbound
16653 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_constants.html | an unexpected uncaught JS exception reported through window.onerror - TypeError: label is null at chrome://global/content/bindings/videocontrols.xml:699
Comment 5 Wesley Johnston (:wesj) 2012-06-15 11:31:38 PDT
Created attachment 633595 [details] [diff] [review]
Patch v2

This adds some strings we ignored before because... we forgot them I guess. I'll carry over mbrubeck's review and get one from someone who can approve toolkit changes.
Comment 6 Wesley Johnston (:wesj) 2012-06-15 11:32:33 PDT
Created attachment 633596 [details]
Screenshot of error on Native Fennec

Screenshot. Also will push this to try. Also, forgot to port the styles change to XUL, so another patch is coming.
Comment 7 Wesley Johnston (:wesj) 2012-06-15 11:35:05 PDT
Created attachment 633600 [details] [diff] [review]
Patch v2.1
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-06-16 06:50:40 PDT
https://hg.mozilla.org/mozilla-central/rev/57fd3ee041cf
Comment 10 Wesley Johnston (:wesj) 2012-06-18 09:59:56 PDT
Comment on attachment 633600 [details] [diff] [review]
Patch v2.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 659402
User impact if declined: Video controls do not behave correctly (fade in and out) and have the wrong layout on mobile
Testing completed (on m-c, etc.): Landed on mc Jun14
Risk to taking this patch (and alternatives if risky):  Low risk. Gives us back our old beahvior.
String or UUID changes made by this patch:
Comment 11 Alex Keybl [:akeybl] 2012-06-19 19:30:47 PDT
(In reply to Wesley Johnston (:wesj) from comment #10)
> String or UUID changes made by this patch:

Do the changes in videocontrols.xml qualify as strings that will need to be newly localized?
Comment 12 Wesley Johnston (:wesj) 2012-06-19 19:36:10 PDT
Hmm... These were checked into Firefox 11 (bug 513405). I guess there is a possibility there is a language that we want to ship mobile in that hasn't already translated them for desktop, but I'm guessing no one matches that criteria. Axel, do you know how to check this?
Comment 13 Axel Hecht [:Pike] 2012-06-20 03:33:23 PDT
This is copying over the strings from videoControls to touchControls, both used for video elements, but with different UI interactions?

In that case, re-using the strings should be OK.

PS: You can search for strings in l10n on mxr, like http://mxr.mozilla.org/l10n-mozilla-aurora/search?string=error.srcNotSupported
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-21 15:07:14 PDT
This is just making use of strings that have already been added - this patch alone has no l10n impact.
Comment 15 Alex Keybl [:akeybl] 2012-06-24 13:16:30 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #14)
> This is just making use of strings that have already been added - this patch
> alone has no l10n impact.

Thanks Gavin.
Comment 16 Scoobidiver (away) 2012-07-09 14:41:12 PDT
Did it land in Aurora?
Comment 17 Wesley Johnston (:wesj) 2012-07-09 14:45:42 PDT
No....

Did now though: http://hg.mozilla.org/releases/mozilla-aurora/rev/f5a50e834842

Note You need to log in before you can comment on or make changes to this bug.