Suppress window animation on first window opening

RESOLVED FIXED in Firefox 55

Status

()

Firefox
General
P1
normal
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-performance])

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

2 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

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

Comment 3

2 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

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

Comment 4

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

Updated

2 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

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

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

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

Updated

a month ago
Attachment #8864575 - Flags: review?(florian)

Comment 9

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

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

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

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

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

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

Comment 17

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

Updated

a month ago
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
You need to log in before you can comment on or make changes to this bug.