Readability review for Obj-C for feldstein

Readibility review code using the cocoa browser accessibility stuff for
objective c

Review URL: http://codereview.chromium.org/2951011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@53723 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 144 insertions, 53 deletions

diff --git a/chrome/browser/cocoa/browser_accessibility.h b/chrome/browser/cocoa/browser_accessibility.h
index 3c8683a..32af50e 100644
--- a/chrome/browser/cocoa/browser_accessibility.h
+++ b/chrome/browser/cocoa/browser_accessibility.h
@@ -9,15 +9,15 @@
 #import <Cocoa/Cocoa.h>
 
 #import "base/scoped_nsobject.h"
-#include "chrome/browser/cocoa/browser_accessibility_delegate.h"
+#import "chrome/browser/cocoa/browser_accessibility_delegate.h"
 #include "webkit/glue/webaccessibility.h"
 
 using webkit_glue::WebAccessibility;
 
 // BrowserAccessibility is a cocoa wrapper around the WebAccessibility
 // object. The renderer converts webkit's accessibility tree into a
-// WebAccessibility tree and passes it to us over IPC.  This class
-// converts it into a format Cocoa can query.
+// WebAccessibility tree and passes it to the browser process over IPC.
+// This class converts it into a format Cocoa can query.
 @interface BrowserAccessibility : NSObject {
  @private
   WebAccessibility webAccessibility_;
@@ -25,17 +25,31 @@ using webkit_glue::WebAccessibility;
   scoped_nsobject<NSMutableArray> children_;
   // The parent of the accessibility object.  This can be another
   // BrowserAccessibility or a RenderWidgetHostViewCocoa.
-  id parent_;
+  id parent_;  // weak
 }
 
-- (id)initWithObject:(const WebAccessibility)accessibility
+// This creates a cocoa browser accessibility object around
+// the webkit glue WebAccessibility object.  The delegate is
+// used to communicate with the host renderer.  None of these
+// parameters can be null.
+- (id)initWithObject:(const WebAccessibility&)accessibility
             delegate:(id<BrowserAccessibilityDelegate>)delegate
               parent:(id)parent;
 
+// Children is an array of BrowserAccessibility objects, representing
+// the accessibility children of this object.
 @property(nonatomic, readonly) NSArray* children;
+// isIgnored returns whether or not the accessibility object
+// should be ignored by the accessibility hierarchy.
 @property(nonatomic, readonly, getter=isIgnored) BOOL ignored;
+// The origin of this object in the page's document.
+// This is relative to webkit's top-left origin, not Cocoa's
+// bottom-left origin.
 @property(nonatomic, readonly) NSPoint origin;
+// A string indicating the role of this object as far as accessibility
+// is concerned.
 @property(nonatomic, readonly) NSString* role;
+// The size of this object.
 @property(nonatomic, readonly) NSSize size;
 
 @end
diff --git a/chrome/browser/cocoa/browser_accessibility.mm b/chrome/browser/cocoa/browser_accessibility.mm
index 6dbf3ce..cea970a 100644
--- a/chrome/browser/cocoa/browser_accessibility.mm
+++ b/chrome/browser/cocoa/browser_accessibility.mm
@@ -4,9 +4,10 @@
 
 #include <execinfo.h>
 
+#import "chrome/browser/cocoa/browser_accessibility.h"
+
 #include "base/string16.h"
 #include "base/sys_string_conversions.h"
-#include "chrome/browser/cocoa/browser_accessibility.h"
 #include "chrome/browser/renderer_host/render_widget_host_view_mac.h"
 #include "third_party/WebKit/WebKit/chromium/public/WebRect.h"
 
