Closed Bug 394887 Opened 12 years ago Closed 12 years ago

Consider creating widgets for nsMenuPopupFrames lazily

Categories

(Core :: XUL, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9beta1

People

(Reporter: Gavin, Assigned: enndeakin)

References

(Depends on 1 open bug)

Details

(Keywords: perf)

Attachments

(2 files, 3 obsolete files)

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.
Attached patch naive attempt (obsolete) — Splinter Review
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).
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.
Ah, I see. Should we search the parent chain, instead of just checking the direct parent?
(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.
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.
Attached patch create more popup widgets lazily (obsolete) — Splinter Review
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.
Assignee: jag → enndeakin
Attachment #279620 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Summary: Consider create widgets for nsMenuPopupFrames lazily → Consider creating widgets for nsMenuPopupFrames lazily
Attached patch also move IsLeaf into cpp file (obsolete) — Splinter Review
Attachment #279956 - Attachment is obsolete: true
Attachment #280187 - Flags: superreview?(bzbarsky)
Attachment #280187 - Flags: review?(bzbarsky)
Attachment #280187 - Attachment is obsolete: true
Attachment #280188 - Flags: superreview?(bzbarsky)
Attachment #280188 - Flags: review?(bzbarsky)
Attachment #280187 - Flags: superreview?(bzbarsky)
Attachment #280187 - Flags: review?(bzbarsky)
Blocks: 395980
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.
Attachment #280188 - Flags: superreview?(bzbarsky)
Attachment #280188 - Flags: superreview+
Attachment #280188 - Flags: review?(bzbarsky)
Attachment #280188 - Flags: review+
Attached patch address commentsSplinter Review
Attachment #280188 - Flags: approval1.9?
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.
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.

Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 396412
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9 M9
Keywords: perf
You need to log in before you can comment on or make changes to this bug.