Replace ui::AXViewState with AXNodeData and AXActionData

Replace ui::AXViewState with AXNodeData and AXActionData

This is a refactoring change.

Originally AXViewState was created when Views accessibility
didn't share any code with the rest of Chrome. Now there's
a lot more overlap, and every place where AXViewState is
consumed, it's just converted into AXNodeData. This change
eliminates AXViewState and uses AXNodeData in Views directly,
allowing us to delete some conversion code.

To replace AXViewState::set_value_callback, which was rather
awkward, this change adds a virtual function to View that
takes an AXActionData, a general-purpose struct that encodes an
accessible action to take on an object. That provides a
cleaner solution for set_value_callback and allows for
more streamlining of code later.

I suggest reviewing Patch Set 4 first, as that includes
only the non-mechanical changes.

BUG=662064
Committed: https://crrev.com/13e5258f7ad40c6dee7bd64e92d72c1a3f683451
Cr-Commit-Position: refs/heads/master@{#429823}

[dmazzoni, 2016-11-03 07:40:16]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 07:40:43]

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, 2016-11-03 07:45:22]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-11-03 07:45:23]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dmazzoni, 2016-11-03 16:06:16]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 16:06:56]

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, 2016-11-03 16:46:54]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-11-03 16:46:56]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dmazzoni, 2016-11-03 16:55:06]

Description was changed from

==========
Not for review yet. Replace ui::AXViewState with ui::AXNodeData

BUG=
==========

to

==========
Replace ui::AXViewState with AXNodeData and AXActionData

This is a refactoring change.

Originally AXViewState was created when Views accessibility
didn't share any code with the rest of Chrome. Now there's
a lot more overlap, and every place where AXViewState is
consumed, it's just converted into AXNodeData. This change
eliminates AXViewState and uses AXNodeData in Views directly,
allowing us to delete some conversion code.

To replace AXViewState::set_value_callback, which was rather
awkward, add a virtual function to View that takes an
AXActionData, a general-purpose struct that encodes an
accessible action to take on an object. That provides a
cleaner solution for set_value_callback and allows for
more streamlining of code later.

I suggest reviewing these files first:
chrome/browser/ui/views/omnibox/omnibox_view_views.cc
chrome/browser/ui/views/omnibox/omnibox_view_views.h
ui/accessibility/ax_enums.idl
ui/accessibility/ax_node_data.cc
ui/accessibility/ax_node_data.h
ui/accessibility/ax_view_state.cc
ui/accessibility/ax_view_state.h
ui/views/accessibility/native_view_accessibility.cc
ui/views/controls/textfield/textfield.cc
ui/views/controls/textfield/textfield.h
ui/views/view.h

