Views a11y: Convert View's NativeViewAccessibility to a unique_ptr.

Views a11y: Convert View's NativeViewAccessibility to a unique_ptr.

Currently, instances of NativeViewAccessibility depend on
NativeViewAccessibility::Destroy() being called in order to be released. By
converting the Views NativeViewAccessibility member to a unique_ptr, it will be
automatically destroyed with its corresponding View.

BUG=610589
Review-Url: https://codereview.chromium.org/2718073003
Cr-Commit-Position: refs/heads/master@{#454808}
Committed: https://chromium.googlesource.com/chromium/src/+/28468e05efc7fde6e9201a29a25b2f8904b5b5e4

[Patti Lor, 2017-02-28 02:29:02]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 02:29:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 02:45:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 02:46:00]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Patti Lor, 2017-02-28 03:50:57]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 03:51:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 05:15:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 05:15:48]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-02-28 05:24:46]

patricialor@chromium.org changed reviewers:
+ tapted@chromium.org

[Patti Lor, 2017-02-28 05:24:47]

Ignored elements CL part 1 :)

[dmazzoni, 2017-02-28 05:41:20]

dmazzoni@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2017-02-28 05:41:20]

lgtm

[tapted, 2017-02-28 06:29:12]

lgtm % nits

(sky should take a look at the views::View changes though) Thanks!

https://codereview.chromium.org/2718073003/diff/20001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility.cc (right):

https://codereview.chromium.org/2718073003/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:21: return
base::WrapUnique<NativeViewAccessibility>(
nit: Comment here like

// Use WrapUnique rather than MakeUnique to invoke the protected constructor.

https://codereview.chromium.org/2718073003/diff/20001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility.h (right):

https://codereview.chromium.org/2718073003/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.h:76:
NativeViewAccessibility(View* view);
nit: this should be declared explicit..

https://codereview.chromium.org/2718073003/diff/20001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility_auralinux.cc (right):

https://codereview.chromium.org/2718073003/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility_auralinux.cc:172: return
base::MakeUnique<NativeViewAccessibilityAuraLinux>(view);
huhhh .. I guess we can't declare NativeViewAccessibilityAuraLinux's constructor
private because this method is actually on NativeViewAccessibility, so it would
need to be a friend.

(note we probably don't need to reorder the methods in the .cc since they're
different classes - either is fine)

https://codereview.chromium.org/2718073003/diff/20001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility_win.cc (right):

https://codereview.chromium.org/2718073003/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility_win.cc:55:
NativeViewAccessibilityWin::NativeViewAccessibilityWin(View* view)
The two NativeViewAccessibilityWin methods should be put back where they were to
match the .h. 

 - Create() can stay where it was too (in which case update AuraLinux), or
 - Create() can stay down here (in which case update Mac) :)

[Patti Lor, 2017-02-28 07:36:19]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 07:36:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 08:29:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 08:29:29]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-02-28 22:24:24]

patricialor@chromium.org changed reviewers:
+ sky@chromium.org

[Patti Lor, 2017-02-28 22:24:25]

Hi sky@, PTAL at changes in ui/views/view.*.

Thanks!

https://codereview.chromium.org/2718073003/diff/20001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility.cc (right):

