394887 - Consider creating widgets for nsMenuPopupFrames lazily

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Description

[:Gavin Sharp [email: [e-mail]]]

See bug 391385 comment 11. Adding empty <panel>s to the browser chrome seems to have a fairly large impact on Txul. Widgets are already created lazily for popups of type ePopupTypeMenu, but Neil suggested we might be able to lazily create widgets for other of menus as well. This could help reduce the performance impact of bug 391385 and hopefully bug 383183.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: naive attempt

This is what I tried locally, from a quick glance at the code you mentioned. It seems to cause sizing problems when autocomplete popups are initially displayed (they're fine once they've been shown once).

Comment 2

[Neil Deakin]

Yes, the autocomplete uses 'sizetopopup', yet it isn't the parent of the popup frame (a popupset is in-between for some reason), so it never ends up checking the sizetopopup condition below.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Ah, I see. Should we search the parent chain, instead of just checking the direct parent?

Comment 4

[Neil Deakin]

(In reply to comment #3)
> Ah, I see. Should we search the parent chain, instead of just checking the
> direct parent?
> 

That would work for some cases, but some autocomplete popups (such as those added in 391385) are outside of the autocomplete widget. In reality, the popup code doesn't know what will be used as the anchor, if anything, at the time the frame is constructed.

The autocomplete widget doesn't even use sizetopopup properly (see bug 387548). One possible way is to add a sizetomenu attribute (or another suitable attribute) on the popup which causes the widget to be pre-created.

Or, we could dive deeper into layout in order to find out why autocomplete needs the widget to be pre-created.

Comment 5

[Neil Deakin]

OK, the autocomplete widget doesn't work because it has this function 'adjustHeight' that is called before the popup is showing which uses the view and the row height to calculate the tree size. However, when no child frames are being constructed, this is null/0 so the height doesn't get determined properly.

I think the easiest solution is to add some attribute to a popup which can be checked to see if the widget creation should be skipped.

Comment 6

[Neil Deakin]

Attachment: create more popup widgets lazily

This creates widgets only for menus with sizetopopup and popups with a type attribute, such as the autocomplete widget.

This will need to be tested extensively to make sure it doesn't cause any regressions.

Comment 7

[Neil Deakin]

Attachment: also move IsLeaf into cpp file

Comment 8

[Neil Deakin]

Attachment: this is the real patch

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 280188 [details] [diff] [review]
this is the real patch

>Index: layout/xul/base/src/nsMenuPopupFrame.cpp
>+  if (!mGeneratedChildren) {

I'd reverse this test, return PR_FALSE if mGeneratedChildren, then outdent everything following.

>+    if (parentContent &&
>+        !parentContent->HasAttr(kNameSpaceID_None, nsGkAtoms::sizetopopup))
>+      return PR_TRUE;

And this part could be |return parentContent && !parentContent->HasAttr(....)|;

>+  }
>+
>+   return PR_FALSE;
>+}

This was indented wrong; watch the indents as you outdent?

r+sr=bzbarsky with that.

Comment 10

[Neil Deakin]

Attachment: address comments

Comment 11

[Neil Deakin]

OK, I tried checking this in and it caused a large performance regression. I investigated and discovered that the bookmarks menus use type="places", causing these to be generated upfront which is fairly bad. So I'll either need to specifically look for type="autocomplete" or find some other mechanism which filters out the right items.

Comment 12

[Neil Deakin]

I checked in a modified version of this patch which moved the type attribute check inside the check for non ePopupType_Menu so it only checks the type for menus. TXul is better (5-6ms) and startup is a bit better, although strangely not on Windows. I'm going to mark this fixed.