211308 - remove style hint parameter from AttributeChanged notifications

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think we can remove the style hint parameter from AttributeChanged
notifications.  I'll attach a patch that does this.  (It does it for
nsIFrame::AttributeChanged too -- although we should really remove
nsIFrame::AttributeChanged, since pretty much by definition any use of it is a
design flaw)

The following changes within the patch are of interest -- the rest is boring:

 * nsXULElement.cpp, where I'm removing an optimization that shouldn't be
necessary anymore now that I've landed bug 209733 (this is the reason I did bug
209733)

 * removing some very minor optimizations that probably never help anything in
nsTextControlFrame.cpp (major design problems here, though! -- and I almost have
a patch to fix them).  This does change when we call
nsBoxFrame::AttributeChanged, though, thanks to a helpful |else| that probably
shouldn't be there.

 * changing nsCSSFrameConstructor::AttributeChanged not to use the hint (the
hope is that this will speed up a bunch of cases where the property-based hints
were too inaccurate and forced us to do to much work).  I should attach a diff
-uw of this file, since it involves some reindenting.  (This will also make it
much easier to move towards new types of hints, such as view-change and
abs-pos-move.)

 * some ifdef-DEBUG changes in svg, where I used printf as designed, instead of
one-piece-at-a-time.

 * nsSliderFrame, where I removed an optimization of dubious value similar to
the one in nsTextControlFrame

 *

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: nsCSSFrameConstructor.cpp changes (diff -uw)

Note that there are some things here that change from an unknown hint to a none
hint.  I'm hoping this will allow us to remove the unknown hint.

Comment 3

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

> we should really remove nsIFrame::AttributeChanged, since pretty much by
> definition any use of it is a design flaw

Hmm. I recently changed nsGfxScrollFrame to receive updates to scrollbar
'curpos' attributes via nsSliderFrame::AttributeChanged (it used to use a
document listener! urk). How do you think that should be done instead?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't see why the slider should work by attributes at all, rather than by some
API.  Perhaps some of the XUL uses are acceptable, though, since some XUL
elements are essentially formatting objects.  However, it doesn't make sense for
HTML attribute handling to be implemented in frames when display types can be
changed.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This patch causes a bunch of regressions because of
nsXULElement::GetMappedAttributeImpact (and perhaps something else related to
the nsCSSFrameConstructor changes).

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This adds a little more code removal in nsGenericHTMLElement.cpp and
nsCSSDeclaration.{h,cpp}.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I should undo the changes to nsCSSFrameConstructor::AttributeChanged, rename
nsIStyledContent::GetMappedAttributeImpact to GetAttributeChangeHint (only for
things not reflected in the style context), and separate out most of the HTML
implementations of GetMappedAttributeImpact into IsAttributeMapped.  I think we
could also move a good bit of nsXULElement::GetMappedAttributeImpact (the
preventative part) into nsBoxFrame::AttributeChanged.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, never mind the last sentence.  I think those optimizations were
designed to prevent style reresolution.  I'm guessing they were written before
shaver wrote AttributeAffectsStyle (now HasAttributeDependentStyle).

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Another change of interest in my next patch (I just saw a mistake and then
realized there were some other things I wanted to do) will be the change to
nsHTMLOptionElement.cpp, although I need to figure out what bryner was trying to
do in bug 130115.

