Add API to register style sheets but without loading them

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

({dev-doc-needed})

Trunk
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Posted patch load-sheet (obsolete) — Splinter Review
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 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.
Attachment #8439465 - Flags: feedback?(bzbarsky)
Assignee: nobody → evilpies
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.
Blocks: abp
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
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 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?
Attachment #8439465 - Flags: feedback?(bzbarsky) → feedback-
> 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?
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.
Posted patch v1 (obsolete) — Splinter Review
Attachment #8439465 - Attachment is obsolete: true
Attachment #8440278 - Flags: review?(bzbarsky)
Attachment #8440278 - Flags: review?(bzbarsky)
Posted patch v1 (obsolete) — Splinter Review
Attachment #8440278 - Attachment is obsolete: true
Attachment #8440279 - Flags: review?(bzbarsky)
Posted patch testSplinter Review
I wasn't really sure which test suite to use, but I figured test_additional_sheets.html would be a good example.
Attachment #8440296 - Flags: review?(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).
Attachment #8440279 - Flags: review?(bzbarsky) → review+
Comment on attachment 8440296 [details] [diff] [review]
test

r=me if you add the missing newline at EOF.
Attachment #8440296 - Flags: review?(bzbarsky) → review+
(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).
> 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.
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!
Posted patch v2Splinter Review
Attachment #8440279 - Attachment is obsolete: true
Attachment #8447215 - Flags: review?(bzbarsky)
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 on attachment 8447215 [details] [diff] [review]
v2

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

r=me with that.
Attachment #8447215 - Flags: review?(bzbarsky) → review+
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
I am not sure what is going on here. Maybe this just needs a clobber? Or some UUID change was wrong?
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.
https://hg.mozilla.org/mozilla-central/rev/2a5d0b307055
https://hg.mozilla.org/mozilla-central/rev/c0076344ffbb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
It seems that nsIStyleSheetService.preloadSheet() and nsIDOMWindowUtils.addSheet() didn't get documented.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.