Use Blink accessibility enums in Chromium

Use Blink accessibility enums in Chromium

This is a refactoring-only change! Eliminates enums for
accessibility roles, states, and notifications and uses
equivalent enums from Blink instead. Saves several
giant switch statements, saves a few characters per
line, and makes future changes to roles and states simpler.
Another benefit is that you can search for a role, state,
or event and find all of the places where it's used throughout
the code at once.

Also renames WebAccessibilityObject -> WebAXObject
and notification -> event in the context of accessibility.

BUG=269034
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221295

[aboxhall, 2013-08-29 00:59:06]

Still a bit confused as to what's coming from AccessibilityNodeData vs.
WebAXEnums so please ignore suggestions to remove the accessibility_node_data.h
header file where it doesn't make sense.

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/accessibility_win_browsertest.cc (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/accessibility_win_browsertest.cc:765:
IA2_ROLE_TOGGLE_BUTTOn, STATE_SYSTEM_FOCUSABLE | STATE_SYSTEM_PRESSED);
Something funny happened here.

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility.h (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility.h:15: #include
"content/common/accessibility_node_data.h"
Should this point to the webkit enums instead?

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_android.cc (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_android.cc:10: #include
"content/common/accessibility_node_data.h"
Similarly.

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_cocoa.h (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_cocoa.h:13: #include
"content/common/accessibility_node_data.h"
Should this be removed?

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_cocoa.mm:199:
@"AXLandmarkApplication" },
Does this fit on previous line now? (and below)

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_cocoa.mm:519:
GetState(browserAccessibility_, WebKit::WebAXStateEnabled)];
What's the difference between enabled and available?

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_manager.h (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_manager.h:13: #include
"content/common/accessibility_node_data.h"
Remove?

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_manager_mac.mm (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_manager_mac.mm:115: default:
Are we concerned about not handled event types here?

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_manager_unittest.cc
(right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_manager_unittest.cc:10:
#include "content/common/accessibility_node_data.h"
Remove?

https://codereview.chromium.org/23651003/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.h:20: #include
"content/common/accessibility_node_data.h"
Remove?

https://codereview.chromium.org/23651003/diff/3001/content/renderer/accessibi...
File content/renderer/accessibility/renderer_accessibility_complete.cc (right):

https://codereview.chromium.org/23651003/diff/3001/content/renderer/accessibi...
content/renderer/accessibility/renderer_accessibility_complete.cc:543: // Make
sure the browser gets a event when the scroll
an event

[dmazzoni, 2013-08-29 03:05:15]

On Wed, Aug 28, 2013 at 5:59 PM, <aboxhall@chromium.org> wrote:

> Still a bit confused as to what's coming from AccessibilityNodeData vs.
> WebAXEnums so please ignore suggestions to remove the
> accessibility_node_data.h
> header file where it doesn't make sense.
>

My eventual goal: AccessibilityNodeData will just be a struct, and all of
the enums will come from Blink.

Currently: role, state, and event enums come from Blink, but attribute
enums stay in Chromium.

I will try removing header files in a few places.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2013-08-29 04:47:14]

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/accessibility_win_browsertest.cc (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/accessibility_win_browsertest.cc:765:
IA2_ROLE_TOGGLE_BUTTOn, STATE_SYSTEM_FOCUSABLE | STATE_SYSTEM_PRESSED);
On 2013/08/29 00:59:06, aboxhall wrote:
> Something funny happened here.

Done.

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility.h (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility.h:15: #include
"content/common/accessibility_node_data.h"
On 2013/08/29 00:59:06, aboxhall wrote:
> Should this point to the webkit enums instead?

In this case it needs both.

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_android.cc (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_android.cc:10: #include
"content/common/accessibility_node_data.h"
On 2013/08/29 00:59:06, aboxhall wrote:
> Similarly.

Most of these files still use AccessibilityNodeData for the attribute enums and
sometimes because they access the struct itself.

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_cocoa.h (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_cocoa.h:13: #include
"content/common/accessibility_node_data.h"
On 2013/08/29 00:59:06, aboxhall wrote:
> Should this be removed?

Yes! This one was superfluous.

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_cocoa.mm:199:
@"AXLandmarkApplication" },
On 2013/08/29 00:59:06, aboxhall wrote:
> Does this fit on previous line now? (and below)

Yes, thanks. I wrapped above and forgot to check this section.

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_cocoa.mm:519:
GetState(browserAccessibility_, WebKit::WebAXStateEnabled)];
On 2013/08/29 00:59:06, aboxhall wrote:
> What's the difference between enabled and available?

Enabled seems a lot more clear to me (and web-like). Windows calls it
"unavailable" when a control is disabled.

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_manager.h (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_manager.h:13: #include
"content/common/accessibility_node_data.h"
On 2013/08/29 00:59:06, aboxhall wrote:
> Remove?

No, this one is used.

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_manager_mac.mm (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_manager_mac.mm:115: default:
On 2013/08/29 00:59:06, aboxhall wrote:
> Are we concerned about not handled event types here?

Unfortunately that'd make it impossible to add new enums in Blink. One downside
of moving the enums there.

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_manager_unittest.cc
(right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_manager_unittest.cc:10:
#include "content/common/accessibility_node_data.h"
On 2013/08/29 00:59:06, aboxhall wrote:
> Remove?

No, it's used.

https://codereview.chromium.org/23651003/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.h:20: #include
"content/common/accessibility_node_data.h"
On 2013/08/29 00:59:06, aboxhall wrote:
> Remove?

Used.

https://codereview.chromium.org/23651003/diff/3001/content/renderer/accessibi...
File content/renderer/accessibility/renderer_accessibility_complete.cc (right):

https://codereview.chromium.org/23651003/diff/3001/content/renderer/accessibi...
content/renderer/accessibility/renderer_accessibility_complete.cc:543: // Make
sure the browser gets a event when the scroll
On 2013/08/29 00:59:06, aboxhall wrote:
> an event

Done.

[aboxhall, 2013-08-29 15:34:08]

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility.h (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility.h:15: #include
"content/common/accessibility_node_data.h"
On 2013/08/29 04:47:15, Dominic Mazzoni wrote:
> On 2013/08/29 00:59:06, aboxhall wrote:
> > Should this point to the webkit enums instead?
> 
> In this case it needs both.

Where are the WebKit enums coming from in this file?

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_manager_mac.mm (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_manager_mac.mm:115: default:
On 2013/08/29 04:47:15, Dominic Mazzoni wrote:
> On 2013/08/29 00:59:06, aboxhall wrote:
> > Are we concerned about not handled event types here?
> 
> Unfortunately that'd make it impossible to add new enums in Blink. One
downside
> of moving the enums there.

In the sense that we need to do changes in two parts, and the Blink part would
break Chrome?

How about a warning here, then? Then at least we'll get some warning while the
two are out of sync.

[dmazzoni, 2013-08-29 17:44:58]

Feedback addressed, see below.

One last note: I needed to fix one windows test expectation - the expectation
was wrong, this refactoring fixed a bug where aria role=application was mapped
to the wrong role (because there were two role enums confusingly with
application in the name).

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility.h (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility.h:15: #include
"content/common/accessibility_node_data.h"
On 2013/08/29 15:34:09, aboxhall wrote:
> On 2013/08/29 04:47:15, Dominic Mazzoni wrote:
> > On 2013/08/29 00:59:06, aboxhall wrote:
> > > Should this point to the webkit enums instead?
> > 
> > In this case it needs both.
> 
> Where are the WebKit enums coming from in this file?

Well, accessibility_node_data.h has to include WebAXEnums.h already, so from
there - but you're right, this file should include WebAXEnums.h explicitly
(include what you use)

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_manager_mac.mm (right):

https://codereview.chromium.org/23651003/diff/3001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_manager_mac.mm:115: default:
On 2013/08/29 15:34:09, aboxhall wrote:
> > Unfortunately that'd make it impossible to add new enums in Blink. One
> downside
> > of moving the enums there.
> 
> In the sense that we need to do changes in two parts, and the Blink part would
> break Chrome?

That's right, it'd be impossible to add a new enum to Blink without breaking
Chrome.

> How about a warning here, then? Then at least we'll get some warning while the
> two are out of sync.

Sure, done.

[aboxhall, 2013-08-29 17:47:48]

lgtm

[commit-bot: I haz the power, 2013-08-30 13:58:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/23651003/46001

[commit-bot: I haz the power, 2013-08-30 14:09:06]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[dmazzoni, 2013-08-30 14:51:38]

+piman for owners review of content/

[dmazzoni, 2013-09-03 20:21:05]

+jam for OWNERS review of content/

[jam, 2013-09-03 20:45:40]

lgtm

[commit-bot: I haz the power, 2013-09-03 22:03:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/23651003/46001

[commit-bot: I haz the power, 2013-09-03 22:03:50]

Failed to apply patch for chrome/renderer/chrome_render_view_observer.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/renderer/chrome_render_view_observer.cc
  Hunk #1 succeeded at 40 (offset 2 lines).
  Hunk #2 FAILED at 55.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/renderer/chrome_render_view_observer.cc.rej

Patch:       chrome/renderer/chrome_render_view_observer.cc
Index: chrome/renderer/chrome_render_view_observer.cc
diff --git a/chrome/renderer/chrome_render_view_observer.cc
b/chrome/renderer/chrome_render_view_observer.cc
index
09cbb91b01fcec44d66afaa11f46d02aa70eb52b..f4bdd266a18ada14f5e2f25a463d004aa0d31824
100644
--- a/chrome/renderer/chrome_render_view_observer.cc
+++ b/chrome/renderer/chrome_render_view_observer.cc
@@ -38,7 +38,7 @@
 #include "third_party/WebKit/public/platform/WebString.h"
 #include "third_party/WebKit/public/platform/WebURLRequest.h"
 #include "third_party/WebKit/public/platform/WebVector.h"
-#include "third_party/WebKit/public/web/WebAccessibilityObject.h"
+#include "third_party/WebKit/public/web/WebAXObject.h"
 #include "third_party/WebKit/public/web/WebDataSource.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebElement.h"
@@ -55,7 +55,7 @@
 #include "v8/include/v8-testing.h"
 #include "webkit/glue/webkit_glue.h"
 
-using WebKit::WebAccessibilityObject;
+using WebKit::WebAXObject;
 using WebKit::WebCString;
 using WebKit::WebDataSource;
 using WebKit::WebDocument;

[commit-bot: I haz the power, 2013-09-04 16:36:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/23651003/56001

[dmazzoni, 2013-09-04 17:20:59]

+jschuh for content/common/accessibility_messages.h
+jamesr for content/browser/DEPS (including
third_party/WebKit/public/web/WebAXEnums.h)

[jschuh, 2013-09-04 19:03:12]

ipc security lgtm. two new enums

[jamesr, 2013-09-04 19:37:35]

lgtm for DEPS

[commit-bot: I haz the power, 2013-09-04 19:47:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/23651003/56001

[commit-bot: I haz the power, 2013-09-04 23:32:24]

Change committed as 221295