Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect")

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: neerja)

Tracking

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

3 years ago
We're slowly migrating from using nsAutoPtr to UniquePtr as our RAII strongly-owning type.

Filing this bug on fixing one particular instance, in nsStyleStruct.h:
> struct nsStyleImage
> {
> [...]
>   nsAutoPtr<nsStyleSides> mCropRect;

https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.h#376

In particular, we should:
 - Replace the nsAutoPtr.h include with "mozilla/UniquePtr.h", at the top of nsStyleStruct.h

 - Change the member-variable declaration (quoted above) to mozilla::UniquePtr<nsStyleSides> instead of nsAutoPtr<nsStyleSides>.

 - Change this variable's getter to return a "const UniquePtr<nsStyleSides>&" instead of a raw pointer (per the recommendation in the UniquePtr documentation, "If you want to expose the related resource [...] it's better to expose a |const UniquePtr&|" https://dxr.mozilla.org/mozilla-central/source/mfbt/UniquePtr.h#170 )

 - Change the callers to accommodate this. I think there are only two callers:
 https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsStyleImage%3A%3AGetCropRect%28%29+const%22  And probably only the second one needs to change -- to use "const UniquePtr<nsStyleSides>&" as its local-variable-decl type.

 - Change this variable's setter ("SetCropRect") to simply take a UniquePtr instead of a raw pointer (per UniquePtr documentation, "To unconditionally transfer ownership of a UniquePtr"), and perform the assignment using Move(), and add Move() around the setter's argument at the callsites.

 - Probably one or two other things that need to change, too, but hopefully the above list is mostly exhaustive.

(I'm planning on assigning this bug to a new engineer starting next week, so please nobody take it. :))
Reporter

Updated

3 years ago
Flags: needinfo?(dholbert)
Reporter

Updated

3 years ago
Assignee: nobody → npancholi
Status: NEW → ASSIGNED
Flags: needinfo?(dholbert)
Reporter

Comment 2

3 years ago
Comment on attachment 8775705 [details]
Bug 1288797 - Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect")

https://reviewboard.mozilla.org/r/67846/#review64868

This looks pretty good! Marking it r- for now, but I think it'll be r+ with the following addressed:

::: layout/style/nsComputedDOMStyle.cpp:2091
(Diff revision 1)
>      {
>        imgIRequest *req = aStyleImage.GetImageData();
>        nsCOMPtr<nsIURI> uri;
>        req->GetURI(getter_AddRefs(uri));
>  
> -      const nsStyleSides* cropRect = aStyleImage.GetCropRect();
> +      const mozilla::UniquePtr<nsStyleSides>& cropRect = aStyleImage.GetCropRect();

Please shorten this to just "UniquePtr" here. (Not "mozilla::UniquePtr")

(For brevity, in .cpp files, we usually don't bother with the "mozilla::" prefix on mozilla-namespaced stuff -- we add "using" declarations at the top of the file ("using namespace mozilla;" in this case) which tell the compiler about the namespace and let it figure out that UniquePtr really means mozilla::UniquePtr.)

(We only drop the namespace like this for .cpp files.  In .h files, on the other hand, we *do* use the "mozilla::" prefix, because we don't want to put "using" declarations in header files generally [because that'd arbitrarily mess with behavior in .cpp files that include that header file].)

::: layout/style/nsStyleStruct.cpp:1971
(Diff revision 1)
>    } else if (aOther.mType == eStyleImageType_Element) {
>      SetElementId(aOther.mElementId);
>    }
>  
> -  SetCropRect(aOther.mCropRect);
> +  nsStyleSides* copyOfCropRect = new nsStyleSides(*(aOther.mCropRect.get()));
> +  SetCropRect(MakeUnique<nsStyleSides>(*copyOfCropRect));

It looks "copyOfCropRect" is an extra intermediate copy that we're making here (and then leaking it, I think), because MakeUnique allocates its own copy (separate from the one allocated via |new| here).

We should skip the |new| and just do something like:
`SetCropRect(MakeUnique<nsStyleSides>(*aOther.mCropRect.get())`

I think that should work -- it should invoke the nsStyleSides copy-constructor under the hood, I expect...

::: layout/style/nsStyleStruct.cpp:2082
(Diff revision 1)
>      mType = eStyleImageType_Element;
>    }
>  }
>  
>  void
> -nsStyleImage::SetCropRect(nsStyleSides* aCropRect)
> +nsStyleImage::SetCropRect(mozilla::UniquePtr<nsStyleSides> aCropRect)

As noted above, since this is in a .cpp file, you shouldn't use the "mozilla::" prefix here. Just UniquePtr.

