Closed Bug 1176233 Opened 9 years ago Closed 9 years ago

Simplify fullscreen code

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 41
Tracking Status
firefox40 --- fixed
firefox41 --- verified

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file)

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
Attached patch patchSplinter Review
Attachment #8624712 - Flags: review?(quanxunzhen)
Is this going to be safe for branch uplift right before we go to beta? bug 1174431 also affects 40.
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 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+
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
Probably we want to uplift this patch for bug 1174431?
Flags: needinfo?(dao)
I'll request approval once this has baked in Nightly for a few more days.
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?
Dão, please n-i me when you think it is ready. Thanks
Depends on: 1177185
Attachment #8624712 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
(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)
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)
(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.
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.
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.
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(florin.mezei)
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+
Needs rebasing for beta uplift.
Flags: needinfo?(dao)
Depends on: 1191966
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)
This sounds more like a bug fixed by bug 1137009.
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)
Depends on: 1235592
Depends on: 1274834
Regressions: 1601706
No longer depends on: 1235592
Regressions: 1235592
Regressions: 1611689
No longer regressions: 1611689
You need to log in before you can comment on or make changes to this bug.