refactor some of nsXBLPrototypeResources to make it easier to deal with for style sheet changes

RESOLVED FIXED in Firefox 32

Status

()

Core
XBL
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

Trunk
mozilla33
Points:
---

Firefox Tracking Flags

(firefox31 wontfix, firefox32 fixed, firefox33 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
Instead of allowing random objects to poke into public member variables on nsXBLPrototypeResources, let's encapsulate them to make code that manipulates them more sane.
(Assignee)

Comment 1

4 years ago
Created attachment 8440479 [details] [diff] [review]
Part 1: Give nsXBLPrototypeResources::mRuleProcessor a more concrete type.

We never create any other kind of nsIStyleRuleProcessor than a nsCSSRuleProcessor.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8440479 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

4 years ago
Created attachment 8440480 [details] [diff] [review]
Part 2: Encapsulate nsXBLPrototypeResources::mRuleProcessor.
Attachment #8440480 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

4 years ago
Created attachment 8440481 [details] [diff] [review]
Part 3: Encapsulate nsXBLPrototypeResources::mStyleSheetList.

I'm giving this an nsStyleSet-like API, for familiarity.
Attachment #8440481 - Flags: review?(bzbarsky)
Comment on attachment 8440479 [details] [diff] [review]
Part 1: Give nsXBLPrototypeResources::mRuleProcessor a more concrete type.

r=me
Attachment #8440479 - Flags: review?(bzbarsky) → review+
Comment on attachment 8440480 [details] [diff] [review]
Part 2: Encapsulate nsXBLPrototypeResources::mRuleProcessor.

r=me
Attachment #8440480 - Flags: review?(bzbarsky) → review+
Comment on attachment 8440481 [details] [diff] [review]
Part 3: Encapsulate nsXBLPrototypeResources::mStyleSheetList.

>+    binding->PrototypeBinding()->AppendStyleSheetsTo(*array);

Why AppendStyleSheetsTo, not just AppendStyleSheets?

>+nsXBLPrototypeBinding::RequireResources()

Normally we'd call this EnsureResources.

r=me with at least the EnsureResources change made.
Attachment #8440481 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 7

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #6)
> Comment on attachment 8440481 [details] [diff] [review]
> Part 3: Encapsulate nsXBLPrototypeResources::mStyleSheetList.
> 
> >+    binding->PrototypeBinding()->AppendStyleSheetsTo(*array);
> 
> Why AppendStyleSheetsTo, not just AppendStyleSheets?

I thought AppendStyleSheets would sound like you were passing in the array of sheets that you wanted to append to nsXBLPrototypeResources::mStyleSheetList.
Oh, I see.  I totally got what the function did backwards somehow.
Hey Cameron, this work blocks bug 1025738 which we'd like to backport to Aurora. Is this bug reasonably safe to land there as well? If so, can you please nominate it for approval? Thanks!
(Assignee)

Comment 14

4 years ago
Comment on attachment 8440479 [details] [diff] [review]
Part 1: Give nsXBLPrototypeResources::mRuleProcessor a more concrete type.

Approval Request Comment
[Feature/regressing bug #]: -
[User impact if declined]: Would prevent bug 1025738 from landing, which solves some memory leak problems.
[Describe test coverage new/current, TBPL]: Been on m-c for a couple of weeks.
[Risks and why]: Low, this is a small, no-behaviour-change refactoring.
[String/UUID change made/needed]: -

(request applies to all 3 parts in this bug)
Attachment #8440479 - Flags: approval-mozilla-aurora?
status-firefox32: --- → affected
status-firefox33: --- → fixed
Attachment #8440479 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.