386380 - Txul regresssion after XUL popup rewrite landed

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Since the landing of Neil's XUL popup rewrite, Txul regressed by 3-7% across the board.

Comment 1

[Neil Deakin]

Likely caused by the creation of the popup frames (and more likely the views/widgets) right away rather than when they are opened. Might be possible to delay the widget creation until later though.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, the widgets are what I'd guess to be the most likely problem...

Comment 3

[Neil Deakin]

Attachment: create widgets later

This patch is the differences from the patch in bug 279703.

It cleans up the view creation code so that it creates a hidden view at the right  position rather than creating one and fiddling with it, and moves the widget code   so that widgets are created just before the popup opens.

This change improves the performance regression on my two machines, although I can't tell whether it fixes the regression completely and the results I'm getting are just noise, or whether there is still some work to be done.

I guess roc would be a suitable reviewer if bz is away, would be good to get this reviewed soon.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 270664 [details] [diff] [review]
create widgets later

++  nsIView* ourView = GetView();
++  if (!ourView->HasWidget()) {
++    CreateWidgetForView(ourView);
++  }

Share this in an EnsureWidget helper function? That function should assert that the frame has no child frames, since trying to do this when the frame might have children with their own widgets would be disastrous.

Comment 5

[Neil Deakin]

Attachment: Found an issue with the above patch

An issue with the patch above is that submenus don't open properly. As they haven't had widgets created yet, the code in nsContainerFrame::SyncFrameViewProperties defaults to thinking that the view is visible, thus, not adjusting it properly when the view really is visible.

Not sure if the check for (windowType == eWindowType_popup) can just be an assertion.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 270731 [details] [diff] [review]
Found an issue with the above patch

Yeah, I think the eWindowType_popup check can just be an assertion

Comment 7

[Neil Deakin]

These patches look to have fixed the txul regression, so marking this fixed.