I've seen this in tons of profiles. popup.xml's state getter is unbelieably expensive since it flushes by calling nsBoxObject::GetFrame(false) which flushes frames <http://dxr.allizom.org/mozilla-central/layout/xul/base/src/nsBoxObject.cpp#l112>. Can we somehow avoid flushing there?

Thanks for your patch, Neil! That was fast :-) So here's a question, there's a bunch of other places in this code where we call GetFrame. Can we avoid this in some of those other cases as well?

Probably only matters for setConsumeRollupEvent (newer code should use the consumeoutsideclicks attribute anyway) and outerScreenRect, as the former is expected to be called before opening the popup and the latter to be consistent with other coordinate getting methods. The remainder you probably wouldn't call while the popup is open.

(In reply to comment #3) > Probably only matters for setConsumeRollupEvent (newer code should use the > consumeoutsideclicks attribute anyway) and outerScreenRect, as the former is > expected to be called before opening the popup and the latter to be consistent > with other coordinate getting methods. Can you please file bugs on those? I'm not really familiar with what those names mean. :-)

The patch above causes some failures in the private browsing tests, but these are fixed by the patch in bug 621944, which by a wild coincidence, I happened to be working on the same day.

Comment on attachment 663530 [details] [diff] [review] patch that doesn't flush for other methods > NS_IMETHODIMP > nsPopupBoxObject::GetAutoPosition(bool* aShouldAutoPosition) > { >- nsMenuPopupFrame *menuPopupFrame = do_QueryFrame(GetFrame(false)); >+ nsMenuPopupFrame *menuPopupFrame = mContent ? do_QueryFrame(mContent->GetPrimaryFrame()) : nullptr; > if (menuPopupFrame) { > *aShouldAutoPosition = menuPopupFrame->GetAutoPosition(); > } > > return NS_OK; > } Shouldn't this have a default value? The other getters seem to have one already.

(In reply to neil@parkwaycc.co.uk from comment #7) > Comment on attachment 663530 [details] [diff] [review] > patch that doesn't flush for other methods > > > NS_IMETHODIMP > > nsPopupBoxObject::GetAutoPosition(bool* aShouldAutoPosition) > > { > >- nsMenuPopupFrame *menuPopupFrame = do_QueryFrame(GetFrame(false)); > >+ nsMenuPopupFrame *menuPopupFrame = mContent ? do_QueryFrame(mContent->GetPrimaryFrame()) : nullptr; > > if (menuPopupFrame) { > > *aShouldAutoPosition = menuPopupFrame->GetAutoPosition(); > > } > > > > return NS_OK; > > } > Shouldn't this have a default value? The other getters seem to have one > already. I will add one.

Comment on attachment 664118 [details] [diff] [review] patch, v2 Sorry for the delay. >- nsMenuPopupFrame *menuPopupFrame = do_QueryFrame(GetFrame(false)); >+ nsMenuPopupFrame *menuPopupFrame = mContent ? do_QueryFrame(mContent->GetPrimaryFrame()) : nullptr; ... >- nsMenuPopupFrame *menuPopupFrame = do_QueryFrame(GetFrame(false)); >+ nsMenuPopupFrame *menuPopupFrame = do_QueryFrame(mContent->GetPrimaryFrame()); I assume that this one should get the ?: treatment too? r=me with that fixed.