Readability review for Obj-C for feldstein

Readability review for Obj-C for feldstein

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

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53723

[feldstein, 2010-07-13 21:22:55]

Hey Pink, Can you help me out with this readability review?

[pink (ping after 24hrs), 2010-07-16 18:33:21]

A good start. Needs some style cleanup, but mostly needs a lot of function-level
commenting.

http://codereview.chromium.org/2951011/diff/1/2
File chrome/browser/cocoa/browser_accessibility.h (right):

http://codereview.chromium.org/2951011/diff/1/2#newcode11
chrome/browser/cocoa/browser_accessibility.h:11: #include
"chrome/browser/cocoa/browser_accessibility_delegate.h"
#import?

http://codereview.chromium.org/2951011/diff/1/2#newcode18
chrome/browser/cocoa/browser_accessibility.h:18: // WebAccessibility tree and
passes it to us over IPC.  This class
we tend to discourage sentenced written in second person. Ask Mark would put it,
"who exactly is 'we'?" Comments not written in this voice tend to sound better
and make more sense overall.

http://codereview.chromium.org/2951011/diff/1/2#newcode27
chrome/browser/cocoa/browser_accessibility.h:27: id parent_;
if this is weak ownership (as it appears), it needs a comment.

http://codereview.chromium.org/2951011/diff/1/2#newcode30
chrome/browser/cocoa/browser_accessibility.h:30: - (id)initWithObject:(const
WebAccessibility)accessibility
should this be a const reference to avoid a copy when passing the param? Or is
WebAccessibility an alias type for a pointer type?

http://codereview.chromium.org/2951011/diff/1/3
File chrome/browser/cocoa/browser_accessibility.mm (right):

http://codereview.chromium.org/2951011/diff/1/3#newcode19
chrome/browser/cocoa/browser_accessibility.mm:19: std::map<int32, string16>&
attributes,
according to c++ style, all reference params must be const.

http://codereview.chromium.org/2951011/diff/1/3#newcode21
chrome/browser/cocoa/browser_accessibility.mm:21: std::map<int32,
string16>::iterator iter =
can you use ...::const_iterator? Does it need to be non-const for this usage?

