Scope and clean up uses of AccessibilityMode.

content/common/accessibility_mode.h

+// Copyright 2014 The Chromium Authors. All rights reserved.

[danakj, 2017/02/22 21:27:56]

2017

[dougt, 2017/02/22 22:19:59]

Done.

+// 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_

[danakj, 2017/02/22 21:27:56]

white space around the #ifndef#define

[dougt, 2017/02/22 22:19:59]

Done.

+#define CONTENT_COMMON_ACCESSIBILITY_MODE_ENUMS_H_
+// Note: keep enums in content/browser/resources/accessibility/accessibility.js
+// in sync with these two enums

[danakj, 2017/02/22 21:27:56]

These aren't enums anymore maybe clarify this comment

[dougt, 2017/02/22 22:19:58]

dropping comment.  Adding a PRESUBMIT in a follow up (Bug 692087).

+
+namespace content {
+
+class AccessibilityMode {
+ public:
+  const static int OFF = 0;

[danakj, 2017/02/22 21:27:56]

constexpr instead of const.

kOff, kNativeApis, etc.

[dougt, 2017/02/22 22:19:58]

Done.

[dougt, 2017/02/22 22:19:59]

Done.

+
+  // Native accessibility APIs, specific to each platform, are enabled.
+  // When this mode is set that indicates the presence of a third-party
+  // client accessing Chrome via accessibility APIs. However, unless one
+  // of the modes below is set, the contents of web pages will not be
+  // accessible.
+  const static int 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 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 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.
+  const static int 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 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 mode is off it's still possible to request inline
+  // text boxes for a specific node on-demand, asynchronously.
+  const static int 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.
+  const static int 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.
+  const static int HTML = 1 << 4;
+
+  const static int COMPLETE =
+      NATIVE_APIS | WEB_CONTENTS | INLINE_TEXT_BOXES | SCREEN_READER | HTML;
+
+  const static int WEB_CONTENTS_ONLY =
+      WEB_CONTENTS | INLINE_TEXT_BOXES | SCREEN_READER | HTML;
+
+  AccessibilityMode(int flag = OFF) : flags_(flag) {}

[danakj, 2017/02/22 21:27:56]

explicit

[dougt, 2017/02/22 22:19:58]

Done.

+
+  bool has(int flag) const { return (flags_ & flag) > 0; }
+  int get() const { return flags_; }

[danakj, 2017/02/22 21:27:56]

do you need a get()? it breaks the encapsulation

[dougt, 2017/02/22 22:19:58]

Sadly, for now yes.  The encapsulation gets broken in 3 places:

* When we IPC serialize (will got away with mojo)

* When we pass mode to chrome://accessibility (We could pass modes instead)

* Difference calculations for reporting. In browser_accessibility_state_impl.cc,
we want to know the absolute mode change and then report which flags have
changed.  Doing tihs on the raw int value is easier.  We could have a
difference() method that gives us the difference between two modes.

follow up for (2) and (3) ?

+
+  void set(int flag) { flags_ |= flag; }

[danakj, 2017/02/22 21:27:56]

set is maybe an odd name here, you're getting a union of modes, or adding modes,
not setting (replacing) the modes

[aboxhall, 2017/02/22 22:05:50]

Maybe set(int flag, bool state)?

[danakj, 2017/02/22 22:08:19]

If you go with a setter that can add or remove then toggle() could maybe go
away.

+  void set(AccessibilityMode& mode) { set(mode.get()); }
+
+  void clear() { set(OFF); }

[danakj, 2017/02/22 21:27:56]

this does nothing (a |= 0) == a

The OFF flag doesn't seem like it has a lot of value, and you could just use 0
instead in the places you need it (the constructor only?)

[aboxhall, 2017/02/22 22:05:50]

It's used in a bunch of places to avoid running code if accessibility is off.

[danakj, 2017/02/22 22:08:19]

An empty() function here could do that for us.

[dougt, 2017/02/22 22:19:58]

Removed.
What Alice said.

+
+  void toggle(int flag) { flags_ ^= flag; }
+
+  bool operator==(AccessibilityMode rhs) const {

[danakj, 2017/02/22 21:27:56]

== should come with a != too

[dougt, 2017/02/22 22:19:59]

Done.

+    if (flags_ == rhs.flags_)
+      return true;
+    return false;
+  }
+
+  bool operator==(int rhs) const {
+    if (flags_ == rhs)
+      return true;
+    return false;
+  }
+
+  bool operator!=(int rhs) const {
+    if (flags_ == rhs)

[danakj, 2017/02/22 21:27:56]

define operator!= in terms of !operator==(). so return !(*this == rhs)

comparing flags_ breaks the encapsulation again though. do you need them?

+      return false;
+    return true;
+  }
+
+  void operator=(int flags) { flags_ = flags; }

[danakj, 2017/02/22 21:27:56]

i think you'd do better to have operator=(AccessibilityMode), this breaks
encapsulation and may do conversions you didn't intend

[dougt, 2017/02/22 22:19:58]

What do you think if this also used:

[dougt, 2017/02/22 22:21:27]

setMode(int mode, bool value)

+
+ private:
+  int flags_;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_COMMON_ACCESSIBILITY_MODE_ENUMS_H_