69142 - Setting attributes on menus thrashes in RecreateFrameForContent on MacOS

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mike Pinkerton (not reading bugmail)]

As part of profiling window activation (when a window comes to the fg), i noticed 
that 50% of activation processing was spent in RecreateFrameForContent() after 
setting attributes on content nodes representing menu items.

Recall that on the mac, menus on the menubar are native and have a rule that 
makes them display:none. This won't ever change, yet the style system, upon 
seeing the attributeChanged message, tries its hardest to squeeze out a frame, 
wastes lots of time, and eventually ends up back at the start w/out a frame.

50% of window activate translates into 25% of window refresh time (the time from 
when you click in a bg window until the paint completes updating all the areas 
damaged from bringing the window to the front). This is very noticable on mac 
(flashing) and contributes to it feeling sluggish. 

ideas?

Comment 1

[Marc Attinasi]

Hmm. I wonder if we can optimize the computation of a style change's impact such
that display:none causes nothing to happen. Sounds like it is worth looking into.

So, what attribute is changing? Is there some style rule that has an attribute
selector with that attribute? If not, I wonder why shaver's optimization isn't
kicking in (the optimization to skip attribute changed notification reresolves
if there is not rule based on the attrib).

Comment 2

[Mike Pinkerton (not reading bugmail)]

there are rules based on that attribute for _some_ menus, just not those on the 
menubar. i think shaver's optimizations fall by the wayside when anyone, 
anywhere, uses a rule with that attribute. what say you, shaver?

I'm guessing the attribute is the "disabled" attribute, since we're updating 
menu/command state when bringing the window to the fg. 

Comment 3

[David Hyatt]

I have a patch that takes this off the map.  Basically, I looked at the code and
you can't stop RecreateFramesForContent from being called.  It's correct that
the code calls it.

The only reason the method is slow in the case where you have no parent frame is
that ContentInserted is stupid.  I have a simple patch to ContentInserted that
makes it much smarter about not doing any work when the parent content node has
no frame.  This takes RecreateFramesForContent off the map on my Quantify runs.

Comment 4

[David Hyatt]

Attachment: Make ContentInserted smarter

Comment 5

[David Hyatt]

Looking for r and sr.

Comment 6

[Marc Attinasi]

Patch looks good to me r/sr=attinasi

Comment 7

[Chris Waterson]

Make a note about why the container might be null (e.g., ``this happens if we're
inserting into an element that is currently mapped `display: none', for
example''). Do that, sr=waterson

Comment 8

[David Hyatt]

Fix was checked in ages ago.  Not sure how this bug stayed open.