I also want to remove eChangeHint_None (since I find a zero-constant odd for a
bitfield -- I think it's much clearer to use |0|) and Unknown/UNKNOWN hints.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, actually, now that I see what bryner's change was, it's not the change from
that patch, but from the initial XBL select landing (bug 112713).

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmm.  There seems to be a very slight page load slowdown:

without patch (but with other patches in my main tree):
                                                                                
    Test id: 3F09B3DF9B
    Avg. Median : 417 msec      Minimum     : 171 msec
    Average     : 418 msec      Maximum     : 1251 msec
                                                                                
    Test id: 3F09CF7413
    Avg. Median : 417 msec      Minimum     : 174 msec
    Average     : 417 msec      Maximum     : 1223 msec
                                                                                
with patch:
                                                                                
    Test id: 3F09B80183
    Avg. Median : 421 msec      Minimum     : 182 msec
    Average     : 421 msec      Maximum     : 1225 msec
                                                                                
    Test id: 3F09D1016D
    Avg. Median : 422 msec      Minimum     : 173 msec
    Average     : 423 msec      Maximum     : 1227 msec

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

My baseline tree had the patch for bug 210550 in it, which showed the
improvement in bug 210550 comment 3, so perhaps there isn't a problem.

I need to try a clean tree...

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

New numbers, which show no significant difference:

baseline:
                                                                                
    Test id: 3F0B90F1F6
    Avg. Median : 421 msec      Minimum     : 183 msec
    Average     : 422 msec      Maximum     : 1231 msec
                                                                                
    Test id: 3F0B97B532
    Avg. Median : 427 msec      Minimum     : 199 msec
    Average     : 440 msec      Maximum     : 1481 msec
                                                                                
    Test id: 3F0B9C9566
    Avg. Median : 425 msec      Minimum     : 180 msec
    Average     : 425 msec      Maximum     : 1237 msec
                                                                                
with patch:
                                                                                
    Test id: 3F0B9467E9
    Avg. Median : 422 msec      Minimum     : 178 msec
    Average     : 425 msec      Maximum     : 1233 msec
                                                                                
    Test id: 3F0B9A015F
    Avg. Median : 422 msec      Minimum     : 177 msec
    Average     : 420 msec      Maximum     : 1203 msec
                                                                                
    Test id: 3F0B9F1767
    Avg. Median : 424 msec      Minimum     : 187 msec
    Average     : 427 msec      Maximum     : 1235 msec

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127069 [details] [diff] [review]
patch

>Index: content/html/content/src/nsGenericHTMLElement.cpp

>+  // XXXldb What's this doing here?
>+  { &nsHTMLAtoms::_baseHref },

I believe the idea is that if that changes the :visited state of the node could
change or something like that....

Imo, that could be safely removed -- anyone dynamically mutation that attr had
better not be expecting anything good to come of it.

>Index: content/html/content/src/nsGenericHTMLElement.h

>+  static const AttributeDependenceEntry sImageMarginPositionAttributeMap[];

Wouldn't |sImageMarginSizeAttributeMap[]| be a better name?

>Index: content/html/content/src/nsHTMLFrameSetElement.cpp

>+  nsresult rv =
>+    nsGenericElement::GetAttributeChangeHint(aAttribute, aModType, aHint);

Shouldn't that be nsGenericHTMLContainerElement::GetAttributeChangeHint (i.e.
call the immediate superclass)?

>Index: content/html/content/src/nsHTMLHRElement.cpp
>+  nsresult rv =
>+    nsGenericElement::GetAttributeChangeHint(aAttribute, aModType, aHint);

nsGenericHTMLLeafElement::GetAttributeChangeHint

>Index: content/html/content/src/nsHTMLImageElement.cpp

>+  nsresult rv =
>+    nsGenericElement::GetAttributeChangeHint(aAttribute, aModType, aHint);

nsGenericHTMLLeafElement::GetAttributeChangeHint

>Index: content/html/content/src/nsHTMLInputElement.cpp

>+  nsresult rv =
>+    nsGenericElement::GetAttributeChangeHint(aAttribute, aModType, aHint);

nsGenericHTMLLeafFormElement::GetAttributeChangeHint

>Index: content/html/content/src/nsHTMLMapElement.cpp

>+  nsresult rv =
>+    nsGenericElement::GetAttributeChangeHint(aAttribute, aModType, aHint);

nsGenericHTMLContainerElement::GetAttributeChangeHint

>Index: content/html/content/src/nsHTMLOptionElement.cpp

>+  nsresult rv =
>+    nsGenericElement::GetAttributeChangeHint(aAttribute, aModType, aHint);

nsGenericHTMLContainerElement::GetAttributeChangeHint

>Index: content/html/content/src/nsHTMLPreElement.cpp

