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

(Blocks 1 bug, {dev-doc-needed})

Trunk
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

5 years ago
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.
Assignee

Comment 1

5 years ago
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

Updated

5 years ago
Assignee: nobody → evilpies

Comment 2

5 years ago
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.

Updated

5 years ago
Blocks: abp
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Assignee

Comment 3

5 years ago
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-
Assignee

Comment 5

5 years ago
> 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.
Assignee

Comment 7

5 years ago
Posted patch v1 (obsolete) — Splinter Review
Attachment #8439465 - Attachment is obsolete: true
Attachment #8440278 - Flags: review?(bzbarsky)
Assignee

Updated

5 years ago
Attachment #8440278 - Flags: review?(bzbarsky)
Assignee

Comment 8

5 years ago
Posted patch v1 (obsolete) — Splinter Review
Attachment #8440278 - Attachment is obsolete: true
Attachment #8440279 - Flags: review?(bzbarsky)
Assignee

Comment 9

5 years ago
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+
Assignee

Comment 12

5 years ago
(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.
Assignee

Comment 14

5 years ago
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!
Assignee

Comment 15

5 years ago
Posted patch v2Splinter Review
Attachment #8440279 - Attachment is obsolete: true
Attachment #8447215 - Flags: review?(bzbarsky)
Assignee

Comment 16

5 years ago
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+
Assignee

Comment 18

5 years ago
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
Assignee

Comment 20

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

Comment 21

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33

Comment 24

3 years ago
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.