Closed Bug 1253007 Opened 4 years ago Closed 4 years ago
Ptr for Range Paint Info in ns Pres Shell .cpp
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... 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+
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+
You need to log in before you can comment on or make changes to this bug.