I've conveniently uploaded these into a separate
patch set (that doesn't compile) for easier review,
then the full change as a follow-up.

The rest of the files contain only mechanical changes.

BUG=662064
==========

[dmazzoni, 2016-11-03 16:57:14]

Description was changed from

==========
Replace ui::AXViewState with AXNodeData and AXActionData

This is a refactoring change.

Originally AXViewState was created when Views accessibility
didn't share any code with the rest of Chrome. Now there's
a lot more overlap, and every place where AXViewState is
consumed, it's just converted into AXNodeData. This change
eliminates AXViewState and uses AXNodeData in Views directly,
allowing us to delete some conversion code.

To replace AXViewState::set_value_callback, which was rather
awkward, add a virtual function to View that takes an
AXActionData, a general-purpose struct that encodes an
accessible action to take on an object. That provides a
cleaner solution for set_value_callback and allows for
more streamlining of code later.

I suggest reviewing these files first:
chrome/browser/ui/views/omnibox/omnibox_view_views.cc
chrome/browser/ui/views/omnibox/omnibox_view_views.h
ui/accessibility/ax_enums.idl
ui/accessibility/ax_node_data.cc
ui/accessibility/ax_node_data.h
ui/accessibility/ax_view_state.cc
ui/accessibility/ax_view_state.h
ui/views/accessibility/native_view_accessibility.cc
ui/views/controls/textfield/textfield.cc
ui/views/controls/textfield/textfield.h
ui/views/view.h

I've conveniently uploaded these into a separate
patch set (that doesn't compile) for easier review,
then the full change as a follow-up.

The rest of the files contain only mechanical changes.

BUG=662064
==========

to

==========
Replace ui::AXViewState with AXNodeData and AXActionData

This is a refactoring change.

Originally AXViewState was created when Views accessibility
didn't share any code with the rest of Chrome. Now there's
a lot more overlap, and every place where AXViewState is
consumed, it's just converted into AXNodeData. This change
eliminates AXViewState and uses AXNodeData in Views directly,
allowing us to delete some conversion code.

To replace AXViewState::set_value_callback, which was rather
awkward, add a virtual function to View that takes an
AXActionData, a general-purpose struct that encodes an
accessible action to take on an object. That provides a
cleaner solution for set_value_callback and allows for
more streamlining of code later.

I suggest reviewing these files first:
chrome/browser/ui/views/omnibox/omnibox_view_views.cc
chrome/browser/ui/views/omnibox/omnibox_view_views.h
ui/accessibility/ax_enums.idl
ui/accessibility/ax_node_data.cc
ui/accessibility/ax_node_data.h
ui/accessibility/ax_view_state.cc
ui/accessibility/ax_view_state.h
ui/views/accessibility/ax_view_obj_wrapper.cc
ui/views/accessibility/native_view_accessibility.cc
ui/views/controls/textfield/textfield.cc
ui/views/controls/textfield/textfield.h
ui/views/view.h

I've conveniently uploaded these into a separate
patch set (that doesn't compile) for easier review,
then the full change as a follow-up.

The rest of the files contain only mechanical changes.

BUG=662064
==========

[dmazzoni, 2016-11-03 17:14:23]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 17:14:52]

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

[dmazzoni, 2016-11-03 18:01:20]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 18:02:20]

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

[dmazzoni, 2016-11-03 18:03:12]

Description was changed from

==========
Replace ui::AXViewState with AXNodeData and AXActionData

This is a refactoring change.

Originally AXViewState was created when Views accessibility
didn't share any code with the rest of Chrome. Now there's
a lot more overlap, and every place where AXViewState is
consumed, it's just converted into AXNodeData. This change
eliminates AXViewState and uses AXNodeData in Views directly,
allowing us to delete some conversion code.

To replace AXViewState::set_value_callback, which was rather
awkward, add a virtual function to View that takes an
AXActionData, a general-purpose struct that encodes an
accessible action to take on an object. That provides a
cleaner solution for set_value_callback and allows for
more streamlining of code later.

I suggest reviewing these files first:
chrome/browser/ui/views/omnibox/omnibox_view_views.cc
chrome/browser/ui/views/omnibox/omnibox_view_views.h
ui/accessibility/ax_enums.idl
ui/accessibility/ax_node_data.cc
ui/accessibility/ax_node_data.h
ui/accessibility/ax_view_state.cc
ui/accessibility/ax_view_state.h
ui/views/accessibility/ax_view_obj_wrapper.cc
ui/views/accessibility/native_view_accessibility.cc
ui/views/controls/textfield/textfield.cc
ui/views/controls/textfield/textfield.h
ui/views/view.h

I've conveniently uploaded these into a separate
patch set (that doesn't compile) for easier review,
then the full change as a follow-up.

The rest of the files contain only mechanical changes.

BUG=662064
==========

to

==========
Replace ui::AXViewState with AXNodeData and AXActionData

This is a refactoring change.

Originally AXViewState was created when Views accessibility
didn't share any code with the rest of Chrome. Now there's
a lot more overlap, and every place where AXViewState is
consumed, it's just converted into AXNodeData. This change
eliminates AXViewState and uses AXNodeData in Views directly,
allowing us to delete some conversion code.

To replace AXViewState::set_value_callback, which was rather
awkward, this change adds a virtual function to View that
takes an AXActionData, a general-purpose struct that encodes an
accessible action to take on an object. That provides a
cleaner solution for set_value_callback and allows for
more streamlining of code later.

I suggest reviewing Patch Set 4 first, as that includes
only the non-mechanical changes.

BUG=662064
==========

[dmazzoni, 2016-11-03 18:04:01]

[email protected] changed reviewers:
+ [email protected]

[commit-bot: I haz the power, 2016-11-03 18:57:57]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-11-03 18:57:58]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sky, 2016-11-03 20:10:14]

