1253007 - use UniquePtr for RangePaintInfo in nsPresShell.cpp

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: use UniquePtr for RangePaintInfo in nsPresShell.cpp

UniquePtr > nsAutoPtr, and having the ownership information in function
signatures is nice too.

Comment 2

[Daniel Holbert [: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.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 1 - use UniquePtr for RangePaintInfo in nsPresShell.cpp

Addressed review comments.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 2 - pass a const& to PaintRangePaintInfo; rs=dholbert

Follow-up to use const&.  Carrying over rs+.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f8180923bc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/a85779947e3d

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1f8180923bc2
https://hg.mozilla.org/mozilla-central/rev/a85779947e3d