Closed
Bug 1362103
Opened 8 years ago
Closed 8 years ago
Suppress window animation on first window opening
Categories
(Firefox :: General, defect, P1)
Firefox
General
Tracking
()
People
(Reporter: mconley, Assigned: mconley)
References
(Blocks 1 open bug)
Details
(Whiteboard: [photon-performance][qa-commented])
Attachments
(1 file)
See summary. It's an idea that spun out of a meeting I was in today.
Bug 1336230 added the ability to do this.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
I haven't tested this yet - need to test it on my Windows box.
Assignee | ||
Comment 3•8 years ago
|
||
To be clear, for the people following along, the animation I'm referring to is the OS animation when windows open. Windows has had these for a while, and OS X got them with Lion, I believe.
This has nothing to do with the in-browser animations.
Updated•8 years ago
|
Flags: qe-verify?
Priority: -- → P2
Comment 4•8 years ago
|
||
For testing the differences between the two modes, might be useful to tie this to a pref.
Updated•8 years ago
|
Summary: Suppress animation on first window opening → Suppress window animation on first window opening
Updated•8 years ago
|
No longer blocks: photon-performance-triage
Comment hidden (mozreview-request) |
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8864575 [details]
Bug 1362103 - Suppress animation on first window opening.
https://reviewboard.mozilla.org/r/136244/#review141830
::: browser/components/nsBrowserContentHandler.js:399
(Diff revision 2)
> throw e;
> }
> // NS_ERROR_INVALID_ARG is thrown when flag exists, but has no param.
> if (cmdLine.handleFlag("private-window", false)) {
> openWindow(null, this.chromeURL, "_blank",
> - "chrome,dialog=no,private,all" + this.getFeatures(cmdLine),
> + "chrome,suppressanimation,dialog=no,private,all" + this.getFeatures(cmdLine),
Leftover since this is now handled by getFeatures?
::: browser/components/nsBrowserContentHandler.js:583
(Diff revision 2)
> if (PrivateBrowsingUtils.isInTemporaryAutoStartMode) {
> this.mFeatures = ",private";
> }
> +
> + if (Services.prefs.getBoolPref("browser.suppress_first_window_animation")) {
> + let win = RecentWindow.getMostRecentBrowserWindow({private: true});
{private: true} will give you *only* private windows. I believe you want all windows here?
Assignee | ||
Comment 7•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8864575 [details]
Bug 1362103 - Suppress animation on first window opening.
https://reviewboard.mozilla.org/r/136244/#review141830
> Leftover since this is now handled by getFeatures?
Whoops - indeed, leftovers. I should probably have done a self-review. :/
> {private: true} will give you *only* private windows. I believe you want all windows here?
Ah, good catch! I thought that meant to _include_ private windows. Thank you!
Updated•8 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 55.5 - May 15
Priority: P2 → P1
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8864575 -
Flags: review?(florian)
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8864575 [details]
Bug 1362103 - Suppress animation on first window opening.
https://reviewboard.mozilla.org/r/136244/#review142570
::: browser/components/nsBrowserContentHandler.js:583
(Diff revision 3)
> if (PrivateBrowsingUtils.isInTemporaryAutoStartMode) {
> this.mFeatures = ",private";
> }
> +
> + if (Services.prefs.getBoolPref("browser.suppress_first_window_animation")) {
> + let win = RecentWindow.getMostRecentBrowserWindow();
I'm afraid using RecentWindow here means we are now loading this module early during startup, when it used to be lazy loaded only when clicking a link from an external window and Firefox was already open.
Wouldn't using Services.wm.getMostRecentWindow("navigator:browser") be enough here for the simple check we are doing?
Assignee | ||
Comment 10•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8864575 [details]
Bug 1362103 - Suppress animation on first window opening.
https://reviewboard.mozilla.org/r/136244/#review142570
> I'm afraid using RecentWindow here means we are now loading this module early during startup, when it used to be lazy loaded only when clicking a link from an external window and Firefox was already open.
>
> Wouldn't using Services.wm.getMostRecentWindow("navigator:browser") be enough here for the simple check we are doing?
> Wouldn't using Services.wm.getMostRecentWindow("navigator:browser") be enough here for the simple check we are doing?
Yeah, I agree that'd be better. New patch coming up, thanks!
Comment hidden (mozreview-request) |
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8864575 [details]
Bug 1362103 - Suppress animation on first window opening.
https://reviewboard.mozilla.org/r/136244/#review142590
::: browser/components/nsBrowserContentHandler.js:584
(Diff revision 4)
> this.mFeatures = ",private";
> }
> +
> + if (Services.prefs.getBoolPref("browser.suppress_first_window_animation")) {
> + let win = Services.wm.getMostRecentWindow("navigator:browser");
> + if (!win) {
You don't need the win variable and can simplify:
if (...getBoolPref(...) &&
!...getMostRecentWindow(...)) {
this.mFeatures = ",suppressanimation";
}
Also, it seems this wants to be a '+=' operator, and not just '='. The line for the 'private' flag seems similarly broken.
Attachment #8864575 -
Flags: review?(florian)
Assignee | ||
Comment 13•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8864575 [details]
Bug 1362103 - Suppress animation on first window opening.
https://reviewboard.mozilla.org/r/136244/#review142590
> You don't need the win variable and can simplify:
> if (...getBoolPref(...) &&
> !...getMostRecentWindow(...)) {
> this.mFeatures = ",suppressanimation";
> }
>
> Also, it seems this wants to be a '+=' operator, and not just '='. The line for the 'private' flag seems similarly broken.
> Also, it seems this wants to be a '+=' operator, and not just '='. The line for the 'private' flag seems similarly broken.
Oh holy smokes, good eyes! Thanks for catching that!
Comment hidden (mozreview-request) |
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8864575 [details]
Bug 1362103 - Suppress animation on first window opening.
https://reviewboard.mozilla.org/r/136244/#review142598
Thanks!
Attachment #8864575 -
Flags: review?(florian) → review+
Comment 16•8 years ago
|
||
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c2c91678807a
Suppress animation on first window opening. r=florian
Comment 17•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•8 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
Assignee | ||
Comment 18•7 years ago
|
||
QE notes:
Prior to this patch, on Windows and OS X, there'd be a brief window animation when opening the initial browser window.
This patch suppresses that animation, so we get the initial window consuming its final space on the screen immediately without any transitions.
For verifying this bug, I'd start the browser and ensure the initial window does not have the animation on OS X and Windows. It might be worth testing the following additional scenarios:
1) Firefox is started by clicking on a link in another application
2) Firefox is started from the command line
Basically, variations on how Firefox can start. They should all suppress that initial window.
Assignee | ||
Updated•7 years ago
|
Whiteboard: [photon-performance] → [photon-performance][qa-commented]
Updated•7 years ago
|
Comment 19•7 years ago
|
||
Enviroments:
Windows 8.1 x64, Windows 10 x64, Mac OSX 10.12
Prior to verifying and regression testing I reproduced the animation with a Nightly build from 05.15 on target OSes.
test areas:
-open from command line
-open from Skype, facebook, different browser (or different FF channel)
-open with parameters;
No visible regressions, so marking this as verified for 55.0b9 20170713130618 and 57.0a1 20170802100302.
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•