Boris Zbarsky [:bzbarsky] Assignee
	Description • 19 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 19 years ago

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...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 19 years ago

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.

	Johnny Stenback (:jst)
	Comment 5 • 19 years ago

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

sr=jst

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 6 • 19 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 19 years ago

> 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");

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 8 • 19 years ago

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

absolutly

	David Baron :dbaron:
	Comment 9 • 19 years ago

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.)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 19 years ago

> 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.

	David Baron :dbaron:
	Comment 12 • 19 years ago

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

r=dbaron on the nsCSSStyleSheet.cpp changes

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 19 years ago

This is what I just checked in.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 19 years ago

Fixed on trunk.

	David Baron :dbaron:
	Comment 15 • 19 years ago

(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.

	David Baron :dbaron:
	Comment 16 • 19 years ago

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