Closed Bug 1253007 Opened 6 years ago Closed 6 years ago

use UniquePtr for RangePaintInfo in nsPresShell.cpp


(Core :: Layout, defect)

Not set



Tracking Status
firefox47 --- fixed


(Reporter: froydnj, Assigned: froydnj)



(2 files, 1 obsolete file)

No description provided.
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...

 - 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+
Addressed review comments.
Attachment #8725880 - Flags: review+
Attachment #8725853 - Attachment is obsolete: true
Follow-up to use const&.  Carrying over rs+.
Attachment #8725881 - Flags: review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.