Closed
Bug 1336227
Opened 7 years ago
Closed 6 years ago
Show a blank window as soon as possible after start-up (pref'd off)
Categories
(Firefox :: General, enhancement, P2)
Tracking
()
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: mconley, Assigned: florian)
References
(Depends on 1 open bug, Blocks 2 open bugs, Regressed 2 open bugs, )
Details
(Whiteboard: [fxperf])
Attachments
(2 files, 1 obsolete file)
49.46 KB,
application/x-zip-compressed
|
Details | |
6.63 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
As part of our efforts to improve perceived performance, one of the things that the UX team has identified coming out of the Photon work is that we want the initial browser window to appear instantaneously. This is true _even if we haven't figured out how to lay out the UI yet_. The way that Chrome does this is by displaying a blank, undecorated window very soon after process start. Edge shows a splash screen (an idea which we've evaluated and discarded). By showing a window more quickly, we also (I believe) get the Firefox icon into the taskbar more quickly. We should be using Edge as our timing benchmark here, since it appears to be fastest. The request from UX is that we beat or at least be at parity with Edge on showing something after startup. Putting this under Widget: Win32, since I think this is likely going to be platform specific, and if we start anywhere, it's going to be Windows.
Reporter | ||
Comment 1•7 years ago
|
||
We might want to speak with the Hasal[1] folk to see if their framework could possibly be instrumented to give us real concrete numbers on how Edge performs here, to give us a raw number to beat. [1]: https://wiki.mozilla.org/Hasal
Comment 2•7 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #1) > We might want to speak with the Hasal[1] folk to see if their framework > could possibly be instrumented to give us real concrete numbers on how Edge > performs here, to give us a raw number to beat. Just recording the effects on a representative machine should be enough to have a side by side comparison. This could even be done on low, mid and high tier laptop. How well is this instrumented in Telemetry currently? Do we know the breakdown of startup or would it make sense to add better probes (window visible, tabs painted, window interactive)?
Flags: needinfo?(mconley)
Reporter | ||
Comment 3•7 years ago
|
||
(In reply to :Harald Kirschner :digitarald from comment #2) > (In reply to Mike Conley (:mconley) from comment #1) > > We might want to speak with the Hasal[1] folk to see if their framework > > could possibly be instrumented to give us real concrete numbers on how Edge > > performs here, to give us a raw number to beat. > > Just recording the effects on a representative machine should be enough to > have a side by side comparison. This could even be done on low, mid and high > tier laptop. > > How well is this instrumented in Telemetry currently? Do we know the > breakdown of startup or would it make sense to add better probes (window > visible, tabs painted, window interactive)? It looks like nsIAppStartup's getStartupInfo will return information on a bunch of start-up timeline information - specifically, these: http://searchfox.org/mozilla-central/rev/f5077ad52f8b90183e73038869f6140f0afbf427/toolkit/components/startup/StartupTimeline.h#6-21 This information is submitted to Telemetry under "simple measurements", and should be represented as "number of milliseconds that we hit this point in the timeline in ms". So the ones we probably care about are createTopLevelWindow and firstPaint.
Flags: needinfo?(mconley)
Reporter | ||
Comment 5•7 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #4) > mconley, any sense of priority on this? Yeah - this is a high-priority one. UX wants this for Photon, but it can go out before Photon is ready.
Flags: needinfo?(mconley)
Reporter | ||
Comment 6•7 years ago
|
||
Here's phlsa's mock-up of what UX is requesting: https://phlsa.github.io/photon-performance/startup.html Note that, according to phlsa, it's not necessary (and might not be short-term feasible) to have the content area be rendered even if the browser UI isn't ready. What's most important is for _anything_ (even a blank rectangle) to display and for the icon to appear in the taskbar, immediately once the user starts Firefox.
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Comment 7•7 years ago
|
||
One thing that was mentioned in the commentary in https://docs.google.com/document/d/10R8CqOhdcGdkqDJFOQ67vzSiA7aNDinzQLR1OIP4sNI/edit#, but is not part of the mock-up, is that we could show a screenshot of the browser UI in this window while Gecko is still spinning up. I know there was some concern about showing _content_ that the user might not want displayed from the last shutdown, but we could just show the UI with blank content. We'd need to sort out when to invalidate that screenshot (any time the browser UI changes dimensions or composition, I guess), but I think that is probably strictly better than showing just a blank window.
Comment 8•7 years ago
|
||
re: I would experiment with showing hints of UI (wireframe style?), as with themes and addons the final UI could look very different from the default. The other concern is showing anything that might look interactive (but isn't).
Reporter | ||
Comment 9•7 years ago
|
||
For Windows at least, we need to be processing messages on the native window on the thread that hopes to use that window, in order to make the window paint and to not seem frozen. Regardless of what we show in that initial window (screenshot, blank, something else), we have a few choices here: 1) Create and show that initial window off of the main thread, but that window will need to be closed / hidden once Gecko is ready and we'll then replace it with an actual Firefox window. I suspect this is probably pretty easy, but is probably not an amazing way to transition is from not running to ready. We might be able to do some sleight of hand to disguise it, but the seams with this approach will probably show a bit no matter what. 2) Create and show that initial window on the main thread, and initialize Gecko _off_ of the main thread. This would allow us to paint that window and make it seem responsive while Gecko starts up. Then, once Gecko is ready, have Gecko "take over" that window, and turn it into a Firefox window. (2) Is probably the one that'll feel the most "right", and there's actually prior art for this - Metro did something like this back in the day. It is, however, a non-trivial chunk of work to make it work for Desktop-proper. There'd be a lot of plumbing required to do this, and that's not considering any fallout for a11y or other unknown unknowns. jimm estimated this work to be something like 6 months (3 months until we had something we could have something on its feet, the other 3 to deal with potential fallout), and 57 is cut in June (~4 months from today). So there's some risk with (2). canuckistani, of these two options, do you have a feeling about which one we should place our bet on?
Flags: needinfo?(jgriffiths)
Comment 10•7 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #9) > For Windows at least, we need to be processing messages on the native window > on the thread that hopes to use that window, in order to make the window > paint and to not seem frozen. > > Regardless of what we show in that initial window (screenshot, blank, > something else), we have a few choices here: > > 1) Create and show that initial window off of the main thread, but that > window will need to be closed / hidden once Gecko is ready and we'll then > replace it with an actual Firefox window. I suspect this is probably pretty > easy, but is probably not an amazing way to transition is from not running > to ready. We might be able to do some sleight of hand to disguise it, but > the seams with this approach will probably show a bit no matter what. If we go down this road there are some clever things we can do, like make it really obvious that Firefox is still loading and not running yet. This is actually a really old technique, the "loading" dialogue a lot of pro software does ( Pshop, Ableton, various IDEs ). For unavoidably long start times this is much better than the user clicking on the icon, then nothing happens for several seconds, *then* the window appears. > > 2) Create and show that initial window on the main thread, and initialize > Gecko _off_ of the main thread. This would allow us to paint that window and > make it seem responsive while Gecko starts up. Then, once Gecko is ready, > have Gecko "take over" that window, and turn it into a Firefox window. This would be much better, especially if we could do something, anything in the window before we load Gecko into it. > (2) Is probably the one that'll feel the most "right", and there's actually > prior art for this - Metro did something like this back in the day. It is, > however, a non-trivial chunk of work to make it work for Desktop-proper. > There'd be a lot of plumbing required to do this, and that's not considering > any fallout for a11y or other unknown unknowns. jimm estimated this work to > be something like 6 months (3 months until we had something we could have > something on its feet, the other 3 to deal with potential fallout), and 57 > is cut in June (~4 months from today). > > So there's some risk with (2). canuckistani, of these two options, do you > have a feeling about which one we should place our bet on? I think we should go for (2) regardless, I think this is just something we need. The question we're asking is instead, what should we do now, before 57. I think we should do (1) and consider it an opportunity for whimsy injection, assuming the engineering involved is minimal. I think we should investigate (2) and put it in a higher priority backlog, unless someone comes up with a creative solution to (2) where it takes a lot less time.
Flags: needinfo?(jgriffiths)
Reporter | ||
Comment 11•7 years ago
|
||
I should also mention secret option (3), which is to profile startup and just try to make us start up faster in general so that we don't have to do any of this fake window stuff. But that's filled with a ton of unknowns and I'm not sure how much room there is for improvement. Also, I made a mistake in comment 9 - 57 isn't cut in June. It's cut in August. That's ~6 months.
Reporter | ||
Comment 12•7 years ago
|
||
jimm has also expressed an interest in having himself (or his team?) throw some effort into (2) once e10s+a11y is out the door. Hey canuckistani, I'm a little confused on this bit: (In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #10) > The question we're asking is instead, what should we do now, before > 57. I think we should do (1) and consider it an opportunity for whimsy > injection, assuming the engineering involved is minimal. Assuming we can get some engineers allocated to (2) with a reasonable expectation of having it ship in 57, are you saying we should also try (1) first, either first or in tandem? What did you have in mind for whimsy injection? Some kind of pref-onnable thing that we could drop into a SHIELD study to see if it improves the perceived start-up performance? Or something else altogether?
Flags: needinfo?(jgriffiths)
Comment 13•7 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #11) > I should also mention secret option (3), which is to profile startup and > just try to make us start up faster in general so that we don't have to do > any of this fake window stuff. But that's filled with a ton of unknowns and > I'm not sure how much room there is for improvement. FWIW I think the last time we actively looked into startup performance was years ago. I will not be surprised at all if we have grown some low hanging fruit to pick there over the years.
Comment 14•7 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #12) > jimm has also expressed an interest in having himself (or his team?) throw > some effort into (2) once e10s+a11y is out the door. > > Hey canuckistani, I'm a little confused on this bit: > > (In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #10) > > The question we're asking is instead, what should we do now, before > > 57. I think we should do (1) and consider it an opportunity for whimsy > > injection, assuming the engineering involved is minimal. > > Assuming we can get some engineers allocated to (2) with a reasonable > expectation of having it ship in 57, are you saying we should also try (1) > first, either first or in tandem? What did you have in mind for whimsy > injection? Some kind of pref-onnable thing that we could drop into a SHIELD > study to see if it improves the perceived start-up performance? Or something > else altogether? If we go with that assumption, then no. I think 1) should only be pursued if we have low confidence in 2) - sorry for the confusion.
Flags: needinfo?(jgriffiths)
Comment 15•7 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #7) > One thing that was mentioned in the commentary in > https://docs.google.com/document/d/ > 10R8CqOhdcGdkqDJFOQ67vzSiA7aNDinzQLR1OIP4sNI/edit#, but is not part of the > mock-up, is that we could show a screenshot of the browser UI in this window > while Gecko is still spinning up. If we do this, can we please try to make it as clear as possible that the window is not interactive yet, so people don't try to click or type things, and get frustrated when their input gets lost? Or at the lack of rollover effects, and perceived poor responsiveness. This was a common frustration with XUL Fennec, and I'd rather not wind up in that boat again. A soft-focused, or low-detail, screenshot might be enough. Then we could smooth transition into the real UI when it's ready.
(In reply to Kris Maglione [:kmag] from comment #15) > (In reply to Mike Conley (:mconley) from comment #7) > > One thing that was mentioned in the commentary in > > https://docs.google.com/document/d/ > > 10R8CqOhdcGdkqDJFOQ67vzSiA7aNDinzQLR1OIP4sNI/edit#, but is not part of the > > mock-up, is that we could show a screenshot of the browser UI in this window > > while Gecko is still spinning up. > > If we do this, can we please try to make it as clear as possible that the > window is not interactive yet, so people don't try to click or type things, > and get frustrated when their input gets lost? Or at the lack of rollover > effects, and perceived poor responsiveness. This was a common frustration > with XUL Fennec, and I'd rather not wind up in that boat again. A > soft-focused, or low-detail, screenshot might be enough. Then we could > smooth transition into the real UI when it's ready. Some recent sites / apps use a "wireframe" look while things are still loading with filled rectangles where controls and text will soon appear. Maybe that's useful here with the browser window?
Updated•7 years ago
|
Priority: -- → P3
Whiteboard: tpi:+
Updated•7 years ago
|
Blocks: photon-performance-triage
Updated•7 years ago
|
No longer blocks: photon-visual
Reporter | ||
Comment 17•7 years ago
|
||
phlsa has told me that we'll have the science informing a go-no-go on this coming Real Soon Now. Stand by.
Updated•7 years ago
|
Whiteboard: tpi:+ → [photon] tpi:+
Updated•7 years ago
|
Priority: P3 → P1
Whiteboard: [photon] tpi:+ → [photon][tpi:+]
Comment 18•7 years ago
|
||
Comment 19•7 years ago
|
||
> 1) Create and show that initial window off of the main thread, but that
> window will need to be closed / hidden once Gecko is ready and we'll then
> replace it with an actual Firefox window. I suspect this is probably pretty
> easy, but is probably not an amazing way to transition is from not running
> to ready. We might be able to do some sleight of hand to disguise it, but
> the seams with this approach will probably show a bit no matter what.
EarlyWindowSwap is a little Windows binary that simulates this type of behavior. There are a number issues we'll run into with this approach. most notably:
1) Mouse interaction issues. If you drag the first window, when the second takes over your mouse will 'drop' the first window.
2) Create window animations throw a wrench in this, particularly on Windows 7.
We can hack on this, but I don't think we'll ever get it looking right. I'd suggest we shoot for option two. I'm going to do some investigating into that next.
Updated•7 years ago
|
Flags: needinfo?(bwinton)
Comment 20•7 years ago
|
||
Could we… Open the new window in the background before closing the old window? Override the window animations when closing the old window (if there are any)? Perhaps fade it out instead? Also, how long do we expect to show the first window for? If it's under a second, then mouse interaction issues will be less of an, uh, issue. ;) Getting time estimates for the two options would also be good. If it's a matter of a day or two difference, that's one thing. If it's the difference between a week and three months, that something entirely different. (Hopefully that was the info you needed from me. :)
Flags: needinfo?(bwinton)
Comment 21•7 years ago
|
||
(And it should go without saying that any comments by :shorlander or :phlsa totally trump anything I've said, and you should go with their opinion if they have one. :)
Comment 22•7 years ago
|
||
(In reply to Blake Winton (:bwinton) (:☕️) from comment #20) > Could we… > > Open the new window in the background before closing the old window? I'm actually doing that in this demo, but the window open and close animations slow the display and positioning of the second window which makes it look like the second window shows up late. > Override the window animations when closing the old window (if there are > any)? Perhaps fade it out instead? We could try and hack around the animations through different window types. I think transparent windows avoid the "swish" effect on Windows 7. Not sure about Windows 10, I'd have to test. I still think this going to end up looking a little kludgy due to focus queues that display for both windows. My point being: completely obscuring this swap operation from the user's view may not be possible. I'll play around with different ideas in this demo I have, see what I can come up with. > Also, how long do we expect to show the first window for? If it's under a > second, then mouse interaction issues will be less of an, uh, issue. ;) Telemetry indicates first window display ranges from 300msec -> 24sec [1]. So we should expect that users will have ample time to look at and attempt to interact with this temporary window before the real browser window shows up. (This is why the mouse interact issues are a concern.) You can be sure users will grab this temporary window and drag it around, resize it, or maybe even close it before the browser windows shows up. We'll have to guard around all these odd cases. > Getting time estimates for the two options would also be good. If it's a > matter of a day or two difference, that's one thing. If it's the difference > between a week and three months, that something entirely different. I'm going to dig into the other option this week to see if I can identify any show stoppers. [1] https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&e10s=true&end_date=2017-03-22&keys=__none__!__none__!__none__&max_channel_version=release%252F52&measure=SIMPLE_MEASURES_CREATETOPLEVELWINDOW&min_channel_version=null&os=Windows_NT&processType=parent&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-03-02&table=0&trim=1&use_submission_date=0
Updated•7 years ago
|
Flags: needinfo?(philipp)
Updated•7 years ago
|
No longer blocks: photon-performance-triage
Whiteboard: [photon][tpi:+] → [tpi:+]
Reporter | ||
Updated•7 years ago
|
See Also: → photon-startup
Reporter | ||
Comment 23•7 years ago
|
||
An additional idea, which bwinton expressed to me in person, but isn't (I believe) represented in this bug, is to open the initial window as early as possible on the main thread, and then start Gecko on that same main thread. This means that, potentially, that initial window is going to go unresponsive (since we'll be busy starting up the engine, and not servicing user events), and I'm unsure how Windows will treat us in that case (I suspect it might show a "Not responding" text next to the window title at some point), but we wanted to get the idea down as yet another option.
Comment 24•7 years ago
|
||
Should we track this bug in photon-performance?
Reporter | ||
Comment 25•7 years ago
|
||
(In reply to Evelyn Hung [:evelyn] from comment #24) > Should we track this bug in photon-performance? I don't personally mind it blocking photon-performance, but just to track - I don't _think_ there'll be work for the Photon Performance team to do here, as this is mostly platform integration work.
Comment 26•7 years ago
|
||
okay, thanks for the explanation.
Comment 27•7 years ago
|
||
Jim, is there something specific that you are needinfoing about? I'll just put everything that comes to mind here for now :) As for bwintons suggestings, from Jims response I take it that even when startup is fast, we will get some flashing with any kind of two-window solution. Slightly related: looking at the startup time histogram, the median is 2.16s and the 75th percentile is 5.18s. To me, that suggests that we can treat super-long start times as an edge case. The experience for users who are in this group is going to be bad, regardless of whether or not we show a window. We still shouldn't completely mess up the experience for this group, but it's probably fair to have a little flickering towards the end if it improves the experience for everyone else (if that's the kind of trade-off we end up having to make) On the window animation: didn't mconley do something to ommit window animations when opening a new window and the previous window is in maximized mode? Could that help us here? One other option would be to actually go with something like a window-sized splash screen. Maybe we can design it in a way that looks just enough like a window to have the effect (same dimensions), but where it doesn't look unnatural that there is some flickering after that? Finally: Jim, could you (or someone else who has it available) post a screen cap of the behavior of EasyWindowSwap? I'm sitting in Taipei this week without access to my Windows machine... :/
Flags: needinfo?(philipp) → needinfo?(jmathies)
Comment 28•7 years ago
|
||
(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #27) > Jim, is there something specific that you are needinfoing about? I'll just > put everything that comes to mind here for now :) I was asking for follow up comments by you post bwinton's comments. > As for bwintons suggestings, from Jims response I take it that even when > startup is fast, we will get some flashing with any kind of two-window > solution. > > Slightly related: looking at the startup time histogram, the median is 2.16s > and the 75th percentile is 5.18s. To me, that suggests that we can treat > super-long start times as an edge case. The experience for users who are in > this group is going to be bad, regardless of whether or not we show a > window. We still shouldn't completely mess up the experience for this group, > but it's probably fair to have a little flickering towards the end if it > improves the experience for everyone else (if that's the kind of trade-off > we end up having to make) Sounds good. > On the window animation: didn't mconley do something to ommit window > animations when opening a new window and the previous window is in maximized > mode? Could that help us here? We can't turn the animation off for top level windows on per window or app basis, it's controlled by a system setting that alters desktop behavior. > One other option would be to actually go with something like a window-sized > splash screen. Maybe we can design it in a way that looks just enough like > a window to have the effect (same dimensions), but where it doesn't look > unnatural that there is some flickering after that? There shouldn't be any flickering once we have the two threads separated. > Finally: Jim, could you (or someone else who has it available) post a screen > cap of the behavior of EasyWindowSwap? I'm sitting in Taipei this week > without access to my Windows machine... :/ There's time. FYI this project was escalated to figure out if we really wanted to invest the time in it for 57. The decision was that we do not, so we're going to look at startup perf instead. That said this project isn't dead, we'd like to return to it after Quantum work ships. Since we have had good UX discussions here, I'm going to convert this bug to a meta for the overall project.
Flags: needinfo?(jmathies)
Updated•7 years ago
|
Assignee | ||
Comment 29•7 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #28) > > On the window animation: didn't mconley do something to ommit window > > animations when opening a new window and the previous window is in maximized > > mode? Could that help us here? > > We can't turn the animation off for top level windows on per window or app > basis, it's controlled by a system setting that alters desktop behavior. The bug philipp remembered is bug 1336230.
Updated•7 years ago
|
Priority: P1 → P2
Comment 30•7 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #28) > (In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from > > On the window animation: didn't mconley do something to ommit window > > animations when opening a new window and the previous window is in maximized > > mode? Could that help us here? > > We can't turn the animation off for top level windows on per window or app > basis, it's controlled by a system setting that alters desktop behavior. (In reply to Florian Quèze [:florian] [:flo] from comment #29) > (In reply to Jim Mathies [:jimm] from comment #28) > > > > On the window animation: didn't mconley do something to ommit window > > > animations when opening a new window and the previous window is in maximized > > > mode? Could that help us here? > > > > We can't turn the animation off for top level windows on per window or app > > basis, it's controlled by a system setting that alters desktop behavior. > > The bug philipp remembered is bug 1336230. ..and I sit here feeling humbled. :) Especially since I reviewed that work.
Comment 31•7 years ago
|
||
Hey Jim, how hard would it be to get an EasyWindowSwap-style demo of "An additional idea, open the initial window as early as possible on the main thread, and then start Gecko on that same main thread." Like, don't actually start gecko, but maybe wait (without pumping the event loop) for 1/2/5/10 seconds before continuing, to see what the behaviour from Windows is in those cases… (It may still be a terrible idea, but I think it's worth a quick test, if that's a thing we could do.)
Flags: needinfo?(jmathies)
Comment 32•7 years ago
|
||
(In reply to Blake Winton (:bwinton) (:☕️) from comment #31) > Hey Jim, how hard would it be to get an EasyWindowSwap-style demo of "An > additional idea, open the initial window as early as possible on the main > thread, and then start Gecko on that same main thread." Like, don't > actually start gecko, but maybe wait (without pumping the event loop) for > 1/2/5/10 seconds before continuing, to see what the behaviour from Windows > is in those cases… > > (It may still be a terrible idea, but I think it's worth a quick test, if > that's a thing we could do.) Not a good idea really. You'll risk getting ghost windowing on that window by Windows if it detects the pump isn't responding. This can also cause lockups in other applications if they try to communicate with that window while the pump isn't running. Once the message pump kicks in, everything would free up and start running normally.
Flags: needinfo?(jmathies)
Assignee | ||
Comment 33•6 years ago
|
||
Here is my prototype, I'm attaching it as a single patch here so that people can play with it. There's a try server push available at https://treeherder.mozilla.org/#/jobs?repo=try&revision=07c12a2370abe6214d6dfff9249dc4203ab22f1c It works well enough at this point that I think the patch is ready to be broken down for review, and that I would like to land this pref'ed off so that we can file follow-up bugs to polish the details. I'm going to treat this bug as a meta bug. Known issues with the current prototype: - on Mac: black borders around the blank window - on Windows: the waiting cursor stays until the mouse is moved - on Linux: the blank window is black instead of white.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → florian
Status: NEW → ASSIGNED
Updated•6 years ago
|
Whiteboard: [fxperf]
Assignee | ||
Comment 34•6 years ago
|
||
I think this is now ready for review in order to land it pref'ed off, so that remaining issues can be handled as follow-up bugs. The most recent try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=02964256cbbb3983c9c787ac73fddc541dc37968
Attachment #8949372 -
Flags: review?(mconley)
Assignee | ||
Updated•6 years ago
|
Attachment #8947486 -
Attachment is obsolete: true
Reporter | ||
Comment 35•6 years ago
|
||
Comment on attachment 8949372 [details] [diff] [review] Patch Review of attachment 8949372 [details] [diff] [review]: ----------------------------------------------------------------- This seems great, thanks florian - I just have a few questions. Once I get those cleared up, I think I'd be comfortable giving an r+. ::: browser/components/nsBrowserGlue.js @@ +24,5 @@ > + > + let screenX = getValue("screenX"); > + let screenY = getValue("screenY"); > + let browserWindowFeatures = > + "chrome,all,dialog=no,extrachrome,menubar,resizable,scrollbars,status," + Are all of these features necessary? I don't, for example, see these features being necessary in nsBrowserContentHandler, so why do we need them here? @@ +31,5 @@ > + > + if (Services.prefs.getBoolPref("browser.suppress_first_window_animation")) > + browserWindowFeatures += ",suppressanimation"; > + > + let win = Services.ww.openWindow(null, "about:blank", null, I guess there's no way to bypass the creation of the content viewer with some kind of nodefaultsrc hackery? @@ +45,5 @@ > + if (AppConstants.platform != "macosx") { > + // On Windows/Linux the position is in device pixels rather than CSS pixels. > + let scale = win.devicePixelRatio; > + if (scale > 1) > + win.moveTo(screenX / scale, screenY / scale); Should we collect this first, and pass the scaled screenX and screenY in the features string? If not, why not? Certainly that'd be less work than opening and moving? Is this related to the "growing" bug that you describe later in this function?
Attachment #8949372 -
Flags: review?(mconley)
Assignee | ||
Comment 36•6 years ago
|
||
(In reply to Mike Conley (:mconley) (:⚙️) from comment #35) > Comment on attachment 8949372 [details] [diff] [review] > Patch > > Review of attachment 8949372 [details] [diff] [review]: > ----------------------------------------------------------------- > > This seems great, thanks florian - I just have a few questions. Once I get > those cleared up, I think I'd be comfortable giving an r+. > > ::: browser/components/nsBrowserGlue.js > @@ +24,5 @@ > > + > > + let screenX = getValue("screenX"); > > + let screenY = getValue("screenY"); > > + let browserWindowFeatures = > > + "chrome,all,dialog=no,extrachrome,menubar,resizable,scrollbars,status," + > > Are all of these features necessary? I don't, for example, see these > features being necessary in nsBrowserContentHandler, so why do we need them > here? I don't fully remember. I started with a copy/paste of something (looks like at least "chrome,all,dialog=no" is everywhere). I remember I had to add "extrachrome" or opening the bookmarks toolbar was impossible. I had to add "resizable" or the window wasn't resizable by default. I don't remember why I added menubar (but it's likely the same behavior as for the bookmarks toolbar) scrollbars and status. How much do you care? > @@ +31,5 @@ > > + > > + if (Services.prefs.getBoolPref("browser.suppress_first_window_animation")) > > + browserWindowFeatures += ",suppressanimation"; > > + > > + let win = Services.ww.openWindow(null, "about:blank", null, > > I guess there's no way to bypass the creation of the content viewer with > some kind of nodefaultsrc hackery? Not that I am aware of, but if you have nice ideas, I'm happy to explore in follow-ups anything that could make this even faster. > @@ +45,5 @@ > > + if (AppConstants.platform != "macosx") { > > + // On Windows/Linux the position is in device pixels rather than CSS pixels. > > + let scale = win.devicePixelRatio; > > + if (scale > 1) > > + win.moveTo(screenX / scale, screenY / scale); > > Should we collect this first, and pass the scaled screenX and screenY in the > features string? If not, why not? You need access to the window object to get win.devicePixelRatio, which is the reason why I had to open the window first. > Certainly that'd be less work than opening > and moving? I don't think moving a window that has never been made visible would be expensive.
Assignee | ||
Updated•6 years ago
|
Attachment #8949372 -
Flags: review?(mconley)
Reporter | ||
Comment 37•6 years ago
|
||
Comment on attachment 8949372 [details] [diff] [review] Patch Thanks! (In reply to Florian Quèze [:florian] from comment #36) > I had to add "resizable" or the window wasn't resizable by default. > I don't remember why I added menubar (but it's likely the same behavior as > for the bookmarks toolbar) scrollbars and status. > How much do you care? I guess it just seems strange to ask for so many features from a window when nsBrowserContentHandler doesn't. Oh well, I guess.
Attachment #8949372 -
Flags: review?(mconley) → review+
Comment 38•6 years ago
|
||
Pushed by florian@queze.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf5dea887c3 Show about:blank as soon as possible during startup (pref'ed off), r=mconley.
Comment 39•6 years ago
|
||
Removing 'meta' since a patch has landed here. Followups should block this bug regardless.
Keywords: meta
Summary: Show a window as soon as possible after start-up, even if it's not showing any UI yet. → Show a blank window as soon as possible after start-up (pref'd off)
Comment 40•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2cf5dea887c3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Comment 41•6 years ago
|
||
I have reproduced this bug on nightly 54.0a1 (2017-02-02) on windows 10, 64 bit. The bug's fix is now verified on Latest Nightly Build ID 20180216223539 User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0 [testday-20180216]
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Type: defect → enhancement
Updated•2 years ago
|
Updated•2 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•