Closed Bug 1247182 Opened 8 years ago Closed 8 years ago

add nsCSSRuleProcessor constructor that takes a sheet array rvalue reference

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(1 file)

There are a few call sites where we really need to pass ownership of the sheet array to the newly created nsCSSRuleProcessor, so we should have a constructor that takes an nsTArray<RefPtr<CSSStyleSheet>>&&.
Attached patch patchSplinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a84a1faed3ef
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8740898 - Flags: review?(bbirtles)
Comment on attachment 8740898 [details] [diff] [review]
patch

Review of attachment 8740898 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSRuleProcessor.cpp
@@ +983,5 @@
>                                         Element* aScopeElement,
>                                         nsCSSRuleProcessor*
>                                           aPreviousCSSRuleProcessor,
>                                         bool aIsShared)
> +  : nsCSSRuleProcessor(Move(sheet_array_type(aSheets)), aSheetType,

I'm probably overlooking something but I don't understand why this is ok.

As I understand it, Move() basically does a static cast to sheet_array_type&&. The call site of this constructor, however, is passing in a const-ref and might be assuming that aSheets will be untouched as a result of calling this constructor. However, if we pass aSheets to the nsCSSRuleProcessor overload that takes an rvalue reference, then initialize mSheets with the move constructor for nsTArray, that will call SwapElements and leave aSheets empty, right? So if the call site assumes it can keep using aSheets it will get a surprise?

If that's right, maybe we need to make one constructor do copying and one do moving, either by duplicating the logic (i.e. not using delegated constructors) or by factoring out a third base constructor that doesn't touch mSheets? Alternatively we could have a templated constructor that takes advantages of universal references to provide the different behaviors.

Alternatively, if we don't need the copy behavior at all we should just make all call sites explicitly pass an rvalue reference so they're declaring they're not going to use aSheets anymore.

Is that right?
The sheet_array_type() in there should be making a copy of aSheets, which is what gets moved into mSheets.  Certainly if aSheets were modified, then all kinds of things would break (since this constructor gets called with some of nsStyleSet's sheet arrays).
(In reply to Cameron McCormack (:heycam) from comment #3)
> The sheet_array_type() in there should be making a copy of aSheets, which is
> what gets moved into mSheets.  Certainly if aSheets were modified, then all
> kinds of things would break (since this constructor gets called with some of
> nsStyleSet's sheet arrays).

Ah, thanks yes! I didn't recognize that as a constructor! In that case, we shouldn't need Move(), right? It should already be an rvalue?
(In reply to Brian Birtles (:birtles) from comment #4)
> Ah, thanks yes! I didn't recognize that as a constructor! In that case, we
> shouldn't need Move(), right? It should already be an rvalue?

Ah yes, I'll try that.
Comment on attachment 8740898 [details] [diff] [review]
patch

Review of attachment 8740898 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSRuleProcessor.cpp
@@ +983,5 @@
>                                         Element* aScopeElement,
>                                         nsCSSRuleProcessor*
>                                           aPreviousCSSRuleProcessor,
>                                         bool aIsShared)
> +  : nsCSSRuleProcessor(Move(sheet_array_type(aSheets)), aSheetType,

As discussed, we should drop the Move() if that works.
Attachment #8740898 - Flags: review?(bbirtles) → review+
https://hg.mozilla.org/mozilla-central/rev/cbb08180a3ec
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: