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

RESOLVED FIXED in mozilla19

Status

()

Core
XUL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: Neil Deakin)

Tracking

Trunk
mozilla19
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy])

Attachments

(1 attachment, 2 obsolete attachments)

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?

Updated

5 years ago
Blocks: 752496
(Assignee)

Comment 1

5 years ago
Created attachment 662571 [details] [diff] [review]
don't flush when retrieving the state
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?
(Assignee)

Comment 3

5 years ago
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.  :-)
(Assignee)

Comment 5

5 years ago
Created attachment 663530 [details] [diff] [review]
patch that doesn't flush for other methods
Attachment #662571 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #663530 - Flags: review?(neil)
(Assignee)

Comment 6

5 years ago
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 7

5 years ago
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.
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
Created attachment 664118 [details] [diff] [review]
patch, v2
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
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

5 years ago
Depends on: 807404
You need to log in before you can comment on or make changes to this bug.