1255069 - use UniquePtr for storage in nsTreeContentView

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: use UniquePtr for storage in nsTreeContentView

This is a sort-of-straightforward search-and-replace.  The only wrinkle is that
where the previous code did:

  Row* row = mRows[...];

or similar, relying on the implicit conversion to Row*, we sometimes do:

  UniquePtr<Row>& row = mRows[...];

and sometimes we do:

  Row* row = mRows[...].get();

The latter is necessary because subsequent code in those functions can modify
|mRows| and thus invalidate our UniquePtr&.  I can convert everything to use
the latter form, if you like.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Nathan Froyd [:froydnj] from comment #1)
> The latter is necessary because subsequent code in those functions can modify
> |mRows| and thus invalidate our UniquePtr&.

Ooh, tricky.

(Looks like the latter is sometimes necessary for a simpler reason -- just because we reassign |row|, e.g. in HasNextSibling and GetLevel.)

> I can convert everything to use the latter form, if you like.

I think I would prefer that, yeah.  That seems like a safer conversion, in terms of likelihood-of-introducing-new-bugs.  And it reduces the amount of auditing that I'd feel that I need to do here (and that anyone would need to know to do, in the future, whenever changing this code), to be sure that absolutely nothing modifies the array (including in helper functions) after we've declared a local UniquePtr<Row>& variable.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8728498 [details] [diff] [review]
use UniquePtr for storage in nsTreeContentView

Canceling review for now -- assuming you're OK with the update per end of comment 1, I'll just review the updated patch.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: use UniquePtr for storage in nsTreeContentView

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8728572 [details] [diff] [review]
use UniquePtr for storage in nsTreeContentView

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

r=me, nits below:

::: layout/xul/tree/nsTreeContentView.cpp
@@ +1061,5 @@
>                              nsGkAtoms::_true, eCaseMatters))
>      return;
>  
> +  UniquePtr<Row>& row =
> +    *aRows.AppendElement(MakeUnique<Row>(aContent, aParentIndex));

I'd prefer:
  aRows.AppendElement(MakeUnique<Row>(aContent, aParentIndex));
  Row* row = aRows.LastElement().get();
...since I'm uncomfortable dealing with a reference to an object that lives in an array that could be reallocated depending on what we do with it.

(Also, the fact that we're using UniquePtr<>& as our local-variable type *suggests* that it's giving us some guarantee that it'll keep the thing alive, but it doesn't actually give us any extra guarantees over what we'd get from a raw pointer. If we happened to clear the array, for example, then touching |row| would be unsafe from that point on, regardless of its type.)

@@ +1143,5 @@
>  int32_t
>  nsTreeContentView::EnsureSubtree(int32_t aIndex)
>  {
> +  
> +  Row* row = mRows[aIndex].get();

Drop the first blank-line-with-whitespace here. (introduced in this patch)

@@ +1156,4 @@
>    int32_t index = 0;
>    Serialize(child, aIndex, &index, rows);
> +  // We can't use InsertElementsAt with an array argument since the
> +  // destination can't steal ownership from its const source argument.

While you're modifying this comment, please also clarify it a bit, to highlight *what we're doing* instead of what we're *not* doing.

E.g.:
  // Insert |rows| into |mRows| at position |aIndex|, by first creating empty
  // UniquePtr entries and then Move'ing |rows|'s entries into them. (Note
  // that we can't simply use InsertElementsAt with an array argument, since
  // [etc]

@@ +1238,5 @@
>    // We can't use InsertElementsAt since the destination can't steal
>    // ownership from its const source argument.
>    for (nsTArray<Row>::index_type i = 0; i < rows.Length(); i++) {
> +    UniquePtr<Row>* newRow = mRows.InsertElementAt(aParentIndex + aIndex + i + 1);
> +    *newRow = Move(rows[i]);

It'd be nice to make this into a safer one-liner while you're here. Can't we just do:
  mRows.InsertElementAt(aParentIndex + aIndex + i + 1,
                        Move(rows[i]);
?

That compiles for me locally, at least.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/203f3bb029cf

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/203f3bb029cf