Scope and clean up uses of AccessibilityMode.

content/common/accessibility_mode_enums.h

Index: content/common/accessibility_mode_enums.h
diff --git a/content/common/accessibility_mode_enums.h b/content/common/accessibility_mode_enums.h
index 46f8b4bb1eddcf1258f0423165f7e3d8c353cb5d..f04d6e033c44fbe5af874900942cf8955753fe80 100644
--- a/content/common/accessibility_mode_enums.h
+++ b/content/common/accessibility_mode_enums.h
@@ -1,68 +1,73 @@
 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
-
 #ifndef CONTENT_COMMON_ACCESSIBILITY_MODE_ENUMS_H_
 #define CONTENT_COMMON_ACCESSIBILITY_MODE_ENUMS_H_
-
 // Note: keep enums in content/browser/resources/accessibility/accessibility.js
 // in sync with these two enums.
-enum AccessibilityModeFlag {
+enum AccessibilityMode : int {

[danakj, 2017/02/17 00:30:39]

If this were an enum, then I think what you want is

enum class AccessibilityMode {
  ..
};

You don't need to specify the underlying type, this may only make it larger than
it needs to be. Unless you really require it to be an int which it doesn't seem
like?

However, this isn't so much an enum (a list of things of which you choose one)
as it is a scoped set of constants. Instead of an enum class you could do this

struct AccessibilityMode {
  enum { NATIVE_APIS = 1 << 0 };
  enum { WEB_CONTENTS = 1 << 1 };
  enum { INLINE_TEXT_BOXES = 1 << 2 };
  ..etc..
};

Then |&^ work on the constants still, and you get scoping, and you can't write a
switch statement on the enum for instance, which doesn't make sense here. And at
that point you could turn NATIVE_APIS into kNativeApis and so on, as these are
clearly constants now instead of an enum.

[dougt, 2017/02/17 17:46:48]

I do see your point about "not being a list of things choose one".  I do think
it's a pretty common pattern to scope values, bit flags, ect., in an enum such
as I am doing here.

One of the values of staying with enums, or enum class as you suggest, is that
it is more strongly typed than just an int. For example, we have to be explicit
when assigning or passing values into a function that take a AccessibilityMode. 
In this CL, there are two places where this came up.  One, where we were passing
|0| into a method which we needed to change to be more explict (yes, good), and
another where we can set up IPC so that it does "some" validation on the input
before posting to the other process.

I would rather not just regress to using #define's or global constant values.

WDYT?

[danakj, 2017/02/17 17:50:57]

That sounds fair, LGTM. I do suggest "enum class" tho and drop the ":int".

[dougt, 2017/02/17 22:30:12]

Moving to enum class would mean many/all of our unitary operands woudldn't work
as is. For example:

 if (accessibility_mode & AccessibilityMode::NATIVE_APIS) {
    ....

Generates a invalid operands to binary expression compile time error. since the
result of the & is a class... and not something that can be used in an if ().

Maybe in the future we would convert this enum directly to a class and have
methods to check for various states.  But I am not sure the value of that yet.

   // Native accessibility APIs, specific to each platform, are enabled.
-  // When this flag is set that indicates the presence of a third-party
+  // When this mode is set that indicates the presence of a third-party
   // client accessing Chrome via accessibility APIs. However, unless one
-  // of the flags below is set, the contents of web pages will not be
+  // of the modes below is set, the contents of web pages will not be
   // accessible.
-  ACCESSIBILITY_MODE_FLAG_NATIVE_APIS = 1 << 0,
+  NATIVE_APIS = 1 << 0,
 
   // The renderer process will generate an accessibility tree containing
   // basic information about all nodes, including role, name, value,
-  // state, and location. This is the minimum flag required in order for
-  // web contents to be accessible, and the remaining flags are meaningless
+  // state, and location. This is the minimum mode required in order for
+  // web contents to be accessible, and the remaining modes are meaningless
   // unless this one is set.
   //
-  // Note that sometimes this flag will be set when
-  // ACCESSIBILITY_MODE_FLAG_NATIVE_APIS is not, when the content layer embedder
-  // is providing accessibility support via some other mechanism other than
-  // what's implemented in content/browser.
-  ACCESSIBILITY_MODE_FLAG_WEB_CONTENTS = 1 << 1,
+  // Note that sometimes this mode will be set when NATIVE_APIS is not, when the
+  // content layer embedder is providing accessibility support via some other
+  // mechanism other than what's implemented in content/browser.
+  WEB_CONTENTS = 1 << 1,
 
   // The accessibility tree will contain inline text boxes, which are
   // necessary to expose information about line breaks and word boundaries.
-  // Without this flag, you can retrieve the plaintext value of a text field
+  // Without this mode, you can retrieve the plaintext value of a text field
   // but not the information about how it's broken down into lines.
   //
-  // Note that when this flag is off it's still possible to request inline
+  // Note that when this mode is off it's still possible to request inline
   // text boxes for a specific node on-demand, asynchronously.
-  ACCESSIBILITY_MODE_FLAG_INLINE_TEXT_BOXES = 1 << 2,
+  INLINE_TEXT_BOXES = 1 << 2,
 
   // The accessibility tree will contain extra accessibility
   // attributes typically only needed by screen readers and other
   // assistive technology for blind users. Examples include text style
   // attributes, table cell information, live region properties, range
   // values, and relationship attributes.
-  ACCESSIBILITY_MODE_FLAG_SCREEN_READER = 1 << 3,
+  SCREEN_READER = 1 << 3,
 
   // The accessibility tree will contain the HTML tag name and HTML attributes
   // for all accessibility nodes that come from web content.
-  ACCESSIBILITY_MODE_FLAG_HTML = 1 << 4,
+  HTML = 1 << 4,
+
+  OFF = 0,
+
+  COMPLETE =
+      NATIVE_APIS | WEB_CONTENTS | INLINE_TEXT_BOXES | SCREEN_READER | HTML,
+
+  WEB_CONTENTS_ONLY = WEB_CONTENTS | INLINE_TEXT_BOXES | SCREEN_READER | HTML
 };
 
-typedef int AccessibilityMode;
+inline AccessibilityMode operator|(AccessibilityMode a, AccessibilityMode b) {
+  return static_cast<AccessibilityMode>(int(a) | int(b));

[dmazzoni, 2017/02/17 00:21:50]

nit: use static_cast<int>(a), etc.

[dougt, 2017/02/17 22:30:12]

Done.

+}
 
-const AccessibilityMode AccessibilityModeOff = 0;
+inline AccessibilityMode& operator|=(AccessibilityMode& a,
+                                     AccessibilityMode b) {
+  return a = a | b;
+}
 
-const AccessibilityMode ACCESSIBILITY_MODE_COMPLETE =
-    ACCESSIBILITY_MODE_FLAG_NATIVE_APIS |
-    ACCESSIBILITY_MODE_FLAG_WEB_CONTENTS |
-    ACCESSIBILITY_MODE_FLAG_INLINE_TEXT_BOXES |
-    ACCESSIBILITY_MODE_FLAG_SCREEN_READER |
-    ACCESSIBILITY_MODE_FLAG_HTML;
+inline AccessibilityMode operator^(AccessibilityMode a, AccessibilityMode b) {
+  return static_cast<AccessibilityMode>(int(a) ^ int(b));

[dmazzoni, 2017/02/17 00:21:50]

same

[dougt, 2017/02/17 22:30:12]

Done.

+}
 
-const AccessibilityMode ACCESSIBILITY_MODE_WEB_CONTENTS_ONLY =
-    ACCESSIBILITY_MODE_FLAG_WEB_CONTENTS |
-    ACCESSIBILITY_MODE_FLAG_INLINE_TEXT_BOXES |
-    ACCESSIBILITY_MODE_FLAG_SCREEN_READER |
-    ACCESSIBILITY_MODE_FLAG_HTML;
+inline AccessibilityMode& operator^=(AccessibilityMode& a,
+                                     AccessibilityMode b) {
+  return a = a ^ b;
+}
 
 #endif  // CONTENT_COMMON_ACCESSIBILITY_MODE_ENUMS_H_