Clean up AutomationPredicate

chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js

Index: chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js
diff --git a/chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js b/chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js
index c7bc3d2f772897c702de244ce530495c7a045836..563e2ff80e12af9cd148f7b11d76f8baa35c41fa 100644
--- a/chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js
+++ b/chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js
@@ -15,7 +15,7 @@ goog.require('constants');
 goog.scope(function() {
 var AutomationNode = chrome.automation.AutomationNode;
 var Dir = constants.Dir;
-var RoleType = chrome.automation.RoleType;
+var Role = chrome.automation.RoleType;
 
 /**
  * @constructor
@@ -33,31 +33,38 @@ AutomationPredicate.Unary;
 AutomationPredicate.Binary;
 
 /**
- * Constructs a predicate given a role.
- * @param {RoleType} role
+ * Constructs a predicate given a list of roles or predicates.
+ * @param {!Array<AutomationPredicate.Unary|Role>} roles
  * @return {AutomationPredicate.Unary}
+ * @private
  */
-AutomationPredicate.withRole = function(role) {
+AutomationPredicate.roles_ = function(roles) {

[dmazzoni, 2016/09/23 17:52:57]

I think it's a little surprising to have a function
called roles_ that can take predicates too. I don't want
to hurt the nice brevity that this achieves, but it might
be nice to do it in a way that's a bit more explicit.

How about this idea:

AutomationPredicate.roles_ matches one role or an array of roles (that doesn't
seem unclear to me)

AutomationPredicate.anyPredicate_ matches if any of the predicates match

Then you could write formField like this:

AutomationPredicate.formField = AutomationPredicate.anyPredicate_(
    AutomationPredicate.button,
    AutomationPredicate.comboBox,
    AutomationPredicate.editText,
    AutomationPredicate.roles_(
        [Role.checkBox,
         Role.listBox,
         Role.slider,
         Role.tab,
         Role.tree]));

I think that'd be just as concise but a little easier to read.

+  if (!roles.length)
+    throw 'Expected at least one predicate.';
   return function(node) {
-    return node.role == role;
+    return roles.some(function(role) {
+      return role == node.role ||
+          (typeof role == 'function' && role(node));
+    });
   };
 };
 
 /** @type {AutomationPredicate.Unary} */
-AutomationPredicate.checkBox = AutomationPredicate.withRole(RoleType.checkBox);
+AutomationPredicate.checkBox = AutomationPredicate.roles_([Role.checkBox]);
 /** @type {AutomationPredicate.Unary} */
-AutomationPredicate.comboBox = AutomationPredicate.withRole(RoleType.comboBox);
+AutomationPredicate.comboBox = AutomationPredicate.roles_(
+    [Role.comboBox, Role.popUpButton, Role.menuListPopup]);
 /** @type {AutomationPredicate.Unary} */
-AutomationPredicate.heading = AutomationPredicate.withRole(RoleType.heading);
+AutomationPredicate.heading = AutomationPredicate.roles_([Role.heading]);
 /** @type {AutomationPredicate.Unary} */
 AutomationPredicate.inlineTextBox =
-    AutomationPredicate.withRole(RoleType.inlineTextBox);
+    AutomationPredicate.roles_([Role.inlineTextBox]);
 /** @type {AutomationPredicate.Unary} */
-AutomationPredicate.link = AutomationPredicate.withRole(RoleType.link);
+AutomationPredicate.link = AutomationPredicate.roles_([Role.link]);
 /** @type {AutomationPredicate.Unary} */
-AutomationPredicate.row = AutomationPredicate.withRole(RoleType.row);
+AutomationPredicate.row = AutomationPredicate.roles_([Role.row]);
 /** @type {AutomationPredicate.Unary} */
-AutomationPredicate.table = AutomationPredicate.withRole(RoleType.table);
+AutomationPredicate.table = AutomationPredicate.roles_([Role.table]);
 
 /**
  * @param {!AutomationNode} node
@@ -67,7 +74,6 @@ AutomationPredicate.button = function(node) {
   return /button/i.test(node.role);
 };
 
-
 /**
  * @param {!AutomationNode} node
  * @return {boolean}
@@ -78,57 +84,29 @@ AutomationPredicate.editText = function(node) {
       !node.parent.state.editable;
 };
 
-/**
- * @param {!AutomationNode} node
- * @return {boolean}
- */
-AutomationPredicate.formField = function(node) {
-  switch (node.role) {
-    case 'button':
-    case 'buttonDropDown':
-    case 'checkBox':
-    case 'comboBox':
-    case 'date':
-    case 'dateTime':
-    case 'details':
-    case 'disclosureTriangle':
-    case 'form':
-    case 'menuButton':
-    case 'menuListPopup':
-    case 'popUpButton':
-    case 'radioButton':
-    case 'searchBox':
-    case 'slider':
-    case 'spinButton':
-    case 'switch':
-    case 'tab':
-    case 'textField':
-    case 'time':
-    case 'toggleButton':
-    case 'tree':
-      return true;
-  }
-  return false;
-};
+/** @type {AutomationPredicate.Unary} */
+AutomationPredicate.formField = AutomationPredicate.roles_([
+    AutomationPredicate.button,
+    Role.checkBox,
+    AutomationPredicate.comboBox,
+    AutomationPredicate.editText,
+    Role.listBox,
+    Role.slider,
+    Role.tab,
+    Role.tree]);
 
-/**
- * @param {!AutomationNode} node
- * @return {boolean}
- */
-AutomationPredicate.landmark = function(node) {
-  switch (node.role) {
-    case 'application':
-    case 'banner':
-    case 'complementary':
-    case 'contentInfo':
-    case 'form':
-    case 'main':
-    case 'navigation':
-    case 'search':
-      return true;
-  }
-  return false;
-};
+/** @type {AutomationPredicate.Unary} */
+AutomationPredicate.landmark = AutomationPredicate.roles_([
+    Role.application,
+    Role.banner,
+    Role.complementary,
+    Role.contentInfo,
+    Role.form,
+    Role.group,
+    Role.main,
+    Role.navigation,
+    Role.region,
+    Role.search]);
 
 /**
  * @param {!AutomationNode} node
@@ -152,11 +130,11 @@ AutomationPredicate.focused = function(node) {
  */
 AutomationPredicate.leaf = function(node) {
   return !node.firstChild ||
-      node.role == RoleType.button ||
-      node.role == RoleType.buttonDropDown ||
-      node.role == RoleType.popUpButton ||
-      node.role == RoleType.slider ||
-      node.role == RoleType.textField ||
+      node.role == Role.button ||
+      node.role == Role.buttonDropDown ||
+      node.role == Role.popUpButton ||
+      node.role == Role.slider ||
+      node.role == Role.textField ||
       node.state.invisible ||
       node.children.every(function(n) {
         return n.state.invisible;
@@ -181,7 +159,7 @@ AutomationPredicate.leafWithText = function(node) {
  */
 AutomationPredicate.leafOrStaticText = function(node) {
   return AutomationPredicate.leaf(node) ||
-      node.role == RoleType.staticText;
+      node.role == Role.staticText;
 };
 
 /**
@@ -202,9 +180,9 @@ AutomationPredicate.object = function(node) {
   return node.state.focusable ||
       (AutomationPredicate.leafOrStaticText(node) &&
        (/\S+/.test(node.name) ||
-        (node.role != RoleType.lineBreak &&
-         node.role != RoleType.staticText &&
-         node.role != RoleType.inlineTextBox)));
+        (node.role != Role.lineBreak &&
+         node.role != Role.staticText &&
+         node.role != Role.inlineTextBox)));
 };
 
 /**
@@ -228,14 +206,14 @@ AutomationPredicate.linebreak = function(first, second) {
  */
 AutomationPredicate.container = function(node) {
   return AutomationPredicate.structuralContainer(node) ||
-      node.role == RoleType.div ||
-      node.role == RoleType.document ||
-      node.role == RoleType.group ||
-      node.role == RoleType.listItem ||
-      node.role == RoleType.toolbar ||
-      node.role == RoleType.window ||
+      node.role == Role.div ||
+      node.role == Role.document ||
+      node.role == Role.group ||
+      node.role == Role.listItem ||
+      node.role == Role.toolbar ||
+      node.role == Role.window ||
       // For example, crosh.
-      (node.role == RoleType.textField && node.state.readOnly) ||
+      (node.role == Role.textField && node.state.readOnly) ||
       (node.state.editable && node.parent && !node.parent.state.editable);
 };
 
@@ -246,10 +224,10 @@ AutomationPredicate.container = function(node) {
  * @return {boolean}
  */
 AutomationPredicate.structuralContainer = function(node) {
-  return node.role == RoleType.rootWebArea ||
-      node.role == RoleType.embeddedObject ||
-      node.role == RoleType.iframe ||
-      node.role == RoleType.iframePresentational;
+  return node.role == Role.rootWebArea ||
+      node.role == Role.embeddedObject ||
+      node.role == Role.iframe ||
+      node.role == Role.iframePresentational;
 };
 
 /**
@@ -260,13 +238,13 @@ AutomationPredicate.structuralContainer = function(node) {
  */
 AutomationPredicate.root = function(node) {
   switch (node.role) {
-    case RoleType.dialog:
-    case RoleType.window:
+    case Role.dialog:
+    case Role.window:
       return true;
-    case RoleType.toolbar:
-      return node.root.role == RoleType.desktop;
-    case RoleType.rootWebArea:
-      return !node.parent || node.parent.root.role == RoleType.desktop;
+    case Role.toolbar:
+      return node.root.role == Role.desktop;
+    case Role.rootWebArea:
+      return !node.parent || node.parent.root.role == Role.desktop;
     default:
       return false;
   }
@@ -289,7 +267,7 @@ AutomationPredicate.shouldIgnoreNode = function(node) {
     return true;
 
   // Ignore list markers since we already announce listitem role.
-  if (node.role == RoleType.listMarker)
+  if (node.role == Role.listMarker)
     return true;
 
   // Don't ignore nodes with names.
@@ -298,13 +276,13 @@ AutomationPredicate.shouldIgnoreNode = function(node) {
 
   // Ignore some roles.
   return AutomationPredicate.leaf(node) &&
-      (node.role == RoleType.client ||
-       node.role == RoleType.column ||
-       node.role == RoleType.div ||
-       node.role == RoleType.group ||
-       node.role == RoleType.image ||
-       node.role == RoleType.staticText ||
-       node.role == RoleType.tableHeaderContainer);
+      (node.role == Role.client ||

[dmazzoni, 2016/09/23 17:52:57]

This looks like a great opportunity to make it more concise
by using AutomationPredicate.roles_().

+       node.role == Role.column ||
+       node.role == Role.div ||
+       node.role == Role.group ||
+       node.role == Role.image ||
+       node.role == Role.staticText ||
+       node.role == Role.tableHeaderContainer);
 };
 
 
@@ -314,11 +292,11 @@ AutomationPredicate.shouldIgnoreNode = function(node) {
  * @return {boolean}
  */
 AutomationPredicate.checkable = function(node) {
-  return node.role == RoleType.checkBox ||
-      node.role == RoleType.radioButton ||
-      node.role == RoleType.menuItemCheckBox ||
-      node.role == RoleType.menuItemRadio ||
-      node.role == RoleType.treeItem;
+  return node.role == Role.checkBox ||
+      node.role == Role.radioButton ||
+      node.role == Role.menuItemCheckBox ||
+      node.role == Role.menuItemRadio ||
+      node.role == Role.treeItem;
 };
 
 // Table related predicates.
@@ -328,9 +306,9 @@ AutomationPredicate.checkable = function(node) {
  * @return {boolean}
  */
 AutomationPredicate.cellLike = function(node) {
-  return node.role == RoleType.cell ||
-      node.role == RoleType.rowHeader ||
-      node.role == RoleType.columnHeader;
+  return node.role == Role.cell ||
+      node.role == Role.rowHeader ||
+      node.role == Role.columnHeader;
 };
 
 /**
@@ -411,7 +389,7 @@ AutomationPredicate.makeTableCellPredicate = function(start, opts) {
  */
 AutomationPredicate.makeHeadingPredicate = function(level) {
   return function(node) {
-    return node.role == RoleType.heading && node.hierarchicalLevel == level;
+    return node.role == Role.heading && node.hierarchicalLevel == level;
   };
 };