@@ -16,9 +17,9 @@ namespace {
 
 // Returns an autoreleased copy of the WebAccessibility's attribute.
 NSString* NSStringForWebAccessibilityAttribute(
-    std::map<int32, string16>& attributes,
+    const std::map<int32, string16>& attributes,
     WebAccessibility::Attribute attribute) {
-  std::map<int32, string16>::iterator iter =
+  std::map<int32, string16>::const_iterator iter =
       attributes.find(attribute);
   NSString* returnValue = @"";
   if (iter != attributes.end()) {
@@ -54,15 +55,17 @@ static const RoleEntry roles[] = {
   { WebAccessibility::ROLE_WEBCORE_LINK, NSAccessibilityLinkRole},
 };
 
+// GetState checks the bitmask used in webaccessibility.h to check
+// if the given state was set on the accessibility object.
 bool GetState(WebAccessibility accessibility, int state) {
   return ((accessibility.state >> state) & 1);
 }
 
-} // anonymous namespace
+} // namespace
 
 @implementation BrowserAccessibility
 
-- (id)initWithObject:(const WebAccessibility)accessibility
+- (id)initWithObject:(const WebAccessibility&)accessibility
             delegate:(id<BrowserAccessibilityDelegate>)delegate
               parent:(id)parent {
   if ((self = [super init])) {
@@ -73,6 +76,8 @@ bool GetState(WebAccessibility accessibility, int state) {
   return self;
 }
 
+// Returns an array of BrowserAccessibility objects, representing the
+// accessibility children of this object.
 - (NSArray*)children {
   if (!children_.get()) {
     const std::vector<WebAccessibility>& accessibilityChildren =
@@ -83,28 +88,33 @@ bool GetState(WebAccessibility accessibility, int state) {
     std::vector<WebAccessibility>::const_iterator iterator;
     for (iterator = accessibilityChildren.begin();
          iterator != accessibilityChildren.end();
-         iterator++) {
-      BrowserAccessibility* child =
+         ++iterator) {
+      scoped_nsobject<BrowserAccessibility> child(
           [[BrowserAccessibility alloc]
               initWithObject:*iterator
                     delegate:delegate_
-                      parent:self];
-      [child autorelease];
+                      parent:self]);
       [children_ addObject:child];
     }
   }
   return children_;
 }
 
+// Returns whether or not this node should be ignored in the
+// accessibility tree.
 - (BOOL)isIgnored {
   return webAccessibility_.role == WebAccessibility::ROLE_IGNORED;
 }
 
+// The origin of this accessibility object in the page's document.
+// This is relative to webkit's top-left origin, not Cocoa's
+// bottom-left origin.
 - (NSPoint)origin {
   return NSMakePoint(webAccessibility_.location.x,
                      webAccessibility_.location.y);
 }
 
+// Returns a string indicating the role of this object.
 - (NSString*)role {
   NSString* role = NSAccessibilityUnknownRole;
   WebAccessibility::Role value = webAccessibility_.role;
@@ -118,46 +128,59 @@ bool GetState(WebAccessibility accessibility, int state) {
   return role;
 }
 
+// Returns the size of this object.
 - (NSSize)size {
   return NSMakeSize(webAccessibility_.location.width,
                     webAccessibility_.location.height);
 }
 
-- (id)accessibilityAttributeValue:(NSString *)attribute {
+// Returns the accessibility value for the given attribute.  If the value isn't
+// supported this will return nil.
+- (id)accessibilityAttributeValue:(NSString*)attribute {
   if ([attribute isEqualToString:NSAccessibilityRoleAttribute]) {
     return [self role];
-  } else if ([attribute isEqualToString:NSAccessibilityDescriptionAttribute]) {
+  }
+  if ([attribute isEqualToString:NSAccessibilityDescriptionAttribute]) {
     return NSStringForWebAccessibilityAttribute(webAccessibility_.attributes,
         WebAccessibility::ATTR_DESCRIPTION);
-  } else if ([attribute isEqualToString:NSAccessibilityPositionAttribute]) {
+  }
+  if ([attribute isEqualToString:NSAccessibilityPositionAttribute]) {
     return [NSValue valueWithPoint:[delegate_ accessibilityPointInScreen:self]];
-  } else if ([attribute isEqualToString:NSAccessibilitySizeAttribute]) {
+  }
+  if ([attribute isEqualToString:NSAccessibilitySizeAttribute]) {
     return [NSValue valueWithSize:[self size]];
-  } else if (
-      [attribute isEqualToString:NSAccessibilityTopLevelUIElementAttribute] ||
+  }
+  if ([attribute isEqualToString:NSAccessibilityTopLevelUIElementAttribute] ||
       [attribute isEqualToString:NSAccessibilityWindowAttribute]) {
     return [delegate_ window];
-  } else if ([attribute isEqualToString:NSAccessibilityChildrenAttribute]) {
+  }
+  if ([attribute isEqualToString:NSAccessibilityChildrenAttribute]) {
     return [self children];
-  } else if ([attribute isEqualToString:NSAccessibilityParentAttribute]) {
+  }
+  if ([attribute isEqualToString:NSAccessibilityParentAttribute]) {
     if (parent_) {
       return NSAccessibilityUnignoredAncestor(parent_);
     }
-  } else if ([attribute isEqualToString:NSAccessibilityTitleAttribute]) {
+  }
+  if ([attribute isEqualToString:NSAccessibilityTitleAttribute]) {
     return base::SysUTF16ToNSString(webAccessibility_.name);
-  } else if ([attribute isEqualToString:NSAccessibilityHelpAttribute]) {
+  }
+  if ([attribute isEqualToString:NSAccessibilityHelpAttribute]) {
     return NSStringForWebAccessibilityAttribute(webAccessibility_.attributes,
                                                 WebAccessibility::ATTR_HELP);
-  } else if ([attribute isEqualToString:NSAccessibilityValueAttribute]) {
+  }
+  if ([attribute isEqualToString:NSAccessibilityValueAttribute]) {
     return base::SysUTF16ToNSString(webAccessibility_.value);
-  } else if (
-      [attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute]) {
+  }
+  if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute]) {
     return NSAccessibilityRoleDescription([self role], nil);
-  } else if ([attribute isEqualToString:NSAccessibilityFocusedAttribute]) {
+  }
+  if ([attribute isEqualToString:NSAccessibilityFocusedAttribute]) {
     NSNumber* ret = [NSNumber numberWithBool:
         GetState(webAccessibility_, WebAccessibility::STATE_FOCUSED)];
     return ret;
-  } else if ([attribute isEqualToString:NSAccessibilityEnabledAttribute] ||
+  }
+  if ([attribute isEqualToString:NSAccessibilityEnabledAttribute] ||
       [attribute isEqualToString:@"AXVisited"] ||
       [attribute isEqualToString:@"AXLoaded"]) {
     return [NSNumber numberWithBool:YES];
@@ -165,14 +188,21 @@ bool GetState(WebAccessibility accessibility, int state) {
   return nil;
 }
 
-- (NSArray *)accessibilityActionNames {
+// Returns an array of action names that this object will respond to.
+- (NSArray*)accessibilityActionNames {
   return [NSArray arrayWithObjects:
       NSAccessibilityPressAction, NSAccessibilityShowMenuAction, nil];
 }
 
-- (NSArray *)accessibilityArrayAttributeValues:(NSString *)attribute
-                                         index:(NSUInteger)index
-                                      maxCount:(NSUInteger)maxCount {
+// Returns a sub-array of values for the given attribute value, starting at
+// index, with up to maxCount items.  If the given index is out of bounds,
+// or there are no values for the given attribute, it will return nil.
+// This method is used for querying subsets of values, without having to
+// return a large set of data, such as elements with a large number of
+// children.
+- (NSArray*)accessibilityArrayAttributeValues:(NSString*)attribute
+                                        index:(NSUInteger)index
+                                     maxCount:(NSUInteger)maxCount {
   NSArray* fullArray = [self accessibilityAttributeValue:attribute];
   if (!fullArray)
     return nil;
@@ -188,34 +218,39 @@ bool GetState(WebAccessibility accessibility, int state) {
   return [fullArray subarrayWithRange:subRange];
 }
 
-- (NSUInteger)accessibilityArrayAttributeCount:(NSString *)attribute {
+// Returns the count of the specified accessibility array attribute.
+- (NSUInteger)accessibilityArrayAttributeCount:(NSString*)attribute {
   NSArray* fullArray = [self accessibilityAttributeValue:attribute];
   return [fullArray count];
 }
 
-- (NSArray *)accessibilityAttributeNames {
+// Returns the list of accessibility attributes that this object supports.
+- (NSArray*)accessibilityAttributeNames {
   return [NSArray arrayWithObjects:
-      NSAccessibilityRoleAttribute,
-      NSAccessibilityRoleDescriptionAttribute,
       NSAccessibilityChildrenAttribute,
+      NSAccessibilityDescriptionAttribute,
+      NSAccessibilityEnabledAttribute,
+      NSAccessibilityFocusedAttribute,
       NSAccessibilityHelpAttribute,
       NSAccessibilityParentAttribute,
       NSAccessibilityPositionAttribute,
+      NSAccessibilityRoleAttribute,
+      NSAccessibilityRoleDescriptionAttribute,
       NSAccessibilitySizeAttribute,
       NSAccessibilityTitleAttribute,
-      NSAccessibilityDescriptionAttribute,
+      NSAccessibilityTopLevelUIElementAttribute,
       NSAccessibilityValueAttribute,
-      NSAccessibilityFocusedAttribute,
-      NSAccessibilityEnabledAttribute,
       NSAccessibilityWindowAttribute,
-      NSAccessibilityTopLevelUIElementAttribute,
       nil];
 }
 
+// Returns the deepest child that has user focus.
+// TODO(feldstein): This does not work yet.
 - (id)accessibilityFocusedUIElement {
   return self;
 }
 
+// Returns the index of the child in this objects array of children.
 - (NSUInteger)accessibilityIndexOfChild:(id)child {
   NSUInteger index = 0;
   for (BrowserAccessibility* childToCheck in [self children]) {
@@ -227,34 +262,43 @@ bool GetState(WebAccessibility accessibility, int state) {
   return NSNotFound;
 }
 
-- (BOOL)accessibilityIsAttributeSettable:(NSString *)attribute {
-  if ([attribute isEqualToString:NSAccessibilityFocusedAttribute]) {
+// Returns whether or not the specified attribute can be set by the
+// accessibility API via |accessibilitySetValue:forAttribute:|.
+- (BOOL)accessibilityIsAttributeSettable:(NSString*)attribute {
+  if ([attribute isEqualToString:NSAccessibilityFocusedAttribute])
     return GetState(webAccessibility_, WebAccessibility::STATE_FOCUSABLE);
-  } else if ([attribute isEqualToString:NSAccessibilityValueAttribute]) {
+  if ([attribute isEqualToString:NSAccessibilityValueAttribute])
     return !GetState(webAccessibility_, WebAccessibility::STATE_READONLY);
-  }
   return NO;
 }
 
+// Returns whether or not this object should be ignored in the accessibilty
+// tree.
 - (BOOL)accessibilityIsIgnored {
   return [self isIgnored];
 }
 
-- (void)accessibilityPerformAction:(NSString *)action {
+// Performs the given accessibilty action on the webkit accessibility object
+// that backs this object.
+- (void)accessibilityPerformAction:(NSString*)action {
   // TODO(feldstein): Support more actions.
   [delegate_ doDefaultAction:webAccessibility_.id];
 }
 
+// Returns the description of the given action.
 - (NSString*)accessibilityActionDescription:(NSString*)action {
   return NSAccessibilityActionDescription(action);
 }
 
+// Sets an override value for a specific accessibility attribute.
+// This class does not support this.
 - (BOOL)accessibilitySetOverrideValue:(id)value
-                         forAttribute:(NSString *)attribute {
+                         forAttribute:(NSString*)attribute {
   return NO;
 }
 
-- (void)accessibilitySetValue:(id)value forAttribute:(NSString *)attribute {
+// Sets the value for an accessibility attribute via the accessibility API.
+- (void)accessibilitySetValue:(id)value forAttribute:(NSString*)attribute {
   if ([attribute isEqualToString:NSAccessibilityFocusedAttribute]) {
     NSNumber* focusedNumber = value;
     BOOL focused = [focusedNumber intValue];
@@ -263,6 +307,9 @@ bool GetState(WebAccessibility accessibility, int state) {
   }
 }
 
+// Returns the deepest accessibility child that should not be ignored.
+// It is assumed that the hit test has been narrowed down to this object
+// or one of its children, so this will never return nil.
 - (id)accessibilityHitTest:(NSPoint)point {
   id hit = self;
   for (id child in [self children]) {
diff --git a/chrome/browser/cocoa/browser_accessibility_delegate.h b/chrome/browser/cocoa/browser_accessibility_delegate.h
index 6cd5563..800174d91 100644
--- a/chrome/browser/cocoa/browser_accessibility_delegate.h
+++ b/chrome/browser/cocoa/browser_accessibility_delegate.h
@@ -9,6 +9,9 @@
 @class BrowserAccessibility;
 @class NSWindow;
 
+// This protocol is used by the BrowserAccessibility objects to pass messages
+// to, or otherwise communicate with, their underlying WebAccessibility
+// objects over the IPC boundary.
 @protocol BrowserAccessibilityDelegate
 - (NSPoint)accessibilityPointInScreen:(BrowserAccessibility*)accessibility;
 - (void)doDefaultAction:(int32)accessibilityObjectId;
diff --git a/chrome/browser/cocoa/browser_accessibility_unittest.mm b/chrome/browser/cocoa/browser_accessibility_unittest.mm
index 2fa93e0..a44942e 100644
--- a/chrome/browser/cocoa/browser_accessibility_unittest.mm
+++ b/chrome/browser/cocoa/browser_accessibility_unittest.mm
@@ -53,28 +53,29 @@ class BrowserAccessibilityTest : public CocoaTest {
     child1.location.y = 0;
     child1.location.width = 250;
     child1.location.height = 100;
-    
+
     WebAccessibility child2;
     child2.location.x = 250;
     child2.location.y = 0;
     child2.location.width = 250;
     child2.location.height = 100;
-    
+
     root.children.push_back(child1);
-    root.children.push_back(child2);    
-    
+    root.children.push_back(child2);
+
     delegate_.reset([[MockAccessibilityDelegate alloc] init]);
     accessibility_.reset(
         [[BrowserAccessibility alloc] initWithObject:root
                                             delegate:delegate_
                                               parent:delegate_]);
   }
- 
+
  protected:
   scoped_nsobject<MockAccessibilityDelegate> delegate_;
   scoped_nsobject<BrowserAccessibility> accessibility_;
 };
 
+// Standard hit test.
 TEST_F(BrowserAccessibilityTest, HitTestTest) {
   BrowserAccessibility* firstChild =
       [accessibility_ accessibilityHitTest:NSMakePoint(50, 50)];
@@ -83,8 +84,34 @@ TEST_F(BrowserAccessibilityTest, HitTestTest) {
         isEqualToString:@"Child1"]);
 }
 
+// Test doing a hit test on the edge of a child.
+TEST_F(BrowserAccessibilityTest, EdgeHitTest) {
+  BrowserAccessibility* firstChild =
+      [accessibility_ accessibilityHitTest:NSMakePoint(0, 0)];
+  EXPECT_TRUE(
+      [[firstChild accessibilityAttributeValue:NSAccessibilityTitleAttribute]
+        isEqualToString:@"Child1"]);
+}
+
+// This will test a hit test with invalid coordinates.  It is assumed that
+// the hit test has been narrowed down to this object or one of its children
+// so it should return itself since it has no better hit result.
+TEST_F(BrowserAccessibilityTest, InvalidHitTestCoordsTest) {
+  BrowserAccessibility* hitTestResult =
+      [accessibility_ accessibilityHitTest:NSMakePoint(-50, 50)];
+  EXPECT_TRUE([accessibility_ isEqualTo:hitTestResult]);
+}
+
+// Test to ensure querying standard attributes works.
 TEST_F(BrowserAccessibilityTest, BasicAttributeTest) {
   NSString* helpText = [accessibility_
       accessibilityAttributeValue:NSAccessibilityHelpAttribute];
   EXPECT_TRUE([helpText isEqualToString: @"HelpText"]);
 }
+
+// Test querying for an invalid attribute to ensure it doesn't crash.
+TEST_F(BrowserAccessibilityTest, InvalidAttributeTest) {
+  NSString* shouldBeNil = [accessibility_
+      accessibilityAttributeValue:@"NSAnInvalidAttribute"];
+  EXPECT_TRUE(shouldBeNil == nil);
+}