Closed
Bug 1176233
Opened 9 years ago
Closed 9 years ago
Simplify fullscreen code
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 41
People
(Reporter: dao, Assigned: dao)
References
Details
Attachments
(1 file)
(deleted),
patch
|
xidorn
:
review+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Despite having been partially refactored recently, the browser fullscreen code is still a mess and hard to follow, which has lead to regressions such as bug 1174431. I think there are some simplification opportunities:
- hide more stuff with CSS instead of JS
- rename showXULChrome to something more sensible, remove the aTag parameter
- replace browser.fullscreen.animateUp with a boolean pref
- get rid of the _shouldAnimate flag and forceHide parameters
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8624712 -
Flags: review?(quanxunzhen)
Comment 2•9 years ago
|
||
Is this going to be safe for branch uplift right before we go to beta? bug 1174431 also affects 40.
Assignee | ||
Comment 3•9 years ago
|
||
Reasonably safe. Since it simplifies the code rather than adding layers of complication on top of it, it would hopefully make followup fixes easier in case more issues with the fullscreen code surface. There's a risk that the simplification is fundamentally flawed, but I think it's straightforward enough that this is very unlikely.
Comment 4•9 years ago
|
||
Comment on attachment 8624712 [details] [diff] [review]
patch
Review of attachment 8624712 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me. Thanks.
::: browser/base/content/browser-fullScreen.js
@@ +508,5 @@
> MousePosTracker.removeListener(this);
> },
>
> + _updateToolbars: function (aEnterFS) {
> + for (let el of document.querySelectorAll("toolbar[fullscreentoolbar=true]")) {
I suppose the selector "toolbar" has the same meaning with the original getElementsByTagNameNS() call even considering namespace. But I'm not that sure.
Attachment #8624712 -
Flags: review?(quanxunzhen) → review+
Assignee | ||
Comment 6•9 years ago
|
||
This has been merged without the bug being updated properly.
https://hg.mozilla.org/mozilla-central/rev/1f7db5d87e80
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Assignee | ||
Comment 8•9 years ago
|
||
I'll request approval once this has baked in Nightly for a few more days.
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8624712 [details] [diff] [review]
patch
Approval Request Comment
[Feature/regressing bug #]: bug 1168397
[User impact if declined]: bug 1174431
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: see comment 3
[String/UUID change made/needed]: none
Flags: needinfo?(dao)
Attachment #8624712 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Comment 10•9 years ago
|
||
Dão, please n-i me when you think it is ready. Thanks
status-firefox40:
--- → affected
Assignee | ||
Updated•9 years ago
|
Attachment #8624712 -
Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] PTO => July 10th from comment #10)
> Dão, please n-i me when you think it is ready. Thanks
Missed that comment. It's been ready ever since I requested approval ;)
Flags: needinfo?(sledru)
Comment 12•9 years ago
|
||
Florin - Given that this is not just a fix but also a larger change to simplify the code, can your team verify the fix on 41 or 42 before we uplift to Beta? If the fix can be verified by Wed, we can take the fix in beta3.
Flags: needinfo?(sledru) → needinfo?(florin.mezei)
Comment 13•9 years ago
|
||
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #12)
> Florin - Given that this is not just a fix but also a larger change to
> simplify the code, can your team verify the fix on 41 or 42 before we uplift
> to Beta? If the fix can be verified by Wed, we can take the fix in beta3.
We'll try to verify this by Wednesday or Thursday at the latest, depending on what other tasks we have in our backlog. Right now it seems we should be able to do it by tomorrow.
Also, just to be clear, here we are only talking about the browser fullscreen mode (e.g. using F11 on Windows), and no connection to things like Video fullscreen.
Assignee | ||
Comment 14•9 years ago
|
||
This is actually relevant for DOM fullscreen (e.g. video) too, since both fullscreen features share some frontend code. That's even part of the reason why the code got messy. For instance, this patch fixed bug 1174431.
Comment 15•9 years ago
|
||
Thanks Dao! Petruta started testing this today on Windows 7 x64 and Mac OS X 10.9.5, and so far things are going well. We intend to continue tomorrow with Windows 10 and Ubuntu and have the final results for this.
Comment 16•9 years ago
|
||
We performed exploratory testing around fullscreen using Aurora 41.0a2 2015-07-08 under Windows 7 x64, Windows 10 Pro x64 (Insider Preview Build 10162, Microsoft Surface Pro 2), Windows 10 Pro x64, Ubuntu 14.04 x32 and Mac OS X 10.9.5.
No new issues were logged, the issues found were either reproducible before the fix entered, so unrelated to fullscreen refactoring, either 3rd-party issues.
We were also able to successfully verify bug 1177185, bug 1174431, bug 1168397 and bug 1162752.
More details of our testing can be found at: https://etherpad.mozilla.org/FullscreenRefactoring-bug1176233
We think that this patch can be uplifted to beta.
Comment 17•9 years ago
|
||
Comment on attachment 8624712 [details] [diff] [review]
patch
With this fix verified, let's get it into beta3. Beta+
Attachment #8624712 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 18•9 years ago
|
||
Needs rebasing for beta uplift.
Flags: needinfo?(dao)
Keywords: branch-patch-needed
Assignee | ||
Comment 19•9 years ago
|
||
Comment 20•9 years ago
|
||
Encountered an issue on Firefox 38.5.0 ESR (under Ubuntu 14.04 32-bit) which is not reproducible on Firefox 45.0a1. It seems to be fixed by this bug.
The “Edit this bookmark” doorhanger is incorrectly displayed in fullscreen mode while using 2 screens.
STR
1. Launch Firefox.
2. Open http://cp.literature.agilent.com/litweb/pdf/5989-8139EN.pdf
3. Enter in fullscreen mode and click “Allow” button.
4. Right click and select “Bookmark this page”.
“Bookmarks this page” doorhanger is displayed on the second screen.
See screenshot: http://i.imgur.com/O3y66lY.jpg
Regression window with --find-fix flag
- First good revision: 1f7db5d87e8059e7e2947c4e1ece06e610107faf
- Last bad revision: 391241169becf6ed866341b96c7a88d4325ed83b
- Pushlog: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=391241169becf6ed866341b96c7a88d4325ed83b&tochange=1f7db5d87e8059e7e2947c4e1ece06e610107faf
Should I file a new bug for this issue, in order to be treated separately?
Flags: needinfo?(dao)
Comment 21•9 years ago
|
||
This sounds more like a bug fixed by bug 1137009.
Assignee | ||
Comment 22•9 years ago
|
||
Either way, no need to report separately if it's fixed on Nightly. It's not the kind of bug we would fix on ESR. ESR gets stability and security fixes only.
Flags: needinfo?(dao)
Assignee | ||
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•