313968 - [FIX]Eliminate nsIStyledContent

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

And push the methods up to nsIContent.  See bug 313343 for the discussion on this.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Actual patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 201022 [details] [diff] [review]
Actual patch

I'm not that happy with the REQUIRES changes, but the only other option I see is to move nsChangeHint to content, which is also not so great...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 201022 [details] [diff] [review]
Actual patch

David, could you look over the nsCSSStyleSheet changes?  I ran into mHasAttributes being uninited when there was no mContent, which meant that the "[x]::-moz-viewport" selector made us crash...  Other than that and a null-check for matching :lang()::-moz-viewport, the changes should be pretty mechanical.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 201022 [details] [diff] [review]
Actual patch

sr=jst

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 201022 [details] [diff] [review]
Actual patch

>+  /**
>+   * Get the class list of this content node (this corresponds to the
>+   * value of the null-namespace attribute whose name is given by
>+   * GetClassAttributeName().  This may be null if there are no
>+   * classes.
>+   */
>+  virtual const nsAttrValue* GetClasses() const = 0;

Should we mention that this can be non-null even if there are no classes? (class="")

>+  /**
>+   * Walk aRuleWalker over the content style rules (presentational
>+   * hint rules) for this content node.
>+   */
>+  NS_IMETHOD WalkContentStyleRules(nsRuleWalker* aRuleWalker) = 0;

This, and some of the other moved methods, should use |virtual nsresult| and similar rather then NS_IMETHOD. Can be for some other time though.

>+  /**
>+   * Is the attribute named stored in the mapped attributes?
>+   *
>+   * This really belongs on nsIHTMLContent instead.
>+   * // XXXbz or we should push up attribute mapping to generic element...

These two comments seems obsolete.


>@@ -3234,17 +3230,17 @@ static PRBool SelectorMatches(RuleProces
>         
>         attr = attr->mNext;
>       } while (attr && result);
>     }
>   }
>   if (result && (aSelector->mIDList || aSelector->mClassList)) {
>     // test for ID & class match
>     result = localFalse;
>-    if (data.mStyledContent) {
>+    if (data.mContent) {

You don't need to ensure that this is an element here?

r=me with that answered

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Should we mention that this can be non-null even if there are no classes?

Yes.  Will do.

> This, and some of the other moved methods, should use |virtual nsresult|

Followup?  I wanted to not touch any actual implementations if I could avoid it...

> You don't need to ensure that this is an element here?

Not really.  The constructor for the |data| struct has:

2593   NS_ASSERTION(!aContent || aContent->IsContentOfType(nsIContent::eELEMENT),
2594                "non-element leaked into SelectorMatches");

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> Followup?  I wanted to not touch any actual implementations if I could avoid
> it...

absolutly

Comment 9

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

Comment on attachment 201022 [details] [diff] [review]
Actual patch

>Index: layout/style/nsCSSStyleSheet.cpp
>@@ -2627,25 +2626,19 @@ RuleProcessorData::RuleProcessorData(nsP

Feel free to remove the double-init of mContent here.

>-        else {
>+        else if (data.mContent) {
>           nsIDocument* doc = data.mContent->GetDocument();
>           if (doc) {
>             // Try to get the language from the HTTP header or if this

In what cases can these null checks fail?  Should we use a more reliable way of getting the document?

>   if (result && (aSelector->mIDList || aSelector->mClassList)) {
>     // test for ID & class match
>     result = localFalse;
>-    if (data.mStyledContent) {
>+    if (data.mContent) {

Should this test data.mHasAttributes instead?  Seems more efficient.  (Were there actually any cases in which we were running SelectorMatches on something that did not implement nsIStyledContent?  I can't think of any off the top of my head.)

Comment 10

[Boris Zbarsky [:bzbarsky]]

> In what cases can these null checks fail?

When resolving a pseudo-element style with a null parent content passed to ResolvePseudoStyleFor.  This happens for at least ::-moz-line-frame, ::-moz-frameset-blank, ::-moz-viewport, ::-moz-viewport-scroll, ::-moz-canvas, ::-moz-page-sequence, ::-moz-page, ::-moz-pagecontent, ::-moz-pagebreak.

> Should this test data.mHasAttributes instead?

I think so, yes.  I'll do that.

> Were there actually any cases in which we were running SelectorMatches on
> something that did not implement nsIStyledContent?

No.  Any time mContent was non-null it implemented nsIStyledContent.

I'll post an updated patch tomorrow.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 12

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

Comment on attachment 201198 [details] [diff] [review]
Updated to comments

r=dbaron on the nsCSSStyleSheet.cpp changes

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to tip

This is what I just checked in.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 15

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

(In reply to comment #9)
> >   if (result && (aSelector->mIDList || aSelector->mClassList)) {
> >     // test for ID & class match
> >     result = localFalse;
> >-    if (data.mStyledContent) {
> >+    if (data.mContent) {
> 
> Should this test data.mHasAttributes instead?  Seems more efficient.  (Were
> there actually any cases in which we were running SelectorMatches on something
> that did not implement nsIStyledContent?  I can't think of any off the top of
> my head.)

I just realized this change probably broke restyles triggered by removal of an id or class attribute when that was the only attribute on the element.

Comment 16

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

... I'll fix that on bug 315567.