http://codereview.chromium.org/2951011/diff/1/3#newcode57
chrome/browser/cocoa/browser_accessibility.mm:57: bool GetState(WebAccessibility
accessibility, int state) {
Comment describing the purpose of the bit math?

http://codereview.chromium.org/2951011/diff/1/3#newcode86
chrome/browser/cocoa/browser_accessibility.mm:86: iterator++) {
C++ style guide says always use pre-increment.

http://codereview.chromium.org/2951011/diff/1/3#newcode87
chrome/browser/cocoa/browser_accessibility.mm:87: BrowserAccessibility* child =
why not use a scoped_nsobject here instead of autoreleasing?

http://codereview.chromium.org/2951011/diff/1/3#newcode126
chrome/browser/cocoa/browser_accessibility.mm:126: -
(id)accessibilityAttributeValue:(NSString *)attribute {
Per chrome guidelines (and consistency), NSString*, no space.

http://codereview.chromium.org/2951011/diff/1/3#newcode126
chrome/browser/cocoa/browser_accessibility.mm:126: -
(id)accessibilityAttributeValue:(NSString *)attribute {
most of the remaining methods in the file need comments.

http://codereview.chromium.org/2951011/diff/1/3#newcode129
chrome/browser/cocoa/browser_accessibility.mm:129: } else if ([attribute
isEqualToString:NSAccessibilityDescriptionAttribute]) {
no else after return, all the way down.

http://codereview.chromium.org/2951011/diff/1/3#newcode168
chrome/browser/cocoa/browser_accessibility.mm:168: - (NSArray
*)accessibilityActionNames {
NSArray*

http://codereview.chromium.org/2951011/diff/1/3#newcode173
chrome/browser/cocoa/browser_accessibility.mm:173: - (NSArray
*)accessibilityArrayAttributeValues:(NSString *)attribute
NSArray*, NSString*

http://codereview.chromium.org/2951011/diff/1/3#newcode173
chrome/browser/cocoa/browser_accessibility.mm:173: - (NSArray
*)accessibilityArrayAttributeValues:(NSString *)attribute
needs a function-level comment. What does this do? What's it for? It can return
nil, in what cases?

http://codereview.chromium.org/2951011/diff/1/3#newcode197
chrome/browser/cocoa/browser_accessibility.mm:197: return [NSArray
arrayWithObjects:
is the ordering important?

http://codereview.chromium.org/2951011/diff/1/3#newcode233
chrome/browser/cocoa/browser_accessibility.mm:233: } else if ([attribute
isEqualToString:NSAccessibilityValueAttribute]) {
no else after return

http://codereview.chromium.org/2951011/diff/1/4
File chrome/browser/cocoa/browser_accessibility_delegate.h (right):

http://codereview.chromium.org/2951011/diff/1/4#newcode11
chrome/browser/cocoa/browser_accessibility_delegate.h:11: @protocol
BrowserAccessibilityDelegate
comment about how this protocol should be used

[feldstein, 2010-07-16 23:12:54]

Round 1, fixed your comments with a few questions.  Thanks for taking the time
for this.

http://codereview.chromium.org/2951011/diff/1/2
File chrome/browser/cocoa/browser_accessibility.h (right):

http://codereview.chromium.org/2951011/diff/1/2#newcode11
chrome/browser/cocoa/browser_accessibility.h:11: #include
"chrome/browser/cocoa/browser_accessibility_delegate.h"
On 2010/07/16 18:33:21, pink wrote:
> #import?

Done. Is there ever a reason to use include?

http://codereview.chromium.org/2951011/diff/1/2#newcode18
chrome/browser/cocoa/browser_accessibility.h:18: // WebAccessibility tree and
passes it to us over IPC.  This class
On 2010/07/16 18:33:21, pink wrote:
> we tend to discourage sentenced written in second person. Ask Mark would put
it,
> "who exactly is 'we'?" Comments not written in this voice tend to sound better
> and make more sense overall.

Done.

http://codereview.chromium.org/2951011/diff/1/2#newcode27
chrome/browser/cocoa/browser_accessibility.h:27: id parent_;
On 2010/07/16 18:33:21, pink wrote:
> if this is weak ownership (as it appears), it needs a comment.

Done.

http://codereview.chromium.org/2951011/diff/1/2#newcode30
chrome/browser/cocoa/browser_accessibility.h:30: - (id)initWithObject:(const
WebAccessibility)accessibility
On 2010/07/16 18:33:21, pink wrote:
> should this be a const reference to avoid a copy when passing the param? Or is
> WebAccessibility an alias type for a pointer type?

Done.

http://codereview.chromium.org/2951011/diff/1/3
File chrome/browser/cocoa/browser_accessibility.mm (right):

http://codereview.chromium.org/2951011/diff/1/3#newcode19
chrome/browser/cocoa/browser_accessibility.mm:19: std::map<int32, string16>&
attributes,
On 2010/07/16 18:33:21, pink wrote:
> according to c++ style, all reference params must be const.

Done.

http://codereview.chromium.org/2951011/diff/1/3#newcode21
chrome/browser/cocoa/browser_accessibility.mm:21: std::map<int32,
string16>::iterator iter =
On 2010/07/16 18:33:21, pink wrote:
> can you use ...::const_iterator? Does it need to be non-const for this usage?

Done.

http://codereview.chromium.org/2951011/diff/1/3#newcode57
chrome/browser/cocoa/browser_accessibility.mm:57: bool GetState(WebAccessibility
accessibility, int state) {
On 2010/07/16 18:33:21, pink wrote:
> Comment describing the purpose of the bit math?

Done, but not sure how much explanation i can give here, it's pretty standard
bitmasking.

http://codereview.chromium.org/2951011/diff/1/3#newcode86
chrome/browser/cocoa/browser_accessibility.mm:86: iterator++) {
On 2010/07/16 18:33:21, pink wrote:
> C++ style guide says always use pre-increment.

Done.

http://codereview.chromium.org/2951011/diff/1/3#newcode87
chrome/browser/cocoa/browser_accessibility.mm:87: BrowserAccessibility* child =
On 2010/07/16 18:33:21, pink wrote:
> why not use a scoped_nsobject here instead of autoreleasing?

Done.

http://codereview.chromium.org/2951011/diff/1/3#newcode126
chrome/browser/cocoa/browser_accessibility.mm:126: -
(id)accessibilityAttributeValue:(NSString *)attribute {
On 2010/07/16 18:33:21, pink wrote:
> Per chrome guidelines (and consistency), NSString*, no space.

Done.

http://codereview.chromium.org/2951011/diff/1/3#newcode126
chrome/browser/cocoa/browser_accessibility.mm:126: -
(id)accessibilityAttributeValue:(NSString *)attribute {
On 2010/07/16 18:33:21, pink wrote:
> most of the remaining methods in the file need comments.

Done.

http://codereview.chromium.org/2951011/diff/1/3#newcode126
chrome/browser/cocoa/browser_accessibility.mm:126: -
(id)accessibilityAttributeValue:(NSString *)attribute {
On 2010/07/16 18:33:21, pink wrote:
> most of the remaining methods in the file need comments.

Done.

http://codereview.chromium.org/2951011/diff/1/3#newcode129
chrome/browser/cocoa/browser_accessibility.mm:129: } else if ([attribute
isEqualToString:NSAccessibilityDescriptionAttribute]) {
On 2010/07/16 18:33:21, pink wrote:
> no else after return, all the way down.

Done.

http://codereview.chromium.org/2951011/diff/1/3#newcode168
chrome/browser/cocoa/browser_accessibility.mm:168: - (NSArray
*)accessibilityActionNames {
On 2010/07/16 18:33:21, pink wrote:
> NSArray*

Done.

http://codereview.chromium.org/2951011/diff/1/3#newcode173
chrome/browser/cocoa/browser_accessibility.mm:173: - (NSArray
*)accessibilityArrayAttributeValues:(NSString *)attribute
On 2010/07/16 18:33:21, pink wrote:
> NSArray*, NSString*

Done.

http://codereview.chromium.org/2951011/diff/1/3#newcode173
chrome/browser/cocoa/browser_accessibility.mm:173: - (NSArray
*)accessibilityArrayAttributeValues:(NSString *)attribute
On 2010/07/16 18:33:21, pink wrote:
> needs a function-level comment. What does this do? What's it for? It can
return
> nil, in what cases?

Done.

http://codereview.chromium.org/2951011/diff/1/3#newcode197
chrome/browser/cocoa/browser_accessibility.mm:197: return [NSArray
arrayWithObjects:
On 2010/07/16 18:33:21, pink wrote:
> is the ordering important?

Not that I'm aware of.  Let me reorder those.

http://codereview.chromium.org/2951011/diff/1/3#newcode233
chrome/browser/cocoa/browser_accessibility.mm:233: } else if ([attribute
isEqualToString:NSAccessibilityValueAttribute]) {
On 2010/07/16 18:33:21, pink wrote:
> no else after return

Done.

http://codereview.chromium.org/2951011/diff/1/4
File chrome/browser/cocoa/browser_accessibility_delegate.h (right):

http://codereview.chromium.org/2951011/diff/1/4#newcode11
chrome/browser/cocoa/browser_accessibility_delegate.h:11: @protocol
BrowserAccessibilityDelegate
On 2010/07/16 18:33:21, pink wrote:
> comment about how this protocol should be used

Done.

[pink (ping after 24hrs), 2010-07-20 20:28:09]

Almost there!

http://codereview.chromium.org/2951011/diff/1/2
File chrome/browser/cocoa/browser_accessibility.h (right):

http://codereview.chromium.org/2951011/diff/1/2#newcode11
chrome/browser/cocoa/browser_accessibility.h:11: #include
"chrome/browser/cocoa/browser_accessibility_delegate.h"
On 2010/07/16 23:12:54, feldstein wrote:
> On 2010/07/16 18:33:21, pink wrote:
> > #import?
> 
> Done. Is there ever a reason to use include?

Yes, #include is for C/C++, #import is for obj-c headers. See the style guide.

http://codereview.chromium.org/2951011/diff/9001/10001#newcode12
chrome/browser/cocoa/browser_accessibility.h:12: #include
"webkit/glue/webaccessibility.h"
this one should be #include, per explanation in style guide.

http://codereview.chromium.org/2951011/diff/9001/10001#newcode27
chrome/browser/cocoa/browser_accessibility.h:27: id parent_;
2-space indent following semi-colon on same-line comment

http://codereview.chromium.org/2951011/diff/9001/10001#newcode30
chrome/browser/cocoa/browser_accessibility.h:30: - (id)initWithObject:(const
WebAccessibility)accessibility
can any of these be null? I assume delegate can, what about parent? What other
assumptions are made the caller might need to know about?

http://codereview.chromium.org/2951011/diff/9001/10001#newcode34
chrome/browser/cocoa/browser_accessibility.h:34: @property(nonatomic, readonly)
NSArray* children;
the comment in the implementation file describes what kind of objects are in the
array, but you shouldn't need to look at the implementation to know how to use
the public api.

http://codereview.chromium.org/2951011/diff/9001/10002
File chrome/browser/cocoa/browser_accessibility.mm (right):

http://codereview.chromium.org/2951011/diff/9001/10002#newcode136
chrome/browser/cocoa/browser_accessibility.mm:136: // Returns the accessibility
value for the given attribute.
Is it ok for the attribute to not be in the list? Is it an error? How is it
handled?

http://codereview.chromium.org/2951011/diff/9001/10004
File chrome/browser/cocoa/browser_accessibility_unittest.mm (right):

http://codereview.chromium.org/2951011/diff/9001/10004#newcode79
chrome/browser/cocoa/browser_accessibility_unittest.mm:79: BrowserAccessibility*
firstChild =
is it worth testing (0, 0), negative points, point outside all known children,
overlapping children, etc?

http://codereview.chromium.org/2951011/diff/9001/10004#newcode89
chrome/browser/cocoa/browser_accessibility_unittest.mm:89: EXPECT_TRUE([helpText
isEqualToString: @"HelpText"]);
How about testing if you give it an attribute it doesn't know how to handle? At
least to ensure it doesn't crash.

[feldstein, 2010-07-21 01:05:33]

another round

http://codereview.chromium.org/2951011/diff/9001/10001
File chrome/browser/cocoa/browser_accessibility.h (right):

http://codereview.chromium.org/2951011/diff/9001/10001#newcode12
chrome/browser/cocoa/browser_accessibility.h:12: #import
"webkit/glue/webaccessibility.h"
On 2010/07/20 20:28:10, pink wrote:
> this one should be #include, per explanation in style guide.

Done.

http://codereview.chromium.org/2951011/diff/9001/10001#newcode27
chrome/browser/cocoa/browser_accessibility.h:27: id parent_; // weak
On 2010/07/20 20:28:10, pink wrote:
> 2-space indent following semi-colon on same-line comment

Done.

http://codereview.chromium.org/2951011/diff/9001/10001#newcode30
chrome/browser/cocoa/browser_accessibility.h:30: - (id)initWithObject:(const
WebAccessibility&)accessibility
On 2010/07/20 20:28:10, pink wrote:
> can any of these be null? I assume delegate can, what about parent? What other
> assumptions are made the caller might need to know about?

None of these can be nil.  If you're missing the delegate then hittesting and
other location based apis will break, along with all the action commands, and if
parent is null, the accessibility api not include it in the hierarchy.

http://codereview.chromium.org/2951011/diff/9001/10001#newcode34
chrome/browser/cocoa/browser_accessibility.h:34: @property(nonatomic, readonly)
NSArray* children;
On 2010/07/20 20:28:10, pink wrote:
> the comment in the implementation file describes what kind of objects are in
the
> array, but you shouldn't need to look at the implementation to know how to use
> the public api.

Done.

http://codereview.chromium.org/2951011/diff/9001/10002
File chrome/browser/cocoa/browser_accessibility.mm (right):

http://codereview.chromium.org/2951011/diff/9001/10002#newcode136
chrome/browser/cocoa/browser_accessibility.mm:136: // Returns the accessibility
value for the given attribute.
On 2010/07/20 20:28:10, pink wrote:
> Is it ok for the attribute to not be in the list? Is it an error? How is it
> handled?

Done.

http://codereview.chromium.org/2951011/diff/9001/10004
File chrome/browser/cocoa/browser_accessibility_unittest.mm (right):

http://codereview.chromium.org/2951011/diff/9001/10004#newcode79
chrome/browser/cocoa/browser_accessibility_unittest.mm:79: BrowserAccessibility*
firstChild =
On 2010/07/20 20:28:10, pink wrote:
> is it worth testing (0, 0), negative points, point outside all known children,
> overlapping children, etc?

I don't think it really handles overlapping children yet, there's no z-index
concept in webaccessibility.

http://codereview.chromium.org/2951011/diff/9001/10004#newcode89
chrome/browser/cocoa/browser_accessibility_unittest.mm:89: EXPECT_TRUE([helpText
isEqualToString: @"HelpText"]);
On 2010/07/20 20:28:10, pink wrote:
> How about testing if you give it an attribute it doesn't know how to handle?
At
> least to ensure it doesn't crash.

Done.

[pink (ping after 24hrs), 2010-07-21 12:50:41]

Only very minor nits left.

http://codereview.chromium.org/2951011/diff/6002/23002
File chrome/browser/cocoa/browser_accessibility.h (right):

http://codereview.chromium.org/2951011/diff/6002/23002#newcode41
chrome/browser/cocoa/browser_accessibility.h:41: // isIgnored returns whetehr or
not the accessibility object
typo: whether.

http://codereview.chromium.org/2951011/diff/6002/23002#newcode42
chrome/browser/cocoa/browser_accessibility.h:42: // should be ignored by the
accessibility heirarchy.
hierarchy

http://codereview.chromium.org/2951011/diff/6002/23002#newcode44
chrome/browser/cocoa/browser_accessibility.h:44: // The origin of this object in
the pages document.
page's

http://codereview.chromium.org/2951011/diff/6002/23002#newcode45
chrome/browser/cocoa/browser_accessibility.h:45: // This is in terms of webkit's
top-left origin, not Cocoa's
s/in terms of/relative to ?

[feldstein, 2010-07-21 18:33:11]

http://codereview.chromium.org/2951011/diff/6002/23002
File chrome/browser/cocoa/browser_accessibility.h (right):

http://codereview.chromium.org/2951011/diff/6002/23002#newcode41
chrome/browser/cocoa/browser_accessibility.h:41: // isIgnored returns whetehr or
not the accessibility object
On 2010/07/21 12:50:41, pink wrote:
> typo: whether.

Done.

http://codereview.chromium.org/2951011/diff/6002/23002#newcode42
chrome/browser/cocoa/browser_accessibility.h:42: // should be ignored by the
accessibility heirarchy.
On 2010/07/21 12:50:41, pink wrote:
> hierarchy

Done.

http://codereview.chromium.org/2951011/diff/6002/23002#newcode44
chrome/browser/cocoa/browser_accessibility.h:44: // The origin of this object in
the pages document.
On 2010/07/21 12:50:41, pink wrote:
> page's

Done.

http://codereview.chromium.org/2951011/diff/6002/23002#newcode45
chrome/browser/cocoa/browser_accessibility.h:45: // This is in terms of webkit's
top-left origin, not Cocoa's
On 2010/07/21 12:50:41, pink wrote:
> s/in terms of/relative to ?

Done.

[pink (ping after 24hrs), 2010-07-23 19:17:54]

lgtm