Clean up accessibility enums for use in Chromium.

Clean up accessibility enums for use in Chromium.

The goal of this change is to make it possible for Chromium to use
Blink accessibility role, state, and event enums throughout
its codebase, rather than copying to separate enums.

* Rename the enums in public/web so they're more concise, e.g.
    WebAccessibilityRoleDocumentArticle -> WebAXTypes::ArticleRole
    WebAccessibilityNotificationLoadComplete -> WebAXTypes::LoadComplete
* Add missing enum values currently used in Chromium, delete unused values.
* Rename a few enum values that were unclear or confusing.

This will require a cleanup change after Chromium switches over.

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

[aboxhall, 2013-08-07 15:58:10]

https://codereview.chromium.org/22331005/diff/4001/Source/core/accessibility/...
File Source/core/accessibility/AccessibilityObject.h (left):

https://codereview.chromium.org/22331005/diff/4001/Source/core/accessibility/...
Source/core/accessibility/AccessibilityObject.h:172: WebCoreLinkRole,
What was this role doing previously? I see there was already a LinkRole.

https://codereview.chromium.org/22331005/diff/4001/Source/core/editing/Append...
File Source/core/editing/AppendNodeCommand.cpp (left):

https://codereview.chromium.org/22331005/diff/4001/Source/core/editing/Append...
Source/core/editing/AppendNodeCommand.cpp:47: static void
sendAXTextChangedIgnoringLineBreaks(Node* node, AXObjectCache::AXTextChange
textChange)
Why don't we need to special case text changes any more?

https://codereview.chromium.org/22331005/diff/4001/Source/web/ChromeClientImp...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/22331005/diff/4001/Source/web/ChromeClientImp...
Source/web/ChromeClientImpl.cpp:809:
m_webView->client()->postAccessibilityNotificationNew(WebAccessibilityObject(obj),
toAXNotification(notification));
Should this have a TODO to remove the duplication once the cleanup is complete?

https://codereview.chromium.org/22331005/diff/4001/Source/web/WebAccessibilit...
File Source/web/WebAccessibilityObject.cpp (right):

https://codereview.chromium.org/22331005/diff/4001/Source/web/WebAccessibilit...
Source/web/WebAccessibilityObject.cpp:594: // Deprecated.
"Use role() instead."?

[dmazzoni, 2013-08-07 16:23:53]

https://codereview.chromium.org/22331005/diff/4001/Source/core/accessibility/...
File Source/core/accessibility/AccessibilityObject.h (left):

https://codereview.chromium.org/22331005/diff/4001/Source/core/accessibility/...
Source/core/accessibility/AccessibilityObject.h:172: WebCoreLinkRole,
On 2013/08/07 15:58:10, aboxhall wrote:
> What was this role doing previously? I see there was already a LinkRole.

No idea why WebKit had both. LinkRole was never used as far as I can tell, so
I'm just mapping WebCoreLinkRole to LinkRole everywhere.

https://codereview.chromium.org/22331005/diff/4001/Source/core/editing/Append...
File Source/core/editing/AppendNodeCommand.cpp (left):

https://codereview.chromium.org/22331005/diff/4001/Source/core/editing/Append...
Source/core/editing/AppendNodeCommand.cpp:47: static void
sendAXTextChangedIgnoringLineBreaks(Node* node, AXObjectCache::AXTextChange
textChange)
On 2013/08/07 15:58:10, aboxhall wrote:
> Why don't we need to special case text changes any more?

We never used them in Chromium. In WebKit, it looks like they
were only used for ATK, and I think we can reconstruct the
information we need on the browser side. I'm deleting them now
because the names of the enums conflicted.

https://codereview.chromium.org/22331005/diff/4001/Source/web/ChromeClientImp...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/22331005/diff/4001/Source/web/ChromeClientImp...
Source/web/ChromeClientImpl.cpp:809:
m_webView->client()->postAccessibilityNotificationNew(WebAccessibilityObject(obj),
toAXNotification(notification));
On 2013/08/07 15:58:10, aboxhall wrote:
> Should this have a TODO to remove the duplication once the cleanup is
complete?

