1024707 - Add API to register style sheets but without loading them

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Tom S]

Attachment: load-sheet

For Adblock we want to have an API that allows us to share nsIStylesSheets between multiple window, but allows us to choose when to load/inject them.

Comment 1

[Tom S]

Comment on attachment 8439465 [details] [diff] [review]
load-sheet

If this approach is fine, I will also add some documentation. I think we probably also need an API for unloading from the nsStyleSheet service, but I am not sure yet.

Comment 2

[Wladimir Palant]

Comment on attachment 8439465 [details] [diff] [review]
load-sheet

(In reply to Tom Schuster [:evilpie] from comment #1)
> I think we
> probably also need an API for unloading from the nsStyleSheet service, but I
> am not sure yet.

With the current patch - yes, because the registered stylesheet will be added to mSheets. However, IMHO that actually shouldn't be done because that stylesheet will be applied to all new documents then. My understanding is that the reference returned by nsIStyleSheet.registerSheet() will be the only reference to that stylesheet then - so simply dropping the reference will be sufficient to have it garbage collected.


>+  nsIStyleSheet registerSheet(in nsIURI sheetURI, in unsigned long type);

This method is misnamed, its functionality would be better described as preloadSheet(). Also, the type parameter here is pointless, it is already being specified when the stylesheet is actually applied.

>   // Allow UA sheets, but not user sheets, to use unsafe rules
>   nsresult rv = loader->LoadSheetSync(aSheetURI, aSheetType == AGENT_SHEET,
>-                                      true, getter_AddRefs(sheet));
>+                                      true, aSheet);

It might be better to use loader->LoadSheet() for preloaded stylesheets to avoid blocking the main thread. However, I don't know whether applying a user stylesheet which hasn't been completely loaded will work correctly.

>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  if (!mSheets[aSheetType].AppendObject(sheet)) {
>+  if (!mSheets[aSheetType].AppendObject(*aSheet)) {

Appending the preloaded stylesheet to mSheets will make it apply to every new document automatically - which is exactly what we are trying to avoid here. Also, if we don't append it then the only reference to the stylesheet will be the one we return. So an explicit "unregister" API should be unnecessary, just dropping the reference to the stylesheet should be sufficient for the garbage collector to do its job.

Side-note: With the amount of branching required, there seems to be little benefit in sharing LoadAndRegisterSheetInternal() with preloaded stylesheets.

Comment 3

[Tom S]

Oh good hint about now adding them to the sheets list, and managing the lifetime based on the JS-object, that indeed seems like the cleaner approach.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8439465 [details] [diff] [review]
load-sheet

I think calling this nsIDocument API LoadStyleSheet is wrong.  The sheet is already loaded.  It just needs to be applied.

Maybe AddAdditionalStyleSheet?

Similar for the windowutils API.

You can't use nsIStyleSheet in IDL.  Use nsIDOMStyleSheet instead.

The stylesheet service bit seems wrong to me.  Once you call this API, it'll apply the sheet to all documents after that, no?

Comment 5

[Tom S]

> The stylesheet service bit seems wrong to me.  Once you call this API, it'll
> apply the sheet to all documents after that, no?

You are right, Wladimir pointed that out already. Is there a better place to put this function now that it only loads the stylesheet and returns nsIDOMStyleSheet?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Putting it on the stylesheet service is fine.  We just shouldn't add the sheet to its internal lists.

If we wanted to, we could still cache it in the service, just in some new member.

Comment 7

[Tom S]

Attachment: v1

Comment 8

[Tom S]

Attachment: v1

Comment 9

[Tom S]

Attachment: test

I wasn't really sure which test suite to use, but I figured test_additional_sheets.html would be a good example.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8440279 [details] [diff] [review]
v1

So what happens if I call AddAdditionalStyleSheet on a document and then recreate its presshell?  The new style set doesn't know about this sheet, right?

>+++ b/dom/interfaces/base/nsIDOMWindowUtils.idl

Does removeSheet work for sheets that were added via addSheet?

Apart from those this looks fine, but we probably need to fix the "recreate the presshell" case (by storing the sheet somewhere on the document).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8440296 [details] [diff] [review]
test

r=me if you add the missing newline at EOF.

Comment 12

[Tom S]

(In reply to Boris Zbarsky [:bz] from comment #10)
> Comment on attachment 8440279 [details] [diff] [review]
> v1
> 
> So what happens if I call AddAdditionalStyleSheet on a document and then
> recreate its presshell?  The new style set doesn't know about this sheet,
> right?
> 
We would just have to do mAdditionalSheets[aType].AppendObject(sheet); right?

Btw, what is this for? Intutively I would say we don't want to change the owner.
  sheet->SetOwningDocument(this);
  MOZ_ASSERT(sheet->IsApplicable());

> >+++ b/dom/interfaces/base/nsIDOMWindowUtils.idl
> 
> Does removeSheet work for sheets that were added via addSheet?
> 
Looking at the code this should work after we added it to mAdditionalSheets. I could also add an overload that takes nsIStyleSheet.
> Apart from those this looks fine, but we probably need to fix the "recreate
> the presshell" case (by storing the sheet somewhere on the document).

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Btw, what is this for?

It's for telling the sheet it's associated with that document (e.g. so that changes to the sheet update the right document).

We can either not care about changes to the sheet, and add some new setup where the document is not set as the owning document for the sheet, or we need to enforce that only unowned sheets are passed into this API and require cloning on the part of API consumers.  And expose the cloning stuff to them, of course...

Maybe the simplest thing is to make the stylesheet service API return a clone of the sheet if it's been loaded before, and have the document API throw if the sheet is already owned by a document, else take ownership of it.

Comment 14

[Tom S]

At least for Adblock most of this sounds too complicated.
First we should not take ownership (Thus we don't care about changes to the sheet) and second we should not accept a sheet with an owner.

>Maybe the simplest thing is to make the stylesheet service API return a clone of the sheet if it's been >loaded before
At least preload doesn't maintain a list of loaded sheets, so I don't think I want to do this.

Thanks bz!

Comment 15

[Tom S]

Attachment: v2

Comment 16

[Tom S]

Oh I just realized something. With this patch we over report memory used by style-sheets, because the memory reporting code doesn't know that the sheet is shared. But that also means we probably need some place where we report the memory used by it at least once.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8447215 [details] [diff] [review]
v2

We should probably throw from the new API if not IsApplicable().

r=me with that.

Comment 18

[Tom S]

I changed the code so that we don't measure the memory of style-sheets that are shared. I also refreshed the uids.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a56c14d382f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f213b3135312

Comment 19

[Carsten Book [:Tomcat]]

sorry had to backout this changes for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=43422289&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=43422568&tree=Mozilla-Inbound

Comment 20

[Tom S]

I am not sure what is going on here. Maybe this just needs a clobber? Or some UUID change was wrong?

Comment 21

[Tom S]

Here is a try push: https://tbpl.mozilla.org/?tree=Try&rev=dfca92915ee7 that already looks pretty good. Maybe we just need to clobber on inbound.

Comment 22

[Tom S]

Now with clobber.
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2a5d0b307055
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c0076344ffbb

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a5d0b307055
https://hg.mozilla.org/mozilla-central/rev/c0076344ffbb

Comment 24

[Wladimir Palant]

It seems that nsIStyleSheetService.preloadSheet() and nsIDOMWindowUtils.addSheet() didn't get documented.