990250 - Merge nsCSSStyleSheet and nsIStyleSheet

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

As of bug 882573, all nsIStyleSheet are nsCSSStyleSheet. We should merge them; dbaron suggested mozilla::CSSStyleSheet.

Comment 1

[Cameron McCormack (:heycam)]

Stealing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=139c587977e7

Comment 2

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 3

[Cameron McCormack (:heycam)]

Attachment: patch v2

A couple of tweaks in response to poiru pointing out to me that we do indeed have versions of InsertElementAt that don't take a fallible_t argument (they're promoted from protected to public with a using statement, which is why I overlooked it).

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thank you!

Comment 5

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

Comment on attachment 8686291 [details] [diff] [review]
patch v2

This would have been better done as 2 separate patches; one to convert
callers to using CSSStyleSheet*, and the other to do the actual folding.


nsDocument.cpp:

>+  for (uint32_t indx = mChildren.ChildCount(); indx-- > 0; ) {

I somewhat prefer != to >, though it doesn't really matter.



I guess I probably don't want to know why some PresShell methods
take nsISupports* parameters for sheets...


In nsStyleSheetService, in both LoadAndRegisterSheet and UnregisterSheet:

>-      const nsCOMArray<nsIStyleSheet> & sheets = mSheets[aSheetType];
>-      serv->NotifyObservers(sheets[sheets.Count() - 1], message, nullptr);
>+      CSSStyleSheet* sheet = mSheets[aSheetType].LastElement();
>+      serv->NotifyObservers(NS_ISUPPORTS_CAST(nsIDOMCSSStyleSheet*, sheet),
>+                            message, nullptr);

I'm a little worried about this passing a different nsISupports pointer,
but I guess it's probably ok.

(Also, do you really need the ".get()" in UnregisterSheet?)


nsStyleSet.cpp:

>-      default:
>+      default: {
>         // levels containing non-CSS stylesheets
>-        NS_ASSERTION(mSheets[aType].Count() == 1, "only one sheet per level");
>-        mRuleProcessors[aType] = do_QueryInterface(mSheets[aType][0]);
>+        NS_ASSERTION(mSheets[aType].Length() == 1, "only one sheet per level");
>+        nsISupports* sheet =
>+          NS_ISUPPORTS_CAST(nsIDOMCSSStyleSheet*, mSheets[aType][0]);
>+        mRuleProcessors[aType] = do_QueryInterface(sheet);
>         break;
>+      }

This code no longer makes sense.  It should have gone away in bug 882573
(although I didn't think then that I was removing the last objects that
implemented both nsIStyleSheet and nsIStyleRuleProcessor -- however,
this patch proves that that's the case).  But this patch turns it into
nonsense, so now it should go.

Please make the default case just be a MOZ_ASSERT(false, "not reached").

And also please remove the comment higher up that says:
>    // handle the types for which have a rule processor that does not
>    // implement the style sheet interface.
and replace it with something based on:
>        // levels containing non-CSS stylesheets
from this code.

r=dbaron with that

Comment 6

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #5)
> In nsStyleSheetService, in both LoadAndRegisterSheet and UnregisterSheet:
> 
> >-      const nsCOMArray<nsIStyleSheet> & sheets = mSheets[aSheetType];
> >-      serv->NotifyObservers(sheets[sheets.Count() - 1], message, nullptr);
> >+      CSSStyleSheet* sheet = mSheets[aSheetType].LastElement();
> >+      serv->NotifyObservers(NS_ISUPPORTS_CAST(nsIDOMCSSStyleSheet*, sheet),
> >+                            message, nullptr);
> 
> I'm a little worried about this passing a different nsISupports pointer,
> but I guess it's probably ok.

The change from nsIStyleSheet to nsIDOMCSSStyleSheet means we keep using the object's first nsISupports pointer.  That's the right thing to do, yes?

I was wondering whether there was anything we could do to prevent the use of an object's non-canonical nsISupports pointers/objects.  I guess not?

Comment 7

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=913560078e15

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5160a4dfba49

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5160a4dfba49