https://codereview.chromium.org/2477463003/diff/80001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2477463003/diff/80001/ui/views/view.h#newcode950
ui/views/view.h:950: virtual void GetAccessibleNodeData(ui::AXNodeData*
node_data) {}
While you're changing all this code is there a reason this function needs to
take a ui::AXNodeData rather than returning a unique_ptr<AXNodeData>?

https://codereview.chromium.org/2477463003/diff/80001/ui/views/view.h#newcode954
ui/views/view.h:954: virtual void HandleAccessibleAction(const ui::AXActionData&
action_data) {}
Would this be easier for subclassing if it returned a boolean? I'm wondering if
doing that means subclass code could look like:

if (SuperClass::HandleAccessibleAction(action_data))
  return;

....
?

[dmazzoni, 2016-11-03 22:27:37]

https://codereview.chromium.org/2477463003/diff/80001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2477463003/diff/80001/ui/views/view.h#newcode950
ui/views/view.h:950: virtual void GetAccessibleNodeData(ui::AXNodeData*
node_data) {}
On 2016/11/03 20:10:14, sky wrote:
> While you're changing all this code is there a reason this function needs to
> take a ui::AXNodeData rather than returning a unique_ptr<AXNodeData>? 

I'm leaning against doing that because this matches the
interface elsewhere in the code, and returning
unique_ptr<AXNodeData> would add an extra copy
(in AXViewObjWrapper::Serialize). I don't see any place
where it'd save a copy. I don't think it'd make any difference
in terms of where we need to include header files.

Did you see a particular benefit of returning
unique_ptr<AXNodeData>?

I don't mind making the change, but I wanted to point out
the reasons for the current interface. Up to you.

https://codereview.chromium.org/2477463003/diff/80001/ui/views/view.h#newcode954
ui/views/view.h:954: virtual void HandleAccessibleAction(const ui::AXActionData&
action_data) {}
On 2016/11/03 20:10:14, sky wrote:
> Would this be easier for subclassing if it returned a boolean? I'm wondering
if
> doing that means subclass code could look like:
> 
> if (SuperClass::HandleAccessibleAction(action_data))
>   return;
> 
> ....
> ?

Sure, done.

I added a comment to clarify that the success/failure
doesn't get communicated to the client.

[sky, 2016-11-03 23:12:47]

https://codereview.chromium.org/2477463003/diff/80001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2477463003/diff/80001/ui/views/view.h#newcode950
ui/views/view.h:950: virtual void GetAccessibleNodeData(ui::AXNodeData*
node_data) {}
On 2016/11/03 22:27:37, dmazzoni wrote:
> On 2016/11/03 20:10:14, sky wrote:
> > While you're changing all this code is there a reason this function needs to
> > take a ui::AXNodeData rather than returning a unique_ptr<AXNodeData>? 
> 
> I'm leaning against doing that because this matches the
> interface elsewhere in the code, and returning
> unique_ptr<AXNodeData> would add an extra copy
> (in AXViewObjWrapper::Serialize). I don't see any place
> where it'd save a copy. I don't think it'd make any difference
> in terms of where we need to include header files.
> 
> Did you see a particular benefit of returning
> unique_ptr<AXNodeData>?
> 
> I don't mind making the change, but I wanted to point out
> the reasons for the current interface. Up to you.

I didn't look everywhere, but in the couple of places I looked the calling code
is:

ui::AXNodeData node_data;
view->GetAccessibleNodeData(&node_data);

It seemed nicer if that is
node_data = view->GetAccessibleNodeData();

You know this code better than I though, so if you don't think it's worth it,
that makes sense. In fact looking at it again maybe it won't fly. There is code
like:

+      ui::AXNodeData node_data;
+      node_data.state = 0;
+      ime.first->GetAccessibleNodeData(&node_data);

Which won't work if you need to pass in real to GetAccessibleNodeData().

[sky, 2016-11-03 23:39:37]

LGTM

[dmazzoni, 2016-11-03 23:42:40]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2016-11-03 23:43:23]

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, 2016-11-04 00:26:15]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-11-04 00:26:17]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[dmazzoni, 2016-11-04 03:14:19]

The CQ bit was checked by [email protected]

