286518 - improve nsIStyleSheetService

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[(not currently active) Ted Mielczarek]

The nsIStyleSheetService from bug 179006 could use a few extra methods to make
it more useful to extensions.

First, a method to determine whether a URL has already been registered with the
service, something like bool sheetRegistered(in nsIURI sheetURI, in unsigned
long type)

Second, a method to unregister an already registered stylesheet.  The stylesheet
would not be applied to any documents loaded after this method is called, but
existing documents would still have the style applied (until reload).  Something
like void unregisterSheet(in nsIURI sheetURI, in unsigned long type);

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: Proposed Interface Changes

Proposed changes to the interface

Comment 2

[(not currently active) Ted Mielczarek]

Attachment: Implements proposed methods

I don't really like all the repeated code, but I don't know of a better way to
write this currently.  I'd appreciate some feedback on the patch.

The code worked fine in my tests from javascript.

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: Same as above, but fixed the off-by-one array error

Forgot to rediff after I fixed that.  Damn faulty docs!

Comment 4

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

A way to avoid the repetition is to replace mAgentSheets and mUserSheets with
nsCOMArray<nsIStyleSheet> mSheets[2];

Then put a runtime assertion that the two constants are 0 and 1 and a runtime
assertion that the input is one of the constants and then just use the sheet
type as the array index.

Comment 5

[(not currently active) Ted Mielczarek]

Attachment: Refactored ala dbaron

That looks a lot nicer.  Not sure if that's the right place for the
NS_ASSERTION though.

Comment 6

[Nickolay_Ponomarev]

Is there any reason you don't include a method to enumerate registered
stylesheets? The interface looks incomplete without it.

Comment 7

[(not currently active) Ted Mielczarek]

(In reply to comment #6)
> Is there any reason you don't include a method to enumerate registered
> stylesheets? The interface looks incomplete without it.

nsIStyleSheet isn't scriptable, so we'd have to implement a new enumerator or do
some wacky conversion to get an enumerator that returns something sensible for
script.  Also, I don't see a persuasive use case for it.  I'd expect most code
would want to register its own set of sheets, and manage them itself.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177828 [details] [diff] [review]
Refactored ala dbaron

>Index: nsStyleSheetService.cpp
> nsStyleSheetService::nsStyleSheetService()
> {
>+  NS_ASSERTION(0 == AGENT_SHEET && 1 == USER_SHEET, "Invalid values for USER_SHEET or AGENT_SHEET");

s/values/value/

>+  PRBool bEqual;
>+  for (PRInt32 i = sheets.Count() - 1; i >= 0; i-- ) {

Move that PRBool into the for loop, please?

>+    nsCOMPtr<nsIURI> uri;
>+    if (NS_SUCCEEDED(sheets[i]->GetSheetURI(getter_AddRefs(uri)))
>+        && NS_SUCCEEDED(uri->Equals(sheetURI, &bEqual))

Probably want to add

   && uri

in here...  just in case.

> nsStyleSheetService::LoadAndRegisterSheet(nsIURI *aSheetURI,
>                                           PRUint32 aSheetType)
> {
>+  NS_ENSURE_TRUE(aSheetType == AGENT_SHEET || aSheetType == USER_SHEET,
>+                 NS_ERROR_INVALID_ARG);

  NS_ENSURE_ARG(aSheetType == AGENT_SHEET || aSheetType == USER_SHEET);

Expands to the same code, and is easier to read...

>+nsStyleSheetService::SheetRegistered(nsIURI *sheetURI,
>+                                     PRUint32 aSheetType, PRBool *_retval)
>+{
>+  NS_ENSURE_TRUE(aSheetType == AGENT_SHEET || aSheetType == USER_SHEET,
>+                 NS_ERROR_INVALID_ARG);

Same here.

>+  NS_ENSURE_TRUE(sheetURI && _retval, NS_ERROR_NULL_POINTER);

NS_ENSURE_ARG_POINTER(sheetURI);

If you want, add NS_PRECONDITION(_retval, "Null out param") or something, but
don't null-check that, since there is no reasonable way to pass in null there
without blatantly violating the XPCOM contract.

>+nsStyleSheetService::UnregisterSheet(nsIURI *sheetURI, PRUint32 aSheetType)
>+{
>+  NS_ENSURE_TRUE(aSheetType == AGENT_SHEET || aSheetType == USER_SHEET,
>+                 NS_ERROR_INVALID_ARG);

NS_ENSURE_ARG(aSheetType == AGENT_SHEET || aSheetType == USER_SHEET);

>+  NS_ENSURE_TRUE(sheetURI, NS_ERROR_NULL_POINTER);

NS_ENSURE_ARG_POINTER(sheetURI);

>+  PRInt32 foundIndex = -1;
>+
>+  foundIndex = FindSheetByURI(mSheets[aSheetType], sheetURI);

Why bother initing to -1?  Just init to the return value of FindSheetByURI.

The rest looks great!

Comment 9

[(not currently active) Ted Mielczarek]

Attachment: Updated per bz's comments

With requested changes.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 178313 [details] [diff] [review]
Updated per bz's comments

r+sr=bzbarsky.	Should I go ahead and check this in for you?

Comment 11

[(not currently active) Ted Mielczarek]

Yes.  Thanks!

Comment 12

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 13

[(not currently active) Ted Mielczarek]

Tested and working fine.