Closed Bug 1220190 Opened 4 years ago Closed 4 years ago

use UniquePtr<T[]> instead of delete[] calls in layout/xul/

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(1 file)

No description provided.
Some things in nsGrid (BuildRows and BuildCellMap) were not especially
straightforward replacements; ideally they will be reasonably easy to review.
Attachment #8681353 - Flags: review?(dholbert)
Comment on attachment 8681353 [details] [diff] [review]
use UniquePtr<T[]> instead of delete[] calls in layout/xul/

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

r=me with the following (though it might be worth doing another round of review; feel free to re-request review if you think so)

::: layout/xul/grid/nsGrid.cpp
@@ +304,5 @@
>   * Given the number of rows create nsGridRow objects for them and full them out.
>   */
>  void
> +nsGrid::BuildRows(nsIFrame* aBox, int32_t aRowCount,
> +                  UniquePtr<nsGridRow[]>* aRows, bool aIsHorizontal)

So here, we're ending up with a *pointer to* a UniquePtr, as an outparam. That's kind of awkward, and it seems like a pattern that's worth avoiding, at least when it's easy to avoid. (and I think it's easy to avoid here?)

Could you refactor this method to just *directly* return a mozilla::UniquePtr<nsGridRow[]>, and have the caller call it like so:
  mRows = BuildRows(...);
  mColumns = BuildRows(...);

@@ +312,2 @@
>      *aRows = nullptr;
>      return;

(So in the refactor suggested above, this case (inside of a "aRowCount == 0" check) would just become "return nullptr" or something like that.)

::: layout/xul/nsSplitterFrame.cpp
@@ +724,5 @@
>      // Now swap the two arrays.
>      nscoord newAfterCount = mChildInfosBeforeCount;
>      mChildInfosBeforeCount = mChildInfosAfterCount;
>      mChildInfosAfterCount = newAfterCount;
> +    mChildInfosBefore.swap(mChildInfosAfter);

While you're shortening a 3-line "swap" operation here, could you also shorten the 3-line "count" swapping right before it, to this one-liner:
  Swap(mChildInfosBeforeCount, mChildInfosAfterCount);

(This is using mozilla::Swap, defined in mfbt/Move.h, which is included in UniquePtr.h so you should have it avaialble here.)
Attachment #8681353 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #2)
>> ::: layout/xul/grid/nsGrid.cpp
> @@ +304,5 @@
> >   * Given the number of rows create nsGridRow objects for them and full them out.
> >   */
> >  void
> > +nsGrid::BuildRows(nsIFrame* aBox, int32_t aRowCount,
> > +                  UniquePtr<nsGridRow[]>* aRows, bool aIsHorizontal)
> 
> So here, we're ending up with a *pointer to* a UniquePtr, as an outparam.
> That's kind of awkward, and it seems like a pattern that's worth avoiding,
> at least when it's easy to avoid. (and I think it's easy to avoid here?)
> 
> Could you refactor this method to just *directly* return a
> mozilla::UniquePtr<nsGridRow[]>, and have the caller call it like so:
>   mRows = BuildRows(...);
>   mColumns = BuildRows(...);

Yeah, that's much better.  I started doing that, but somehow got hung up on the method reusing the storage for mRows and mColumns if possible.  I'll make that change.

> ::: layout/xul/nsSplitterFrame.cpp
> @@ +724,5 @@
> >      // Now swap the two arrays.
> >      nscoord newAfterCount = mChildInfosBeforeCount;
> >      mChildInfosBeforeCount = mChildInfosAfterCount;
> >      mChildInfosAfterCount = newAfterCount;
> > +    mChildInfosBefore.swap(mChildInfosAfter);
> 
> While you're shortening a 3-line "swap" operation here, could you also
> shorten the 3-line "count" swapping right before it, to this one-liner:
>   Swap(mChildInfosBeforeCount, mChildInfosAfterCount);
> 
> (This is using mozilla::Swap, defined in mfbt/Move.h, which is included in
> UniquePtr.h so you should have it avaialble here.)

Good point.  I'll make both swaps use mozilla::Swap for consistency.
Thanks! Yeah, even in that storage-reuse case, it seems like everything should work out fine. (which it sounds like you realized)
https://hg.mozilla.org/mozilla-central/rev/ebe8555c2c31
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1229985
You need to log in before you can comment on or make changes to this bug.