Sure.

https://codereview.chromium.org/22331005/diff/4001/Source/web/WebAccessibilit...
File Source/web/WebAccessibilityObject.cpp (right):

https://codereview.chromium.org/22331005/diff/4001/Source/web/WebAccessibilit...
Source/web/WebAccessibilityObject.cpp:594: // Deprecated.
On 2013/08/07 15:58:10, aboxhall wrote:
> "Use role() instead."?

Sure.

[tfarina, 2013-08-07 17:22:52]

https://codereview.chromium.org/22331005/diff/4001/public/web/WebAccessibilit...
File public/web/WebAccessibilityEnums.h (right):

https://codereview.chromium.org/22331005/diff/4001/public/web/WebAccessibilit...
public/web/WebAccessibilityEnums.h:34: #include "../platform/WebCommon.h"
do you need this include here?

[jamesr, 2013-08-07 17:26:09]

https://codereview.chromium.org/22331005/diff/4001/public/web/WebAccessibilit...
File public/web/WebAccessibilityEnums.h (right):

https://codereview.chromium.org/22331005/diff/4001/public/web/WebAccessibilit...
public/web/WebAccessibilityEnums.h:40: enum AXNotification {
all public Blink API types should have a 'Web' prefix

https://codereview.chromium.org/22331005/diff/4001/public/web/WebAccessibilit...
public/web/WebAccessibilityEnums.h:41: AXActiveDescendantChanged,
all enum values in the Blink public API should be prefixed with the name of the
enum.

This used to all be documented on the wiki, but I'm not sure where that document
ended up :/

[dmazzoni, 2013-08-07 18:27:29]

https://codereview.chromium.org/22331005/diff/4001/public/web/WebAccessibilit...
File public/web/WebAccessibilityEnums.h (right):

https://codereview.chromium.org/22331005/diff/4001/public/web/WebAccessibilit...
public/web/WebAccessibilityEnums.h:34: #include "../platform/WebCommon.h"
On 2013/08/07 17:22:52, tfarina wrote:
> do you need this include here?

Done.

https://codereview.chromium.org/22331005/diff/4001/public/web/WebAccessibilit...
public/web/WebAccessibilityEnums.h:40: enum AXNotification {
On 2013/08/07 17:26:09, jamesr wrote:
> all public Blink API types should have a 'Web' prefix

No problem.

https://codereview.chromium.org/22331005/diff/4001/public/web/WebAccessibilit...
public/web/WebAccessibilityEnums.h:41: AXActiveDescendantChanged,
On 2013/08/07 17:26:09, jamesr wrote:
> all enum values in the Blink public API should be prefixed with the name of
the
> enum.
> 
> This used to all be documented on the wiki, but I'm not sure where that
document
> ended up :/

OK, how about this?

[jamesr, 2013-08-07 18:29:30]

Will everything move over to WebAX.... or will some be left WebAccessibility...?
 I think having a mix will be more confusing.

[dmazzoni, 2013-08-07 18:34:20]

On 2013/08/07 18:29:30, jamesr wrote:
> Will everything move over to WebAX.... or will some be left
WebAccessibility...?
>  I think having a mix will be more confusing.

All enums are now WebAX... - the old enums can be deleted as soon as Chromium
switches over.

The only remaining type in public/web with "WebAccessibility..." is
WebAccessibilityObject - do you think I should change this to WebAXObject in the
same change?

[jamesr, 2013-08-09 22:12:23]

If we're moving to a consistent naming scheme across the board then I don't
think it all has to happen in this patch

[jamesr, 2013-08-09 22:13:05]

The new file should be called WebAXEnums.h, shouldn't it?

[dmazzoni, 2013-08-14 19:17:54]

Maybe it'd be better to have them in a wrapper class -
that's consistent with a lot of other WebKit public
headers, and I think it's a bit more readable. I also
made the header file name match and added a typedef to
rename WebAXObject too.

Please take a look.

[dmazzoni, 2013-08-20 15:30:49]

This look okay?

[aboxhall, 2013-08-20 19:54:11]

lgtm

https://codereview.chromium.org/22331005/diff/29001/public/web/WebAXObject.h
File public/web/WebAXObject.h (right):

https://codereview.chromium.org/22331005/diff/29001/public/web/WebAXObject.h#...
public/web/WebAXObject.h:31: #ifndef WebAXObject_h
Did this detect a strange base version? Is git calling this a move or an add?

[jamesr, 2013-08-21 02:22:52]

I'm sorry for the very slow review but this still doesn't follow blink API
conventions.  Consistency is important.

https://codereview.chromium.org/22331005/diff/29001/Source/web/ChromeClientIm...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/22331005/diff/29001/Source/web/ChromeClientIm...
Source/web/ChromeClientImpl.cpp:815: if (obj) {
early return on !obj is more readable and requires less indentation

https://codereview.chromium.org/22331005/diff/29001/Source/web/ChromeClientIm...
Source/web/ChromeClientImpl.cpp:816: // TODO(dmazzoni): remove this first call
once Chromium has
Blink style is FIXME:, not TODO(name).  You have a bug link here which is more
useful than a person.

https://codereview.chromium.org/22331005/diff/29001/public/web/WebAXObject.h
File public/web/WebAXObject.h (right):

https://codereview.chromium.org/22331005/diff/29001/public/web/WebAXObject.h#...
public/web/WebAXObject.h:31: #ifndef WebAXObject_h
On 2013/08/20 19:54:11, aboxhall wrote:
> Did this detect a strange base version? Is git calling this a move or an add?

Please upload again and use the --similarity and/or --(no-)find-copies flags to
fix this

https://codereview.chromium.org/22331005/diff/29001/public/web/WebAXTypes.h
File public/web/WebAXTypes.h (right):

https://codereview.chromium.org/22331005/diff/29001/public/web/WebAXTypes.h#n...
public/web/WebAXTypes.h:36: class WebAXTypes {
why are the enums inside a class?  I think it'd be more typical to have this be
in the enum type, i.e.

enum WebAXEvent {
  WebAXEventAlert,
  ...
};

https://codereview.chromium.org/22331005/diff/29001/public/web/WebAXTypes.h#n...
public/web/WebAXTypes.h:42: ActiveDescendantChanged,
this doesn't follow the blink public API enum convention.  In the blink public
API, prefix the enum values with the name of the enum.  I feel like I've given
this feedback before :(

https://codereview.chromium.org/22331005/diff/29001/public/web/WebAccessibili...
File public/web/WebAccessibilityObject.h (right):

https://codereview.chromium.org/22331005/diff/29001/public/web/WebAccessibili...
public/web/WebAccessibilityObject.h:56: // TODO(dmazzoni): rename WebAXObject
(http://crbug.com/269034).
TODO() -> FIXME

[dmazzoni, 2013-08-21 03:07:57]

I'm not deliberately ignoring your feedback. I read through
all of the blink public APIs and tried to find something
consistent with existing patterns.

https://codereview.chromium.org/22331005/diff/29001/public/web/WebAXObject.h
File public/web/WebAXObject.h (right):

https://codereview.chromium.org/22331005/diff/29001/public/web/WebAXObject.h#...
public/web/WebAXObject.h:31: #ifndef WebAXObject_h
On 2013/08/20 19:54:11, aboxhall wrote:
> Did this detect a strange base version? Is git calling this a move or an add?

Yeah, git treated this as a copy-and-modify - probably because it's a short file
with a lot of boilerplate in common.

https://codereview.chromium.org/22331005/diff/29001/public/web/WebAXTypes.h
File public/web/WebAXTypes.h (right):

https://codereview.chromium.org/22331005/diff/29001/public/web/WebAXTypes.h#n...
public/web/WebAXTypes.h:36: class WebAXTypes {
On 2013/08/21 02:22:52, jamesr wrote:
> why are the enums inside a class?  I think it'd be more typical to have this
be
> in the enum type, i.e.
> 
> enum WebAXEvent {
>   WebAXEventAlert,
>   ...
> };

It's more convenient to use within Chromium because I can add
"using WebKit::WebAXTypes" to the top of files that use these types
a lot.

https://codereview.chromium.org/22331005/diff/29001/public/web/WebAXTypes.h#n...
public/web/WebAXTypes.h:42: ActiveDescendantChanged,
On 2013/08/21 02:22:52, jamesr wrote:
> this doesn't follow the blink public API enum convention.  In the blink public
> API, prefix the enum values with the name of the enum.  I feel like I've given
> this feedback before :(

It's consistent with all of these, and a dozen more:

class WebApplicationCacheHost {
    enum Status {
        Uncached,
        Idle,
        ...

class WebDOMEvent {
    enum PhaseType {
        CapturingPhase     = 1,
        AtTarget           = 2,
        ...

class WebInputElement : public WebFormControlElement {
    enum SpeechInputState {
        Idle,
        Recording,
        ...

class WebInputEvent {
    enum Type {
        Undefined = -1,
        MouseDown,
        ...

struct WebMediaPlayerAction {
    enum Type {
        Unknown,
        Play,
        ...

class WebPageSerializerClient {
    enum PageSerializationStatus {
        CurrentFrameIsNotFinished,
        CurrentFrameIsFinished,
        ...

[jamesr, 2013-08-21 03:13:15]

You're welcome to fix those enums too.

not lgtm

[dmazzoni, 2013-08-22 20:58:22]

Reverted to previous patch set with top-level enums that follow the
Blink public API style, and addressed other feedback.

https://codereview.chromium.org/22331005/diff/29001/Source/web/ChromeClientIm...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/22331005/diff/29001/Source/web/ChromeClientIm...
Source/web/ChromeClientImpl.cpp:815: if (obj) {
On 2013/08/21 02:22:52, jamesr wrote:
> early return on !obj is more readable and requires less indentation

Done.

https://codereview.chromium.org/22331005/diff/29001/Source/web/ChromeClientIm...
Source/web/ChromeClientImpl.cpp:816: // TODO(dmazzoni): remove this first call
once Chromium has
On 2013/08/21 02:22:52, jamesr wrote:
> Blink style is FIXME:, not TODO(name).  You have a bug link here which is more
> useful than a person.

TODO changed to FIXME throughout.

https://codereview.chromium.org/22331005/diff/29001/public/web/WebAXObject.h
File public/web/WebAXObject.h (right):

https://codereview.chromium.org/22331005/diff/29001/public/web/WebAXObject.h#...
public/web/WebAXObject.h:31: #ifndef WebAXObject_h
On 2013/08/21 02:22:52, jamesr wrote:
> On 2013/08/20 19:54:11, aboxhall wrote:
> > Did this detect a strange base version? Is git calling this a move or an
add?
> 
> Please upload again and use the --similarity and/or --(no-)find-copies flags
to
> fix this

Done.

[jamesr, 2013-08-22 20:59:58]

public/ lgtm. I didn't look at the rest

[dmazzoni, 2013-08-22 21:01:50]

Thanks. Alice, please take a final pass through the rest of
the change when you have a chance.

[aboxhall, 2013-08-26 16:38:37]

lgtm

So much more readable now!

[commit-bot: I haz the power, 2013-08-26 19:04:15]

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

[commit-bot: I haz the power, 2013-08-26 19:39:48]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_blink_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-26 22:36:04]

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

[commit-bot: I haz the power, 2013-08-27 01:01:03]

Change committed as 156740