Closed Bug 905441 Opened 11 years ago Closed 11 years ago

Load style sheets from the parent into the child

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: billm, Assigned: evilpie)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch style-sheets (obsolete) (deleted) — Splinter Review
This is needed for adblock, which uses a big style sheet to blacklist certain kinds of images and elements.
Attachment #790517 - Flags: review?(felipc)
Seems like it would be better for this code to be entirely in the back-end (nsStyleSheetService) if possible.
Component: General → Layout
Product: Firefox → Core
Comment on attachment 790517 [details] [diff] [review] style-sheets I talked to Gavin and he's convinced me that we should do this somewhere in the style sheet service code instead of in JS. I'm thinking we would forward new style sheets to the child using a new message in the PContent protocol. Tom, is there any chance you could take a look at this? If not, I'll try to get to it on Monday.
Attachment #790517 - Attachment is obsolete: true
Attachment #790517 - Flags: review?(felipc)
Flags: needinfo?(evilpies)
I can do this. I already have the forwarding for LoadAndRegisterSheet and UnregisterSheet done. The next thing is requesting all the stylesheets from the parent, when we nsIStyleSheetService instance in the child.
Assignee: wmccloskey → evilpies
Flags: needinfo?(evilpies)
Attached patch v1 (obsolete) (deleted) — Splinter Review
So this implements everything in C++. When somebody calls loadAndRegisterSheet or unregisterSheet in the parent we asynchronously send that to all children as well. The other case is when a new child is created it asks for all style-sheets in the parent, this has a small flaw, because we also send those in the category manager. But actually checking if something is already in that manager is a bit more work. We could potentially just never use the category manager in content.
Comment on attachment 791477 [details] [diff] [review] v1 Roc for the layout bits and smaug for content? I can't really tell who is a good reviewer here.
Attachment #791477 - Flags: review?(roc)
Attachment #791477 - Flags: review?(bugs)
Comment on attachment 791477 [details] [diff] [review] v1 dbaron should definitely review this.
Attachment #791477 - Flags: review?(bugs) → review?(dbaron)
So I'm not crazy about the "doing both" aspect of comment 4 -- i.e., having the child both get the sheets from the parent or from the category manger. I'm inclined to think we ought to be doing one or the other, though I'm also not all that familiar with the architecture here (i.e., how parents and children ought to interact in general, how setup/teardown works). Having the GetStyleSheets be sync also seems a little disturbing. (Sync certainly feels bad to me, though I'm not sure how worried I should actually be.) Could the parent instead send the sheets async to the child during the process of setting up the child? I'm curious what others think fits better with the model of how we set up child processes. needinfo? to jlebar for that since he seems like somebody who might know that, but feel free to pass along to somebody else if more appropriate.
Flags: needinfo?(justin.lebar+bug)
> Could the parent instead send the sheets async to the child during the process of setting up the > child? That seems pretty sane. Then we'd only need one mechanism for getting stylesheets from the parent to the child, and we'd avoid a sync call.
Flags: needinfo?(justin.lebar+bug)
Comment on attachment 791477 [details] [diff] [review] v1 Review of attachment 791477 [details] [diff] [review]: ----------------------------------------------------------------- This is all dbaron :-)
Attachment #791477 - Flags: review?(roc)
Okay I will look at where is a good point to send the stylesheets to the child.
Attached patch v2 (deleted) — Splinter Review
In this new patch we send the load during the ContentChild creation.
Attachment #791477 - Attachment is obsolete: true
Attachment #791477 - Flags: review?(dbaron)
Attachment #792889 - Flags: review?(dbaron)
I was just discussing this issue with Fabrice; two other issues we discussed was: (a) whether userContent.css is currently broken in child processes in B2G (since child processes can't access the profile), and (b) that B2G probably has a theme-ing use case for loading style sheets via the style sheet service (and app: URLs) that would fail security checks in other child processes. That perhaps suggests we'd eventually want this to be sending the style sheet rather than its URL. That said, that issue probably ought to go in a different bug; I was thinking it was closely related to this one, but it should probably be distinct.
Component: Layout → CSS Parsing and Computation
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 792889 [details] [diff] [review] v2 And I'm going to transfer this review request to bz; he knows this code better. (Yes, both of our review queues are overloaded, I think, but...)
Attachment #792889 - Flags: review?(dbaron) → review?(bzbarsky)
Comment on attachment 792889 [details] [diff] [review] v2 >+ SerializeURI(aSheetURI, uri); >+ >+ nsTArray<dom::ContentParent*> children; >+ dom::ContentParent::GetAll(children); Is the idea that children will end up empty in child processes? But why pay the SerializeURI cost in that case? Can we condition this on the process type? r=me modulo that
Attachment #792889 - Flags: review?(bzbarsky) → review+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: