Change nsAutoPtr to UniquePtr in EventTree and Accessible classes

RESOLVED FIXED in Firefox 50

Status

()

defect
P4
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: michael.li11702, Assigned: michael.li11702)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(4 attachments)

No description provided.
Assignee: nobody → nfroyd
Assignee: nfroyd → nobody
Attachment #8766510 - Flags: review?(nfroyd)
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.
Attachment #8766510 - Flags: review?(nfroyd) → review+
In general bugs should be assigned to the patch author. Thanks for the patch Michael!
Assignee: nobody → mili
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.
Attachment #8766812 - Flags: review?(nfroyd) → review+
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.
Attachment #8766818 - Flags: review?(nfroyd) → review+
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.
Priority: -- → P4
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.
Keywords: checkin-needed
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
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da30eecce638
Change nsAutoPtr to UniquePtr in EventTree and Accessible classes. r=nfroyd
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/da30eecce638
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.