Closed Bug 1362103 Opened 7 years ago Closed 7 years ago

Suppress window animation on first window opening

Categories

(Firefox :: General, defect, P1)

Product:
Firefox
Component:
General
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Iteration:
55.5 - May 15
Tracking Flags:
Tracking Status
firefox55 --- verified
firefox57 --- verified

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

Bug 1362103 - Suppress animation on first window opening.
59 bytes, text/x-review-board-request
florian
: review+
Details 
See summary. It's an idea that spun out of a meeting I was in today.

Bug 1336230 added the ability to do this.
I haven't tested this yet - need to test it on my Windows box.
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.
Flags: qe-verify?
Priority: -- → P2
For testing the differences between the two modes, might be useful to tie this to a pref.
Summary: Suppress animation on first window opening → Suppress window animation on first window opening
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?
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!
Status: NEW → ASSIGNED
Iteration: --- → 55.5 - May 15
Priority: P2 → P1
Attachment #8864575 - Flags: review?(florian)
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?
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 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)
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 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+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c2c91678807a
Suppress animation on first window opening. r=florian
https://hg.mozilla.org/mozilla-central/rev/c2c91678807a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
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.
Whiteboard: [photon-performance] → [photon-performance][qa-commented]
See Also: → 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+
Status: RESOLVED → VERIFIED
See Also: → 1387142
No longer depends on: 1399455
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: