Closed
Bug 1253007
Opened 6 years ago
Closed 6 years ago
use UniquePtr for RangePaintInfo in nsPresShell.cpp
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
Details
Attachments
(2 files, 1 obsolete file)
8.31 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
4.54 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
No description provided.
![]() |
Assignee | |
Comment 1•6 years ago
|
||
UniquePtr > nsAutoPtr, and having the ownership information in function signatures is nice too.
Attachment #8725853 -
Flags: review?(dholbert)
Comment 2•6 years ago
|
||
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 | |
Updated•6 years ago
|
Attachment #8725853 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 4•6 years ago
|
||
Follow-up to use const&. Carrying over rs+.
Attachment #8725881 -
Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f8180923bc2 https://hg.mozilla.org/integration/mozilla-inbound/rev/a85779947e3d
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1f8180923bc2 https://hg.mozilla.org/mozilla-central/rev/a85779947e3d
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•