use UniquePtr for RangePaintInfo in nsPresShell.cpp

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

3 years ago
No description provided.
Assignee

Comment 1

3 years ago
UniquePtr > nsAutoPtr, and having the ownership information in function
signatures is nice too.
Attachment #8725853 - Flags: review?(dholbert)
Comment on attachment 8725853 [details] [diff] [review]
use UniquePtr for RangePaintInfo in nsPresShell.cpp

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

Thanks! r=me, one nit and one followup-patch-request below:

::: layout/base/nsPresShell.h
@@ +519,5 @@
>    // create a RangePaintInfo for the range aRange containing the
>    // display list needed to paint the range to a surface
> +  mozilla::UniquePtr<RangePaintInfo>
> +  CreateRangePaintInfo(nsIDOMRange* aRange, nsRect& aSurfaceRect,
> +                       bool aForPrimarySelection);

I find this easier to read if we put each parameter on its own line, as it was before.  The function right after this one uses the one-param-per-line style, too (though it could collapse its args to fewer lines).

So: could you wrap "nsRect& aSurfaceRect" to its own line here? (for readability & consistency w/ adjacent functions & avoiding unnecessary stylistic changes from original code)

@@ +534,5 @@
>     * aFlags - set RENDER_AUTO_SCALE to scale down large images, but it must not
>     *          be set if a custom image was specified
>     */
>    already_AddRefed<SourceSurface>
> +  PaintRangePaintInfo(nsTArray<mozilla::UniquePtr<RangePaintInfo>>* aItems,

I think aItems *really* wants to be have type:
 const nsTArray<mozilla::UniquePtr<RangePaintInfo>>&
...instead of...
 nsTArray<mozilla::UniquePtr<RangePaintInfo>>*

 - Should be "const" because this function doesn't seem to modify the array (adding/removing pieces).
 - Should use a reference because (a) that's now we normally pass nsTArray args, (b) nobody ever passes in null here [or anything other than the address of a local variable] to this function, and (c) this isn't an outparam so there's no semantic benefit to having "&" at the callsite)

Could you add a second patch to make this additional trivial change?

rs=me in advance for this second patch, assuming it compiles.
Attachment #8725853 - Flags: review?(dholbert) → review+
Assignee

Comment 3

3 years ago
Addressed review comments.
Attachment #8725880 - Flags: review+
Assignee

Updated

3 years ago
Attachment #8725853 - Attachment is obsolete: true
Assignee

Comment 4

3 years ago
Follow-up to use const&.  Carrying over rs+.
Attachment #8725881 - Flags: review+

Comment 6

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1f8180923bc2
https://hg.mozilla.org/mozilla-central/rev/a85779947e3d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.