https://codereview.chromium.org/2718073003/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:21: return
base::WrapUnique<NativeViewAccessibility>(
On 2017/02/28 06:29:12, tapted wrote:
> nit: Comment here like
> 
> // Use WrapUnique rather than MakeUnique to invoke the protected constructor.

Done - sorry, I forgot :(

https://codereview.chromium.org/2718073003/diff/20001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility.h (right):

https://codereview.chromium.org/2718073003/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.h:76:
NativeViewAccessibility(View* view);
On 2017/02/28 06:29:12, tapted wrote:
> nit: this should be declared explicit..

Thanks for always picking these up, I always miss it :(

https://codereview.chromium.org/2718073003/diff/20001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility_auralinux.cc (right):

https://codereview.chromium.org/2718073003/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility_auralinux.cc:172: return
base::MakeUnique<NativeViewAccessibilityAuraLinux>(view);
On 2017/02/28 06:29:12, tapted wrote:
> huhhh .. I guess we can't declare NativeViewAccessibilityAuraLinux's
constructor
> private because this method is actually on NativeViewAccessibility, so it
would
> need to be a friend.
> 
> (note we probably don't need to reorder the methods in the .cc since they're
> different classes - either is fine)

Acknowledged.

https://codereview.chromium.org/2718073003/diff/20001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility_win.cc (right):

https://codereview.chromium.org/2718073003/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility_win.cc:55:
NativeViewAccessibilityWin::NativeViewAccessibilityWin(View* view)
On 2017/02/28 06:29:12, tapted wrote:
> The two NativeViewAccessibilityWin methods should be put back where they were
to
> match the .h. 
> 
>  - Create() can stay where it was too (in which case update AuraLinux), or
>  - Create() can stay down here (in which case update Mac) :)

Took the first option to match what they were originally like. Sorry, I checked
them out from my old branch before edits and didn't think about whether the
reorders still made sense, but it should be good now.

[sky, 2017-03-01 00:27:08]

Are there any dependencies in NativeViewAccessibility back on the View that
could be problematic now that NVA is destroyed later on? Before this change
NVA::Destroy() could callback to the View, now it can't.

https://codereview.chromium.org/2718073003/diff/40001/ui/views/controls/tabbe...
File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right):

https://codereview.chromium.org/2718073003/diff/40001/ui/views/controls/tabbe...
ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:187: widget->CloseNow();
nva outlives widget (and thus the tab). Is that ok?

[Patti Lor, 2017-03-01 06:45:37]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 06:45:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patti Lor, 2017-03-01 07:13:14]

No, I don't think so - nothing in the destructor seems to need |view_| and it
will get removed from |parent_widget_| as an observer whether it or the widget
gets destroyed first. I did fix the tab test though :)

https://codereview.chromium.org/2718073003/diff/40001/ui/views/controls/tabbe...
File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right):

https://codereview.chromium.org/2718073003/diff/40001/ui/views/controls/tabbe...
ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:187: widget->CloseNow();
On 2017/03/01 00:27:07, sky wrote:
> nva outlives widget (and thus the tab). Is that ok?

Ah, yeah I think it would be bad if someone tried to use |nva| after this line.
Fixed now, thanks!

[commit-bot: I haz the power, 2017-03-01 07:42:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 07:42:11]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-01 17:09:45]

LGTM - but I would like Domninic to give a final approval on the lifetime issue
I mentioned.

[Patti Lor, 2017-03-03 02:49:33]

Ping for dmazzoni@ - PTAL at sky's comments, thank you!

[dmazzoni, 2017-03-03 08:48:34]

Still lgtm

Good question, but this should be totally fine. It shouldn't
have ever called back into View, I believe, but the reason
we had Destroy() was because originally it was a
reference-counted COM object.

Now that AXPlatformNodeWin is the COM object, it should
be safe to just simply delete NativeViewAccessibility.

[Patti Lor, 2017-03-06 00:46:12]

Awesome, thanks all.

[Patti Lor, 2017-03-06 00:46:19]

The CQ bit was checked by patricialor@chromium.org

[Patti Lor, 2017-03-06 00:46:20]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/2718073003/#ps60001
(title: "Fix long lived NVA in test.")

[commit-bot: I haz the power, 2017-03-06 00:46:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 01:47:26]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488761179669400,
"parent_rev": "d5f275985e8e58c1ef94444d28b7f9cbc66982dc", "commit_rev":
"28468e05efc7fde6e9201a29a25b2f8904b5b5e4"}

[commit-bot: I haz the power, 2017-03-06 01:48:48]

Description was changed from

==========
Views a11y: Convert View's NativeViewAccessibility to a unique_ptr.

Currently, instances of NativeViewAccessibility depend on
NativeViewAccessibility::Destroy() being called in order to be released. By
converting the Views NativeViewAccessibility member to a unique_ptr, it will be
automatically destroyed with its corresponding View.

BUG=610589
==========

to

==========
Views a11y: Convert View's NativeViewAccessibility to a unique_ptr.

Currently, instances of NativeViewAccessibility depend on
NativeViewAccessibility::Destroy() being called in order to be released. By
converting the Views NativeViewAccessibility member to a unique_ptr, it will be
automatically destroyed with its corresponding View.

BUG=610589

Review-Url: https://codereview.chromium.org/2718073003
Cr-Commit-Position: refs/heads/master@{#454808}
Committed:
https://chromium.googlesource.com/chromium/src/+/28468e05efc7fde6e9201a29a25b...
==========

[commit-bot: I haz the power, 2017-03-06 01:48:49]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/28468e05efc7fde6e9201a29a25b...