1014891 - Kill off nsIDocument's "CatalogStyleSheet" methods

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jonathan Watt [:jwatt]]

It is unclear from nsIDocument.h what nsIDocument's "CatalogStyleSheet" methods are for. We have nsIDocument::LoadAdditionalStyleSheet(eAgentSheet, ...) which seems to make nsIDocument::AddCatalogStyleSheet redundant.

Digging back a bit it seems that this all comes from the intention to support:

https://www.oasis-open.org/committees/entity/spec.html

That is, to fix bug 98413.

The AddCatalogStyleSheet caller in nsXMLContentSink::HandleDoctypeDecl makes sense in that context. That said, it is actually dead code. Nothing sets mCatalogData->mAgentSheet, which means HandleDoctypeDecl() is always passed nullptr as its aCatalogData argument, which means nsXMLContentSink::HandleDoctypeDecl never actually calls AddCatalogStyleSheet.

The other callers of EnsureCatalogStyleSheet seem to be abusing EnsureCatalogStyleSheet when they should really be using something else.

So it seems like the "catalog style sheet" stuff in nsIDocument should be removed, and the callers of EnsureCatalogStyleSheet should use something else; preferably something with a more meaningful name and useful documentation.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=63b61ba5a2e4

Comment 2

[Jonathan Watt [:jwatt]]

This does make svg.css and mathml.css come first in the user-agent sheets instead of last (lowers their priority), but I think that's okay since no other sheets seem to mess with elements in those namespaces. (Well, except for the PresShell function that populate mPrefStyleSheet, but I don't think any of that depends on ordering either.)

Comment 3

[Jonathan Watt [:jwatt]]

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

