Closed Bug 990250 Opened 6 years ago Closed 4 years ago

Merge nsCSSStyleSheet and nsIStyleSheet

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: Ms2ger, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

As of bug 882573, all nsIStyleSheet are nsCSSStyleSheet. We should merge them; dbaron suggested mozilla::CSSStyleSheet.
Severity: normal → enhancement
Assignee: nobody → Ms2ger
Depends on: 1022855
Stealing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=139c587977e7
Assignee: Ms2ger → cam
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
Attachment #8685977 - Flags: review?(dbaron)
Attached patch patch v2Splinter Review
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).
Attachment #8685977 - Attachment is obsolete: true
Attachment #8685977 - Flags: review?(dbaron)
Attachment #8686291 - Flags: review?(dbaron)
Thank you!
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
Attachment #8686291 - Flags: review?(dbaron) → review+
(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?
Blocks: 1225345
https://hg.mozilla.org/mozilla-central/rev/5160a4dfba49
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.