1234676 - nsComputedDOMStyle accessors should return already_AddRefed objects, instead of raw pointers to 0-refcount objects

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

nsComputedDOMStyle has a bunch of per-property getters, whose return value is described by this comment:
>  // All of the property getters below return a pointer to a refcounted object
>  // that has just been created, but the refcount is still 0. Caller must take
>  // ownership.
(This comment dates back to bug 585867, mozilla-central changeset 39ec013827f5.)
 
This pattern (returning a refcounted object w/ refcount=0) is odd & awkward, and it forces us to use bad coding practices inside of these functions. (It forces us to store new refcounted things in raw pointers, instead of RefPtrs.)

We should change these functions to return already_AddRefed<CSSValue>, so that their implementations can use RefPtr<> internally and return foo.forget(); at the end.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: add & use a typedef for CSSValue in .h file

First part is to add & use a private CSSValue typedef in the .h file, so we don't end up with the super-long expression
  already_AddRefed<mozilla::dom::CSSValue>
as the return type of all of these methods.

This patch builds successfully on its own.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 2a: Change function signature of DoGetXYZ functions & helpers (but don't change the impl)

Part 2 (the main change here) will be split into two sub-parts.

This first part just changes the function signatures of these getters and some associated helper-functions.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 2b: change the function implementations

...and this part changes the implementations of the functions whose signatures we modified in part 2a.

(I'll fold this together with 2a before landing, since we need both in order to build successfully.)

Comment 4

[Daniel Holbert [:dholbert]]

Notes to assist in skimming / comfortability-with-skimming when reviewing here:
 - Part 1 is a typedef + search-and-replace (and doesn't change the code at all).
 - Part 2a was basically a search-and-replace of the return-value-type in function signatures.
 - Part 2b was several search-and-replaces, for local variables:
     s/nsROCSSPrimitiveValue */RefPtr<nsROCSSPrimitiveValue>/
     s/nsDOMCSSValueList *valueList/RefPtr<nsDOMCSSValueList>/
        (along with the other *-snuggling way of writing these pointers)
     s/return val;/return val.forget();/
     s/return value;/return value.forget();/
     s/return valueList;/return valueList.forget();/

I left just 12 "nsROCSSPrimitiveValue *" local-variables untouched -- those variables are passed to the nsDOMCSSRect constructor (happens twice) and the nsDOMCSSRGBColor constructor (happens once), and we'd need to fix those before we convert these local variables. That should be done separately, if it's worth doing.

Finally: I'll post one more part, "part 3", to cleanup XXX comments that I added in bug 1234707, which we can now address.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part 3: remove some now-unneeded RefPtr local variables (addressing XXXdholbert comments)

This final part removes the now-unneeded local variables which I introduced in bug 1234707 part 4 ("special handling") with XXXdholbert comments.  See bug 1234707 comment 10 & 12 for background on these (why these variables were introduced, & why they can go away now that we have already_AddRefed<> as the return value of the function that they're in and/or the function whose value they were capturing.)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: part 2 in its entirety (2a+2b), as it'll land -- in case it's useful for review

Comment 7

[Daniel Holbert [:dholbert]]

Sorry for the lack of patch-context in these -- I did that locally to reduce likelihood of (& amount of pain caused by) bitrot.

I'd suggest reviewing with a merge tool anyway -- if it's handy, these patches are layered on top of mozilla-central changeset 6c49bb716593 (currently only on m-i, but will be merged to m-c soon I expect).

Comment 8

[Daniel Holbert [:dholbert]]

(Happy to repost with more context if that's useful though.)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: part 4: remove now-obsolete comment about these functions returning object w/ refcount of 0

Realized I need one last change here -- I need to get rid of the now-obsolete comment that I quoted in comment 0 here.

(We don't really need a replacement; already_AddRefed<> is self-documenting about the refcount status.)

Comment 10

[Daniel Holbert [:dholbert]]

Try run (looks good modulo intermittents I think): https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0f9ae6013ec

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8701948 [details] [diff] [review]
part 1: add & use a typedef for CSSValue in .h file

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

::: layout/style/nsComputedDOMStyle.h
@@ +49,4 @@
>  {
>  public:
>    typedef nsCSSProps::KTableEntry KTableEntry;
> +  typedef mozilla::dom::CSSValue CSSValue;

In comment 1 you said this would be a private typedef; did you mean to add it down lower in the file?  (KTableEntry could well become private too.)

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8701951 [details] [diff] [review]
part 2a: Change function signature of DoGetXYZ functions & helpers (but don't change the impl)

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

::: layout/style/nsComputedDOMStyle.h
@@ +131,1 @@
>                                                 bool aAlignTrue,

Please fix the indenting of this and other multi-line declarations.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: part 1 v2: add & use a typedef for CSSValue in .h file [r=heycam]

(In reply to Cameron McCormack (:heycam) (away Jan 1-3) from comment #11)
> In comment 1 you said this would be a private typedef; did you mean to add
> it down lower in the file?  (KTableEntry could well become private too.)

Oh, right you are. Here's the patch with that changed. (I made sure it still compiles.)  Carrying forward r+.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) (away Jan 1-3) from comment #12)
> Please fix the indenting of this and other multi-line declarations.

Will do; I'll post that as a whitespace-only final patch here, with rs=you (rubberstamp)

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: part 5: fix indentation & rewrap too-long decls [rs=heycam]

(Here's the final patch to fix up indentation in decls & rewrap some too-long decls, per comment 12. I'm calling this rubberstamped, but comments are welcome if you feel like skimming it.)

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8702389 [details] [diff] [review]
part 5: fix indentation & rewrap too-long decls [rs=heycam]

Looks fine, thanks.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7504e1ab5c38
https://hg.mozilla.org/integration/mozilla-inbound/rev/3324a9ebbb19
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f6d2bde4a1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a1afdb2383c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1e3bb1b9072

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7504e1ab5c38
https://hg.mozilla.org/mozilla-central/rev/3324a9ebbb19
https://hg.mozilla.org/mozilla-central/rev/1f6d2bde4a1b
https://hg.mozilla.org/mozilla-central/rev/2a1afdb2383c
https://hg.mozilla.org/mozilla-central/rev/b1e3bb1b9072