Scope and clean up uses of AccessibilityMode.

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 6749aafe832138b29352a637eb1107f508b56817..bff4ddc12b796f18935de0632d001585c911c9f5 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -172,6 +172,11 @@ namespace {
 const int kMinimumDelayBetweenLoadingUpdatesMS = 100;
 const char kDotGoogleDotCom[] = ".google.com";
 
+const AccessibilityMode kAccessibilityModeWebContent(

[dcheng, 2017/03/07 08:41:48]

constexpr

(I'm a little surprised that this doesn't trigger any warnings about potential
static initializers)

[dougt, 2017/03/07 16:50:46]

Yes, if it is a constexpr you do get an error/warning:

'AccessibilityMode' is not literal because it is not an aggregate and has no
constexpr constructors other than copy or move constructors

This is because constexpr implies that the value is known at complication time
-- but it's not in this case.

[dcheng, 2017/03/07 18:05:12]

If the constructor is constexpr as well, it should work.

+    AccessibilityMode::kWebContents | AccessibilityMode::kInlineTextBoxes |
+    AccessibilityMode::kScreenReader |
+    AccessibilityMode::kHTML);
+
 #if defined(OS_ANDROID)
 const char kWebContentsAndroidKey[] = "web_contents_android";
 #endif  // OS_ANDROID
@@ -968,7 +973,9 @@ void WebContentsImpl::SetAccessibilityMode(AccessibilityMode mode) {
 }
 
 void WebContentsImpl::AddAccessibilityMode(AccessibilityMode mode) {
-  SetAccessibilityMode(accessibility_mode_ | mode);
+  AccessibilityMode new_mode(accessibility_mode_);
+  new_mode |= mode;
+  SetAccessibilityMode(new_mode);
 }
 
 void WebContentsImpl::RequestAXTreeSnapshot(AXTreeSnapshotCallback callback) {
@@ -1080,20 +1087,24 @@ const std::string& WebContentsImpl::GetUserAgentOverride() const {
 }
 
 void WebContentsImpl::EnableWebContentsOnlyAccessibilityMode() {
-  if (GetAccessibilityMode() != AccessibilityModeOff) {
+  if (!GetAccessibilityMode().is_mode_off()) {
     for (RenderFrameHost* rfh : GetAllFrames())
       ResetAccessibility(rfh);
   } else {
-    AddAccessibilityMode(ACCESSIBILITY_MODE_WEB_CONTENTS_ONLY);
+    AddAccessibilityMode(kAccessibilityModeWebContent);
   }
 }
 
 bool WebContentsImpl::IsWebContentsOnlyAccessibilityModeForTesting() const {
-  return accessibility_mode_ == ACCESSIBILITY_MODE_WEB_CONTENTS_ONLY;
+  return accessibility_mode_ == kAccessibilityModeWebContent;
 }
 
 bool WebContentsImpl::IsFullAccessibilityModeForTesting() const {
-  return accessibility_mode_ == ACCESSIBILITY_MODE_COMPLETE;
+  AccessibilityMode complete_mode(
+      AccessibilityMode::kNativeAPIs | AccessibilityMode::kWebContents |
+      AccessibilityMode::kInlineTextBoxes | AccessibilityMode::kScreenReader |
+      AccessibilityMode::kHTML);

[dcheng, 2017/03/07 08:41:48]

Would be nice not to have this duplicated in
https://codereview.chromium.org/2694413006/diff/140001/content/browser/androi...
as well.

Maybe just define the complete version as a constexpr in the AccessibilityMode
header and use it everywhere?

[dougt, 2017/03/07 16:50:46]

I considered doing that, but would that mean that every compilation unit that
includes this header would have a copy of these values. Probably doesn't matter
that much either way.  I sort of like it where we're very explict at the call
site on exactly what flags we want to use.  WDYT?

[dcheng, 2017/03/07 18:05:12]

See my previous comment about constexpr. Explicit-ness is nice, but when all we
want is to make sure all the flags are included, I think it's better to have it
defined in one place (next to where we define the flags)

+  return accessibility_mode_ == complete_mode;
 }
 
 const PageImportanceSignals& WebContentsImpl::GetPageImportanceSignals() const {