(In reply to Jonathan Watt [:jwatt] from comment #0)
> That said, it is actually dead code. Nothing sets
> mCatalogData->mAgentSheet

Somehow it is being set in some cases, so my fatal assertion is triggered on Try.

Also it seems that devtools can no longer see the svg.css rules if they're not in the catalog sheets list, so toolkit/devtools/server/tests/mochitest/test_styles-svg.html fails.

*sigh*

Comment 4

[Jonathan Watt [:jwatt]]

Okay, so the mAgentSheet is set because kCatalogTable is an array of nsCatalogData objects with initializers:

https://mxr.mozilla.org/mozilla-central/source/parser/htmlparser/src/nsExpatDriver.cpp#261

We could just null out the mathml.css entries in that table though and rely on nsMathMLElement::BindToTree to load the sheet.

bz: thoughts?

Also I seem to remember you talking to someone else on IRC about which sheets are visible to devtools inspector, so maybe you have some thoughts on that also?

Comment 5

[Boris Zbarsky [:bzbarsky]]

mPrefStyleSheet is a user sheet, no a UA sheet, so none of this interacts with it.

> We could just null out the mathml.css entries in that table though and rely on
> nsMathMLElement::BindToTree to load the sheet.

That seems pretty reasonable to me; presumably that works already because we support mathml-in-HTML, right?

I just realized that nsIDocument::CreateStaticClone clones catalog sheets but not additional sheets...  Not sure whether that matters in practice.

I have no idea what's going on with devtools; I'd expect it to work for additional sheets exactly the same way it does for catalog sheets...

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> I have no idea what's going on with devtools; I'd expect it to work for
> additional sheets exactly the same way it does for catalog sheets...

I swear I saw you talking to someone on IRC about how the way devtools inspector gets the UA rules is a hack and horrible, but I can't find that conversation in my logs now. Could you recount the details here? Maybe that will help me debug. I'll also ping the devtools guys.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I don't recall that conversation....

The way inspector gets UA rules, I would assume, is by walking the rules list from the style context via inIDOMUtils.getCSSStyleRules or some such.  But that works directly on the style contexts and ruletree, not on sheet storage.

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> > We could just null out the mathml.css entries in that table though and rely on
> > nsMathMLElement::BindToTree to load the sheet.
> 
> That seems pretty reasonable to me; presumably that works already because we
> support mathml-in-HTML, right?

Right. I'll null out those entries.

> I have no idea what's going on with devtools; I'd expect it to work for
> additional sheets exactly the same way it does for catalog sheets...

This is just because catalog style sheets are user sheets so devtools would see them, whereas this patch makes them user-agent sheets (as they should be) which hides them from devtools. I'll adjust the devtools test so that it doesn't expect to see the svg.css rule.

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 10

[Jonathan Watt [:jwatt]]

Note that bug 1015147 goes on to speed up the EnsureBuiltInUserAgentStyleSheet function by eliminating the string comparison.

Comment 11

[Jonathan Watt [:jwatt]]

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

Still getting failures on this.

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: patch

This fixes things so that we remember to load the sheets if the EnsureOnDemandBuiltInUASheet() call is made before we have a pres shell. It also leaves the code much more like it is now.

Seems to now pass the two types of Try failures that were remaining.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

>+  virtual void EnsureOnDemandBuiltInUASheet(const char *aStyleSheetURI) = 0;

So... I know this is how it used to work, but using a string to identify these is silly.  Can we just do an enum, with a matching static array in enum order of the URIs and a per-document array of bools that we can index into with the enum to check quickly to see whether the sheet is already loaded?

I guess we'd need to clear out this 

>+  virtual int32_t GetNumberOfOnDemandBuiltInUASheets() const = 0;

Why do we need this as public API at all?  The only consumer is nsIDocument::CreateStaticClone.  Make this protected, at least, if we're not just willing to static_cast to nsDocument* in there (which I am, by the way).

>+  virtual nsIStyleSheet* GetOnDemandBuiltInUASheetAt(int32_t aIndex) const = 0;

Likewise.

>+  virtual void AddOnDemandBuiltInUASheet(nsCSSStyleSheet* aSheet) = 0;

Definitely should be protected.

>+++ b/content/base/src/nsDocument.cpp
> nsDocument::ResetStylesheetsToURI(nsIURI* aURI)

So is it me, or does this leave semi-bogus stylesheets in mOnDemandBuiltInUASheets (and used to in mCatalogSheets)?  Should this code clear mOnDemandBuiltInUASheets and reset the relevant boolean members if we add them?

For the code where you basically just renamed the function, could you avoid moving it to avoid losing blame, please.  That would also make it clearer what actually got changed....

Why do no no longer SetOwningDocument in AddOnDemandBuiltInUASheet?  Seems to me like you should.

r=me with those fixed

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Seems to me like you should.

Though given the patch in bug 1015147, I guess not.

Comment 15

[Jonathan Watt [:jwatt]]

(In reply to Boris Zbarsky [:bz] from comment #13)
> using a string to identify these is silly.  Can we just do an enum,
> with a matching static array in
> enum order of the URIs and a per-document array of bools that we can index
> into with the enum to check quickly to see whether the sheet is already
> loaded?

I'll take a look at that, although bug 1015147 makes this a pointer comparison and, as you point out there can be changed to mOnDemandBuiltInUASheets.Contains().

> I guess we'd need to clear out this 

Unfinished sentence?

> >+  virtual int32_t GetNumberOfOnDemandBuiltInUASheets() const = 0;
> 
> Why do we need this as public API at all?  The only consumer is
> nsIDocument::CreateStaticClone.  Make this protected, at least, if we're not
> just willing to static_cast to nsDocument* in there (which I am, by the way).

I'll do the static cast thing and make them non-virtual and protected.

> >+++ b/content/base/src/nsDocument.cpp
> > nsDocument::ResetStylesheetsToURI(nsIURI* aURI)
> 
> So is it me, or does this leave semi-bogus stylesheets in
> mOnDemandBuiltInUASheets (and used to in mCatalogSheets)?  Should this code
> clear mOnDemandBuiltInUASheets and reset the relevant boolean members if we
> add them?

Nice catch.

> For the code where you basically just renamed the function, could you avoid
> moving it to avoid losing blame, please.  That would also make it clearer
> what actually got changed....

Okay. (I was kinda thinking it would be nice to move it to a better place while doing this, and I'd be okay with taking blame and being bugged on how this worked - or this bug should pretty much explain it.)

> Why do no no longer SetOwningDocument in AddOnDemandBuiltInUASheet?  Seems
> to me like you should.

Because the sheets set by nsDocumentViewer::CreateStyleSet don't, and conceptually these are the same, just delay loaded. Also devtools for some reason treats svg.css as a user sheet if these are set, whereas it should treat it as an agent sheet like html.css.

Thanks, bz!

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Unfinished sentence?

Er, that was talking about the Reset case, but then I moved that bit later in my comment and forgot about this bit.  

> I'll do the static cast thing and make them non-virtual and protected.

I don't think you even need methods for the length/sheet-at bits then; just use the array member directly.

> I was kinda thinking it would be nice to move it to a better place while doing this

I guess it's ok...  For future, "moving code" and "substantive changes" should probably be two separate changesets.

Comment 17

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bca61e86b826

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bca61e86b826