And that makes the first call to wrapToolbarItem take 60ms, which causes us to skip four frames, if my math works out… Thoughts on how to fix it: 1) Have the PanelUI not resize itself on mutation if we're entering/leaving customize mode. 2) Use a document fragment to update all the toolbar items with their wrapped versions in one big swoop. As a side note, I wonder if this is the cause for question #1 in bug 873060…

I think we should try to slip a fix for this into the UR build if possible.

Hey Mike - can you maybe take a look at this?

(We probably also want to do something similar on the transition out of Customize mode, when we unwrap the items…)

Comment on attachment 761027 [details] [diff] [review] WIP: add transitioning property to CustomizeMode Review of attachment 761027 [details] [diff] [review]: ----------------------------------------------------------------- Just a drive-by - I don't think I like panelmultiview reaching into CustomizeMode like this. Lots of the UI "prepares" itself before we go into customization mode. You can see that happening in browser/base/content/browser-customization.js. I think we might want to do something similar here. Alter panelUI.js so that it has customizationStart and customizationEnd functions, and have those called within browser-customization.js. Those functions should do the manipulation of the panelmultiview - maybe calling a method on the minding, or (more likely) setting a property. Does that make sense?

Hey Mike, I needed this bug in order to work on bug 881909, and I did some fiddling with it. In the end, I think this is what I need. I call startBatchUpdate and endBatchUpdate in my (soon to be posted) patch in bug 881909. Hope it's cool if I yank this one from you! -Mike

Comment on attachment 761663 [details] [diff] [review] Patch v1 How does this look, Jared? I'll actually be calling beginBatchUpdate and endBatchUpdate in bug 881909.

Comment on attachment 761663 [details] [diff] [review] Patch v1 I've just realized that both this and my patch in bug 881909 don't apply cleanly to trunk. Let me fix that...

Updated the summary, since bug 881909 takes care of making sure we don't reflow when wrapping the items.

Comment on attachment 762099 [details] [diff] [review] Patch v1.2 What do you think of this, Jared?

Comment on attachment 762099 [details] [diff] [review] Patch v1.2 Review of attachment 762099 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/content/panelUI.xml @@ +88,5 @@ > <field name="_anchorElement">null</field> > <field name="_mainViewHeight">0</field> > <field name="_subViewObserver">null</field> > <field name="_transitioning">false</field> > + <field name="ignoreMutations">false</field> This can be a property that calls disconnect or observe when it is set. That would also allow us to keep syncContainerWith*View private. It could also kick off the sync when ignoreMutations becomes true.

Having ignoreMutations take care of connecting / disconnecting observers is kinda complicated...this is what it looks like. Maybe I've been looking at this performance stuff for too long, but this looks pretty complicated. :/ Perhaps we can have the mutation observer functions check ignoreMutations instead?

Ok let's just go with Patch v1.2. Thanks for trying my recommendation.

I liked your suggestion about re-hiding the _sync functions, and I think I've found a happy medium here.

Comment on attachment 762213 [details] [diff] [review] Patch v1.4 I *think* this is the least complicated solution. This way, in bug 881909, I can call ensureRegistered(true), and we don't call the _sync functions until CustomizeMode.enter/exit is ready. Sorry to make you review this thing yet again. :/

Comment on attachment 762213 [details] [diff] [review] Patch v1.4 Review of attachment 762213 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, this looks nice. ::: browser/components/customizableui/content/panelUI.js @@ +200,5 @@ > tempPanel.openPopup(iconAnchor || aAnchor, "bottomcenter topright"); > } > }, > > + /** nit, trailing whitespace

This seems to have broken sizing the menupanel to its contents. :-(

(In reply to :Gijs Kruitbosch from comment #20) > This seems to have broken sizing the menupanel to its contents. :-( We should back this out until we have a fix for the menupanel sizing.

Yep, backed out as: https://hg.mozilla.org/projects/ux/rev/0965605b441e

I think this bug was caused because we were trying to sync the panelview container heights too early, before the panel had even opened. We should only sync the container after ignoreMutations is set to false if the panel is open. If the panel is closed, the sync will happen automatically once the onpopupshowing event is fired.

Fixing some documentation. Going to re-land now.

Relanded as https://hg.mozilla.org/projects/ux/rev/cd6b6424d334