::: layout/style/nsStyleStruct.cpp:2085
(Diff revision 1)
>  
>  void
> -nsStyleImage::SetCropRect(nsStyleSides* aCropRect)
> +nsStyleImage::SetCropRect(mozilla::UniquePtr<nsStyleSides> aCropRect)
>  {
>    if (aCropRect) {
> -    mCropRect = new nsStyleSides(*aCropRect);
> +    mCropRect = Move(aCropRect);

You can just collapse this function to:
 mCropRect = Move(aCropRect);
 
No need for the null-check anymore.  (It was needed back when we did the allocation [or not] in this function, but now the caller is doing the allocation for us, it looks like.  So, we can just accept whatever the caller passes in, whether it's null or not.)

::: layout/style/nsStyleStruct.cpp:2253
(Diff revision 1)
>  {
>    if (mType != aOther.mType) {
>      return false;
>    }
>  
> -  if (!EqualRects(mCropRect, aOther.mCropRect)) {
> +  if (!EqualRects(mCropRect.get(), aOther.mCropRect.get())) {

Let's change EqualRects to take a `const UniquePtr<nsStyleSides>&` instead of a raw pointer, so that we can leave this line un-changed.

(Part of the idea with UniquePtr is to avoid dealing with raw pointers as much as possible, so that ownership and lifetime is made explicit.)
Attachment #8775705 - Flags: review?(dholbert) → review-
Assignee

Comment 3

3 years ago
Okay, I will make these changes and re-send you the review request. Thanks!
Comment hidden (obsolete)
Reporter

Comment 6

3 years ago
(Oh, I see -- it looks like the second commit here is just the new changes; disregard comment 5, I mistakenly thought it was an updated version of the original commit.)
Reporter

Comment 7

3 years ago
Comment on attachment 8775779 [details]
Bug 1288797 - Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect"). Changes after review.

https://reviewboard.mozilla.org/r/67868/#review64888

Looks like this addressed all my comments - thanks! Just one nit, below.

Also: we need to get this combined into a single commit (rather than using separate patches/commits dedicated to fixing review comments).  So, we need to figure out how to squash your two commits into one (with the following nit addressed as well).  Once you've done that (and gotten that updated commit onto this bug), I'll mark it r+ and we can test & land it.

::: layout/style/nsStyleStruct.cpp:2248
(Diff revision 1)
>  {
>    if (mType != aOther.mType) {
>      return false;
>    }
>  
> -  if (!EqualRects(mCropRect.get(), aOther.mCropRect.get())) {
> +  if (!EqualRects(mCropRect, aOther.mCropRect) ) {

Looks like this is introducing an extra space between the close-parenthesis here -- drop that extra space, please.

(Ultimately, this line shouldn't be modified at all by patches on this bug -- it should remain as it was before; and it didn't include a space before.)
Attachment #8775779 - Flags: review?(dholbert) → review-
Assignee

Comment 8

3 years ago
Comment on attachment 8775705 [details]
Bug 1288797 - Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect")

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67846/diff/1-2/
Attachment #8775705 - Flags: review- → review?(dholbert)
Assignee

Updated

3 years ago
Attachment #8775779 - Attachment is obsolete: true
Reporter

Comment 9

3 years ago
Comment on attachment 8775705 [details]
Bug 1288797 - Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect")

https://reviewboard.mozilla.org/r/67846/#review64890

Looks great! r=me
Attachment #8775705 - Flags: review?(dholbert) → review+
Assignee

Comment 10

3 years ago
Comment on attachment 8775705 [details]
Bug 1288797 - Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect")

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67846/diff/2-3/
Reporter

Comment 11

3 years ago
https://reviewboard.mozilla.org/r/67846/#review64958

::: layout/style/nsStyleStruct.cpp:1971
(Diff revisions 2 - 3)
>    } else if (aOther.mType == eStyleImageType_Element) {
>      SetElementId(aOther.mElementId);
>    }
>  
> -  SetCropRect(MakeUnique<nsStyleSides>(*aOther.mCropRect.get()));
> +  UniquePtr<nsStyleSides> cropRectCopy;
> +  if(aOther.mCropRect){

Nit: please add spaces between "if" and the open-paren, and between the close-paren and the curly-brace.

Should be:
  if (aOther.mCropRect) {

Otherwise, this updated version looks good!
Assignee

Comment 12

3 years ago
Comment on attachment 8775705 [details]
Bug 1288797 - Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect")

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67846/diff/3-4/
Reporter

Comment 13

3 years ago
Great, thanks for fixing that - with a successful Try run (kick that off whenever it's convenient), I think this will be ready to check in!
Reporter

Comment 14

3 years ago
The latest try run[1] is a bit colorful, but I'm pretty sure all the failures are sporadic / unrelated. So, I'll go ahead and autoland this. Congrats on getting your first patch into the tree!

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a081611b832

Comment 15

3 years ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4dc4d45c2115
Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect") r=dholbert

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4dc4d45c2115
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee

Comment 17

3 years ago
(In reply to Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) from comment #14)
> The latest try run[1] is a bit colorful, but I'm pretty sure all the
> failures are sporadic / unrelated. So, I'll go ahead and autoland this.
> Congrats on getting your first patch into the tree!
> 
> [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a081611b832

Thanks Daniel! :)
You need to log in before you can comment on or make changes to this bug.