Suppress window animation on first window opening

VERIFIED FIXED in Firefox 55

Status

()

Firefox
General
P1
normal
VERIFIED FIXED
4 months ago
16 days ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified, firefox57 verified)

Details

(Whiteboard: [photon-performance][qa-commented])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
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

4 months ago
I haven't tested this yet - need to test it on my Windows box.
(Assignee)

Comment 3

4 months 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

4 months ago
Flags: qe-verify?
Priority: -- → P2

Comment 4

4 months ago
For testing the differences between the two modes, might be useful to tie this to a pref.

Updated

4 months ago
Summary: Suppress animation on first window opening → Suppress window animation on first window opening
No longer blocks: 1348289
Comment hidden (mozreview-request)

Comment 6

3 months 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

3 months 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

3 months ago
Status: NEW → ASSIGNED
Iteration: --- → 55.5 - May 15
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8864575 - Flags: review?(florian)

Comment 9

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c2c91678807a
Suppress animation on first window opening. r=florian

Comment 17

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c2c91678807a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

3 months ago
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
(Assignee)

Comment 18

a month 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

a month ago
Whiteboard: [photon-performance] → [photon-performance][qa-commented]
See Also: → bug 1387045
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.
status-firefox55: fixed → verified
status-firefox57: --- → verified
Flags: qe-verify+

Updated

17 days ago
Status: RESOLVED → VERIFIED
See Also: → bug 1387142
You need to log in before you can comment on or make changes to this bug.