1283268 - Change nsAutoPtr to UniquePtr in EventTree and Accessible classes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P4)

Comment 1

[Michael Li]

Attachment: Change nsAutoPtr to UniquePtr in EventTree and Accessible classes

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8766510 [details] [diff] [review]
Change nsAutoPtr to UniquePtr in EventTree and Accessible classes

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

Just a few comments, but looks good.  r=me with the below fixed, assuming that you can actually turn the |new| calls into |MakeUnique| calls.

::: accessible/base/EventTree.cpp
@@ +230,5 @@
>  EventTree*
>  EventTree::FindOrInsert(Accessible* aContainer)
>  {
>    if (!mFirst) {
> +    mFirst.reset(new EventTree(aContainer, true));

Instead of .reset(), we prefer:

mFirst = MakeUnique<EventTree>(aContainer, true);

Depending on the visibility of |EventTree|, though, this may not be possible.  (EventTree's constructor might be visible to the enclosing class, but not to the MakeUnique function.)  The compiler will be able to tell you if you get it wrong.

@@ +280,5 @@
>        // Insert the tail node into the hierarchy between the current node and
>        // its parent.
>        node->mFireReorder = false;
> +      UniquePtr<EventTree>& nodeOwnerRef = prevNode ? prevNode->mNext : mFirst;
> +      UniquePtr<EventTree> newNode(new EventTree(aContainer, mDependentEvents.IsEmpty()));

For initializing new variables, we prefer:

auto newNode = MakeUnique<EventTree>(...);

as the type argument to MakeUnique makes it obvious what type the variable has, and this variable declaration is shorter than the alternative without |auto|.

@@ +329,5 @@
>    //   do not emit a reorder event for the container
>    //   if a dependent show event target contains the given container then do not
>    //   emit show / hide events (see Process() method)
>  
> +  prevNode->mNext.reset(new EventTree(aContainer, mDependentEvents.IsEmpty()));

Same advice on .reset() applies here as well.

::: accessible/generic/Accessible.cpp
@@ +2210,5 @@
>  Accessible::EmbeddedChildCount()
>  {
>    if (mStateFlags & eHasTextKids) {
>      if (!mEmbeddedObjCollector)
> +      mEmbeddedObjCollector.reset(new EmbeddedObjCollector(this));

Instead of .reset(), we prefer:

mEmbeddedObjCollector = MakeUnique<EmbeddedObjCollector>(this);

Please also add braces for this if statement.

@@ +2222,5 @@
>  Accessible::GetEmbeddedChildAt(uint32_t aIndex)
>  {
>    if (mStateFlags & eHasTextKids) {
>      if (!mEmbeddedObjCollector)
> +      mEmbeddedObjCollector.reset(new EmbeddedObjCollector(this));

Here too.

@@ +2228,1 @@
>        mEmbeddedObjCollector->GetAccessibleAt(aIndex) : nullptr;

Presumably this conditional can be eliminated because mEmbeddedObjCollector is always non-null?  It would be better to fix that in a followup bug.

@@ +2235,5 @@
>  Accessible::GetIndexOfEmbeddedChild(Accessible* aChild)
>  {
>    if (mStateFlags & eHasTextKids) {
>      if (!mEmbeddedObjCollector)
> +      mEmbeddedObjCollector.reset(new EmbeddedObjCollector(this));

Same advice for .reset() and the if statment as before.

@@ +2241,1 @@
>        mEmbeddedObjCollector->GetIndexAt(aChild) : -1;

Same question for this conditional.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

In general bugs should be assigned to the patch author. Thanks for the patch Michael!

Comment 4

[Michael Li]

Attachment: Change UniquePtr's reset function to MakeUnique function

Comment 5

[Michael Li]

Attachment: Add braces for if block; can't change reset function to MakeUnique

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8766812 [details] [diff] [review]
Change UniquePtr's reset function to MakeUnique function

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

Thanks for making these changes.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8766818 [details] [diff] [review]
Add braces for if block; can't change reset function to MakeUnique

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

Thanks for making these changes.  Before you commit (or set the checkin-needed keyword on this bug so that someone else will check it in for you), please combine these patches into a single patch.

Comment 8

[Michael Li]

Thanks for the feedback!

I also wanted to ask, in EventTree.cpp at line 283:
>UniquePtr<EventTree>& nodeOwnerRef = prevNode ? prevNode->mNext : mFirst;
Switching to MakeUnique, I wasn't able to change the type declaration to auto and keep it as a reference; I'm not sure if it is possible with C++ semantics.
For example, I tried
>auto &nodeOwnerRef = MakeUnique<EventTree>(prevNode ? prevNode->mNext : mFirst);
But this wouldn't compile as the right side isn't const.

Comment 9

[Michael Li]

Attachment: Change nsAutoPtr to UniquePtr in EventTree and Accessible classes

Comment 10

[Michael Li]

As for Comment 8, I suppose using the auto keyword with a reference won't be as clear as writing out UniquePtr<EventTree>& so I'll stick to that.

Comment 11

[Michael Li]

The Try Server results for attachment 8768817 [details] [diff] [review] (which by the way is all my patches combined) are here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe25769faac1

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da30eecce638
Change nsAutoPtr to UniquePtr in EventTree and Accessible classes. r=nfroyd

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/da30eecce638