Closed Bug 792296 Opened 7 years ago Closed 7 years ago

popup.xml's state getter should try to avoid flushing frames

Categories

(Core :: XUL, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: ehsan, Assigned: enndeakin)

References

Details

(Whiteboard: [Snappy])

Attachments

(1 file, 2 obsolete files)

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?
Blocks: 752496
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
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.
Blocks: 792922
(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.  :-)
Attachment #662571 - Attachment is obsolete: true
Attachment #663530 - Flags: review?(neil)
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.
Attached patch patch, v2Splinter Review
Attachment #663530 - Attachment is obsolete: true
Attachment #663530 - Flags: review?(neil)
Attachment #664118 - Flags: review?(neil)
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.
Attachment #664118 - Flags: review?(neil) → review+
https://hg.mozilla.org/mozilla-central/rev/ddfc1c5c777a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 807404
You need to log in before you can comment on or make changes to this bug.