I've noticed that switching in and out of customize mode via tab switch is much, much smoother than opening a new tab. It's also smoother than entering "about:customizing" into a blank tab and pressing Enter. One hypothesis is that the overhead of loading about:customizing (which is essentially a blank document) is hurting us here. We might be able to re-use some of our about:newtab preloading machinery to make this better, assuming that this is indeed where some of the problem is. I'll whip up a POC to see what this would buy us.

Preliminary testing here is pretty promising! Screencaps coming up.

With preloading: http://www.screencast.com/t/NKmDCLaux4f Without preloading: http://www.screencast.com/t/UJHtZktRlaMy Pretty substantial difference! Also, this should make things better across all platforms, not just Windows. I think this is definitely worth pursuing. Marking as a P2, since I think this and bug 974607 are our best bets for Windows smoothness in the short term. Tim - in your estimation, how hard would it be to rig the BrowserNewTabPreloader stuff to accomodate another tab?

Why would we load this page in the first place? Can't we cancel the initial about:blank load and manually set a label and favicon for the tab?

(In reply to Dão Gottwald [:dao] from comment #3) > Why would we load this page in the first place? Can't we cancel the initial > about:blank load and manually set a label and favicon for the tab? That actually might be the better choice, if it's relatively painless to get tabbrowser to bail out of creating a full-on browser/docshell for about:customizing. That might be worth looking into, as I wager that might be simpler than doing the preload trick.

(In reply to Mike Conley (:mconley) from comment #4) > (In reply to Dão Gottwald [:dao] from comment #3) > > Why would we load this page in the first place? Can't we cancel the initial > > about:blank load and manually set a label and favicon for the tab? > > That actually might be the better choice, if it's relatively painless to get > tabbrowser to bail out of creating a full-on browser/docshell for > about:customizing. Please note that calling browser.stop() can also reflow. It would be better to not start loading about:blank at all, not sure if that would break expectations for the tab's <browser> if accessed by other code. Bug 878747 might be of interest.

Thanks Tim! I just attempted switching aboutCustomizing.xul to aboutCustomizing.html, and this did not appear to affect performance much. I'm now going to focus on seeing if we can avoid loading a browser altogether.

Alright, some notes on how we enter customize mode: Customize mode can be entered in several ways - the primary mechanism is a call to gCustomizeMode.enter(), which is fired by both the "Customize" button in the menu panel, the customize entry in the menu bar, and the customize menuitem in the toolbar context menu. The user can also optionally enter customize mode by browsing to about:customizing. The first method of entering is actually just a wrapper around the second - gCustomizeMode.enter() first checks to see if the current browser's URI matches about:customizing. If not, it switches to (or opens) that tab, and then returns. If it is, we keep going and enter the mode. The last piece of the puzzle is an onLocationChange listener in browser.js. We have a location listener set on the tabbrowser element so that if we ever open (or switch to) about:customizing, we'll detect it, and call gCustomizeMode (which will determine that we're at about:customizing, and enter the mode). It's all quite circuitous.

Having fiddled with tabbrowser.xml today, I'm starting to feel like trying to special-case about:customizing to not load the about:customizing document opens us to more potential for regression - and at this stage of the game, I'm not looking to be too risky. I'm probably feeling this way because I'm not 100% familiar with how tabbrowser works, but the whole "loading pages" thing feels like something I'd rather not monkey with on Aurora.

If we don't want to load a page, we should also get rid of the about:customizing URI and most of the complexity you described in comment 7.

(In reply to Dão Gottwald [:dao] from comment #9) > If we don't want to load a page, we should also get rid of the > about:customizing URI and most of the complexity you described in comment 7. Can I get more detail about what you're suggesting? Would customization mode still be associated with a tab? If so, how do you propose we open that tab without loading a URI (even about:blank)?

As this line of thinking (somehow sidestepping the expense of loading about:customizing) is probably our best bet at taking a huge chunk out of the transition cost, I'm marking this as a P2.

Needinfo'ing Dao from comment 10.

Creating a tab and using "nodefaultsrc" to not create a frame loader and not start a load at all would certainly prevent expensive work but might also break assumptions for add-ons and other code out there. It may be best to not associate customization mode with a tab. Or maybe rather a special "virtual tab"?

Just spoke to madhava about this - he said that it's not a priority to keep about:customizing as a destination that users can be linked to, but that it _is_ a goal to associate customization mode with a tab. (In reply to Tim Taubert [:ttaubert] from comment #13) > Creating a tab and using "nodefaultsrc" to not create a frame loader and not > start a load at all would certainly prevent expensive work but might also > break assumptions for add-ons and other code out there. It may be best to > not associate customization mode with a tab. Or maybe rather a special > "virtual tab"? At this late point, I'm also worried about breaking assumptions - especially in something so central and important as tabbrowser.

So, ttaubert and I just had a chat about all of this in IRC, and here's where we've gotten to: 1) We have two options - either preloading about:customizing, or avoiding the load of about:customizing altogether since it's a decidedly useless page. 2) UX has stated that it's not necessary for about:customizing to be a destination that one reaches via the AwesomeBar or via links, so that makes about:customizing even more useless. 3) UX has also stated, however, that it is important for the Customization mode to be associated with a particular tab. Tabs are a conceptual model that users are already used to, and allows us to have a "mode" switch without adding too much in the way of cognitive overhead. Having a tab be associated with customizing can be done via an attribute, or some JS property, or whathaveyou. Unfortunately, it'd also mean using nodefaultsrc and some other modifications to tabbrowser.xml to avoid doing the load altogether. That sounds decidedly dangerous as a road to take on the Aurora train (and we're already half-way through the cycle!). So I think we're going to opt for safety on this one, and try the preload route. This does, unfortunately, mean small but non-zero memory overhead on each window, but it does allow us to achieve our goal with minimal changes to tabbrowser.xml - we'd be (as far as I can tell) encapsulating any regressions solely within the customization mode, and not risk leaking them out into tabbrowser where they effect the primary function of the software.

ttaubert was kind enough to put together a patch for this: https://github.com/ttaubert/gecko-dev/commit/ac234f0c98f95f8bfc6ef991bc3995bcf2b3b3a9.patch I've pushed to try to see what it does to CART (though local testing smooths things out a bunch): Baseline: https://tbpl.mozilla.org/?tree=Try&rev=21e77b801df3 Patch: https://tbpl.mozilla.org/?tree=Try&rev=d7cd73afeb01 Compare talos: http://compare-talos.mattn.ca/?oldRevs=21e77b801df3&newRev=d7cd73afeb01&server=graphs.mozilla.org&submit=true

Did you test the hypothesis that loading about:blank has a similar overhead?

(In reply to Dão Gottwald [:dao] from comment #17) > Did you test the hypothesis that loading about:blank has a similar overhead? I did, in that I altered AboutRedirector.cpp to point about:customizing at about:blank. I assume this is a valid test. If that's the case, then loading about:blank didn't help.

Tim got us started here, and I'm going to try to drive this one over the line.

So, I've tested this, and I think it puts us in the right place. The only drawback, besides the overhead of loading the document in the background, is that it seems to exacerbate bug 975534 - even after waiting for the transition to complete, one needs to wait about 1-2 seconds before exiting customize mode can be smooth. I'll look into what's going on there next. In the meantime, checkpointing the patch here.

Comment on attachment 8381710 [details] [diff] [review] Patch v1 Whoops - didn't mean to request review just yet.

Comment on attachment 8381710 [details] [diff] [review] Patch v1 Alright, I don't think this makes things worse with bug 975534 - I think it's the same behaviour as before. So I'm ready to have somebody look at this. Not asking for feedback from ttaubert, because this is essentially the same patch he posted (except that I altered a comment to not refer to about:newtab, and adjusted PRELOADER_INIT_DELAY_MS back to 5000 as ttaubert suggested).

Comment on attachment 8381710 [details] [diff] [review] Patch v1 Review of attachment 8381710 [details] [diff] [review]: ----------------------------------------------------------------- This is mostly fine except the code that handles multiple hidden browsers. It doesn't seem necessary and can probably be removed. ::: browser/modules/CustomizationTabPreloader.jsm @@ +20,5 @@ > + > +// The interval between swapping in a preload docShell and kicking off the > +// next preload in the background. > +const PRELOADER_INTERVAL_MS = 600; > +// The initial delay before we start preloading our first new tab page. The s/first new tab/customization/ @@ +104,5 @@ > + HiddenBrowsers.init(); > + } > +}; > + > +let HiddenBrowsers = { Do we ever have multiple hidden browsers? We only ever show one customization mode page per browser session, so preloading multiple of them doesn't seem like a useful feature. @@ +170,5 @@ > + > + observe: function () { > + this._timer = null; > + > + // Start pre-loading the new tab page. s/new tab/customization/

Thanks Jared - I'll see if I can get rid of the HiddenBrowsers object.

Alright, I've corrected the erroneous references to new tab, and gotten rid of the HiddenBrowsers object. I renamed CustomizationTabPreloader to CustomizationTabPreloaderInternal, and combined it with HiddenBrowsers, and then introduced a new frozen interface called CustomizationTabPreloader that calls into CustomizationTabPreloaderInternal.

Comment on attachment 8383732 [details] [diff] [review] Patch v1.1 Hey Tim, do my changes to your patch look sane? See my above comment for a description of the changes I made, but the interdiff should help spell it out.

Comment on attachment 8383732 [details] [diff] [review] Patch v1.1 Review of attachment 8383732 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Mike Conley (:mconley) from comment #27) > Hey Tim, do my changes to your patch look sane? They do :) The code is a lot clearer now. Should we file a follow-up about moving "HiddenBrowser" and "HostFrame" to a separate module so that they can be reused instead of duplicated? We could as well have a directory for all of that code, like browser/components/preloader. ::: browser/modules/CustomizationTabPreloader.jsm @@ +22,5 @@ > +// next preload in the background. > +const PRELOADER_INTERVAL_MS = 600; > +// The initial delay before we start preloading our first customization page. The > +// timer is started after the first 'browser-delayed-startup' has been sent. > +const PRELOADER_INIT_DELAY_MS = 5000; How about increasing that to 7s maybe? Or later? It seems that a user is more likely to enter customization mode event later than opening a new tab. That would leave some room and not preload those two at the same time.

(In reply to Tim Taubert [:ttaubert] from comment #28) > Comment on attachment 8383732 [details] [diff] [review] > Patch v1.1 > > Review of attachment 8383732 [details] [diff] [review]: > ----------------------------------------------------------------- > > (In reply to Mike Conley (:mconley) from comment #27) > > Hey Tim, do my changes to your patch look sane? > > They do :) The code is a lot clearer now. Should we file a follow-up about > moving "HiddenBrowser" and "HostFrame" to a separate module so that they can > be reused instead of duplicated? We could as well have a directory for all > of that code, like browser/components/preloader. Good idea - I'll file that next. > > ::: browser/modules/CustomizationTabPreloader.jsm > @@ +22,5 @@ > > +// next preload in the background. > > +const PRELOADER_INTERVAL_MS = 600; > > +// The initial delay before we start preloading our first customization page. The > > +// timer is started after the first 'browser-delayed-startup' has been sent. > > +const PRELOADER_INIT_DELAY_MS = 5000; > > How about increasing that to 7s maybe? Or later? It seems that a user is > more likely to enter customization mode event later than opening a new tab. > That would leave some room and not preload those two at the same time. Sure, done.

Final patch with 7s preload delay.

remote: https://hg.mozilla.org/integration/fx-team/rev/ae231333480e

Filed bug 978133 for DRYing up our preloaders.

Pushed a follow-up to fix "leaks": https://hg.mozilla.org/integration/fx-team/rev/2933b8f7e9f2

(In reply to Tim Taubert [:ttaubert] from comment #33) > Pushed a follow-up to fix "leaks": > > https://hg.mozilla.org/integration/fx-team/rev/2933b8f7e9f2 Ah, thanks for that ttaubert.

https://hg.mozilla.org/mozilla-central/rev/ae231333480e https://hg.mozilla.org/mozilla-central/rev/2933b8f7e9f2

Comment on attachment 8383755 [details] [diff] [review] Patch v1.2 (r+'d by jaws, ttaubert) [Approval Request Comment] Bug caused by (feature/regressing bug #): Australis User impact if declined: A slower customization mode transition. Testing completed (on m-c, etc.): Baking on mozilla-central for a day, and extensive local testing. Risk to taking this patch (and alternatives if risky): Very, very little. It is just CSS, and the rule only applies for a 150ms window. String or IDL/UUID changes made by this patch: None.

Whoops - that "risk" section was for bug 977796. For this bug, the risk is low because the change is almost entirely contained within the new CustomizationTabPreloader module - and if things go wrong with it, it falls back to not using the preloaded browser.

Aaaaand like bug 977796, I thought this was approved by Sylvestre, so it's a=sledru on the commit message instead of a=gavin. My bad. https://hg.mozilla.org/releases/mozilla-aurora/rev/547b83e590a6 https://hg.mozilla.org/releases/mozilla-aurora/rev/bca673f407de