[dmazzoni, 2016-11-04 03:14:21]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected]
Link to the patchset: https://codereview.chromium.org/2477463003/#ps120001
(title: "Fix compile error, Mac test error")

[commit-bot: I haz the power, 2016-11-04 03:14:48]

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, 2016-11-04 05:03:52]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-11-04 05:03:54]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[dmazzoni, 2016-11-04 06:14:30]

The CQ bit was checked by [email protected]

[dmazzoni, 2016-11-04 06:14:31]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected]
Link to the patchset: https://codereview.chromium.org/2477463003/#ps140001
(title: "Fix test")

[commit-bot: I haz the power, 2016-11-04 06:14:52]

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, 2016-11-04 07:24:48]

Description was changed from

==========
Replace ui::AXViewState with AXNodeData and AXActionData

This is a refactoring change.

Originally AXViewState was created when Views accessibility
didn't share any code with the rest of Chrome. Now there's
a lot more overlap, and every place where AXViewState is
consumed, it's just converted into AXNodeData. This change
eliminates AXViewState and uses AXNodeData in Views directly,
allowing us to delete some conversion code.

To replace AXViewState::set_value_callback, which was rather
awkward, this change adds a virtual function to View that
takes an AXActionData, a general-purpose struct that encodes an
accessible action to take on an object. That provides a
cleaner solution for set_value_callback and allows for
more streamlining of code later.

I suggest reviewing Patch Set 4 first, as that includes
only the non-mechanical changes.

BUG=662064
==========

to

==========
Replace ui::AXViewState with AXNodeData and AXActionData

This is a refactoring change.

Originally AXViewState was created when Views accessibility
didn't share any code with the rest of Chrome. Now there's
a lot more overlap, and every place where AXViewState is
consumed, it's just converted into AXNodeData. This change
eliminates AXViewState and uses AXNodeData in Views directly,
allowing us to delete some conversion code.

To replace AXViewState::set_value_callback, which was rather
awkward, this change adds a virtual function to View that
takes an AXActionData, a general-purpose struct that encodes an
accessible action to take on an object. That provides a
cleaner solution for set_value_callback and allows for
more streamlining of code later.

I suggest reviewing Patch Set 4 first, as that includes
only the non-mechanical changes.

BUG=662064
==========

[commit-bot: I haz the power, 2016-11-04 07:24:50]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-04 07:26:12]

Description was changed from

==========
Replace ui::AXViewState with AXNodeData and AXActionData

This is a refactoring change.

Originally AXViewState was created when Views accessibility
didn't share any code with the rest of Chrome. Now there's
a lot more overlap, and every place where AXViewState is
consumed, it's just converted into AXNodeData. This change
eliminates AXViewState and uses AXNodeData in Views directly,
allowing us to delete some conversion code.

To replace AXViewState::set_value_callback, which was rather
awkward, this change adds a virtual function to View that
takes an AXActionData, a general-purpose struct that encodes an
accessible action to take on an object. That provides a
cleaner solution for set_value_callback and allows for
more streamlining of code later.

I suggest reviewing Patch Set 4 first, as that includes
only the non-mechanical changes.

BUG=662064
==========

to

==========
Replace ui::AXViewState with AXNodeData and AXActionData

This is a refactoring change.

Originally AXViewState was created when Views accessibility
didn't share any code with the rest of Chrome. Now there's
a lot more overlap, and every place where AXViewState is
consumed, it's just converted into AXNodeData. This change
eliminates AXViewState and uses AXNodeData in Views directly,
allowing us to delete some conversion code.

To replace AXViewState::set_value_callback, which was rather
awkward, this change adds a virtual function to View that
takes an AXActionData, a general-purpose struct that encodes an
accessible action to take on an object. That provides a
cleaner solution for set_value_callback and allows for
more streamlining of code later.

I suggest reviewing Patch Set 4 first, as that includes
only the non-mechanical changes.

BUG=662064

Committed: https://crrev.com/13e5258f7ad40c6dee7bd64e92d72c1a3f683451
Cr-Commit-Position: refs/heads/master@{#429823}
==========

[commit-bot: I haz the power, 2016-11-04 07:26:15]

Patchset 8 (id:??) landed as
https://crrev.com/13e5258f7ad40c6dee7bd64e92d72c1a3f683451
Cr-Commit-Position: refs/heads/master@{#429823}