>+  nsresult rv =
>+    nsGenericElement::GetAttributeChangeHint(aAttribute, aModType, aHint);

nsGenericHTMLContainerElement::GetAttributeChangeHint

>Index: content/html/content/src/nsHTMLSelectElement.cpp

>+  nsresult rv =
>+    nsGenericElement::GetAttributeChangeHint(aAttribute, aModType, aHint);

nsGenericHTMLContainerFormElement::GetAttributeChangeHint

>Index: content/html/content/src/nsHTMLSharedContainerElem.cpp

>+  PRBool result = nsGenericHTMLContainerElement::HasAttributeDependentStyle(aAttr);
>+

How about:

if (nsGenericHTMLContainerElement::HasAttributeDependentStyle(aAttr)) {
  return PR_TRUE;
}

and similarly throughout this method?

>Index: content/html/content/src/nsHTMLTextAreaElement.cpp

>+  nsresult rv =
>+    nsGenericElement::GetAttributeChangeHint(aAttribute, aModType, 

nsGenericHTMLContainerFormElement::GetAttributeChangeHint


>Index: layout/html/forms/src/nsTextControlFrame.cpp

>   if (nsHTMLAtoms::value == aAttribute) 
>   {
>     // XXX If this should happen when value= attribute is set, shouldn't it
>     // happen when .value is set too?
>-    if (aHint != NS_STYLE_HINT_REFLOW)
>-      nsFormControlHelper::StyleChangeReflow(aPresContext, this);
>+    nsFormControlHelper::StyleChangeReflow(aPresContext, this);

Is this still needed?  The <input> element now gives a change hint of REFLOW
for the value attr, and value does not apply to textarea.

>-  else if ((nsHTMLAtoms::size == aAttribute ||
>-            nsHTMLAtoms::rows == aAttribute ||
>-            nsHTMLAtoms::cols == aAttribute) && aHint != NS_STYLE_HINT_REFLOW) {
>+  else if (nsHTMLAtoms::size == aAttribute ||
>+           nsHTMLAtoms::rows == aAttribute ||
>+           nsHTMLAtoms::cols == aAttribute) {

Same thing.  <textarea> already returns a REFLOW hint for rows/cols and <input>
should just do the same for size.

>Index: layout/html/style/src/nsCSSFrameConstructor.cpp
>+    else {  // no frame now, possibly generate one with new style data
>+      if (affects || hint != 0)
>+        result = MaybeRecreateFramesForContent(aPresContext, aContent);

Wouldn't that be:

if (affects || (hint & (nsChangeHint_ReconstructFrame |
			nsChangeHint_ReconstructDoc))

?  After all, a hint that's smaller than those won't cause frames to suddenly
appear....

r+sr=me with these minor changes.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This should address all of the comments in comment 16.	I made two additional
changes:

> Wouldn't |sImageMarginSizeAttributeMap[]| be a better name?

I also changed MapImagePositionAttributeInto to MapImageSizeAttributesInto.

> Wouldn't that be:
>
> if (affects || (hint & (nsChangeHint_ReconstructFrame |
>			nsChangeHint_ReconstructDoc))
>
> ?  After all, a hint that's smaller than those won't cause frames to suddenly

> appear....

Except those cases are already handled in the outer if-else chain, so I just
changed it to "else if (affects)" and removed the reindentation.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, I'm going to take out the |else if (affects)| change completely, since
the checks in MaybeRecreateFramesForContent should be a good bit faster than
HasAttributeDependentStyle, which is mainly useful because it prevents
reresolution of the whole subtree.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2003-07-11 14:14/15/16 -0700.

Comment 21

[Scott MacGregor]

Could this change have caused the regression in:

http://bugzilla.mozilla.org/show_bug.cgi?id=212575 ? 

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, as I already said in bug 212579 comment 5 (I tested for bug 212575 as well).

Comment 23

[Aleksey Nogin]

In some places the "hint" variable was left around (see, for example,
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/style/src/nsHTMLStyleSheet.cpp&mark=876#866
) causing an "unused variable" warning.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Fixed.