Objective-C readability review for Robert Sesek <rsesek@google.com>.

Objective-C readability review for Robert Sesek <rsesek@google.com>;.

OCL=http://codereview.chromium.org/3461016/
BUG=http://b/3372125
TEST=none
R=pinkerton

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

[pink (ping after 24hrs), 2011-01-24 15:59:11]

A great start. Just very small nits.

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
File chrome/browser/ui/cocoa/page_info_bubble_controller.h (right):

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
chrome/browser/ui/cocoa/page_info_bubble_controller.h:26: @property (nonatomic,
assign) int certID;
no space between @property and (

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
chrome/browser/ui/cocoa/page_info_bubble_controller.h:33:
parentWindow:(NSWindow*)parentWindow;
is |parentWindow| allowed to be nil? What happens if you call showCertWindow and
it is?

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
File chrome/browser/ui/cocoa/page_info_bubble_controller.mm (right):

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
chrome/browser/ui/cocoa/page_info_bubble_controller.mm:56: @interface
PageInfoContentView : NSView {
you can omit the braces in this case.

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
chrome/browser/ui/cocoa/page_info_bubble_controller.mm:69: const NSInteger
kWindowWidth = 380;
since these are effectively coordinates, should they all be CGFloat's as well?

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
chrome/browser/ui/cocoa/page_info_bubble_controller.mm:397: NSRect frame =
NSMakeRect(kFramePadding, offset, kImageSize,
i'd probably break this as:

NSRect frame =
    NSMakeRect(.....);

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
File chrome/browser/ui/cocoa/page_info_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
chrome/browser/ui/cocoa/page_info_bubble_controller_unittest.mm:189: 
omit extra line

[Robert Sesek, 2011-01-24 16:39:56]

Thanks! All feedback addressed.

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
File chrome/browser/ui/cocoa/page_info_bubble_controller.h (right):

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
chrome/browser/ui/cocoa/page_info_bubble_controller.h:26: @property (nonatomic,
assign) int certID;
On 2011/01/24 15:59:11, pink wrote:
> no space between @property and (

The style guide doesn't have this, and we use a mix in Chromium. Should we
standardize one of the two and put it in the guide? If so, I'll message
objc-style and put together a CL.

http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Properties

Here are the actual usage numbers:
$ git gs "@property (" | wc -l
     188
$ git gs "@property(" | wc -l
     118

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
chrome/browser/ui/cocoa/page_info_bubble_controller.h:33:
parentWindow:(NSWindow*)parentWindow;
On 2011/01/24 15:59:11, pink wrote:
> is |parentWindow| allowed to be nil? What happens if you call showCertWindow
and
> it is?

Can't be nil and never is in practice. Added a comment and a DCHCECK.

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
File chrome/browser/ui/cocoa/page_info_bubble_controller.mm (right):

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
chrome/browser/ui/cocoa/page_info_bubble_controller.mm:56: @interface
PageInfoContentView : NSView {
On 2011/01/24 15:59:11, pink wrote:
> you can omit the braces in this case.

Done.

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
chrome/browser/ui/cocoa/page_info_bubble_controller.mm:69: const NSInteger
kWindowWidth = 380;
On 2011/01/24 15:59:11, pink wrote:
> since these are effectively coordinates, should they all be CGFloat's as well?

Done.

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
chrome/browser/ui/cocoa/page_info_bubble_controller.mm:397: NSRect frame =
NSMakeRect(kFramePadding, offset, kImageSize,
On 2011/01/24 15:59:11, pink wrote:
> i'd probably break this as:
> 
> NSRect frame =
>     NSMakeRect(.....);

Done.

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
File chrome/browser/ui/cocoa/page_info_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
chrome/browser/ui/cocoa/page_info_bubble_controller_unittest.mm:189: 
On 2011/01/24 15:59:11, pink wrote:
> omit extra line

Done.

[Robert Sesek, 2011-01-24 17:02:42]

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
File chrome/browser/ui/cocoa/page_info_bubble_controller.h (right):

http://codereview.chromium.org/6360007/diff/1/chrome/browser/ui/cocoa/page_in...
chrome/browser/ui/cocoa/page_info_bubble_controller.h:26: @property (nonatomic,
assign) int certID;
On 2011/01/24 15:59:11, pink wrote:
> no space between @property and (

Removed.

[pink (ping after 24hrs), 2011-01-24 19:22:23]

LGTM

http://codereview.chromium.org/6360007/diff/9001/chrome/browser/ui/cocoa/page...
File chrome/browser/ui/cocoa/page_info_bubble_controller.mm (right):

http://codereview.chromium.org/6360007/diff/9001/chrome/browser/ui/cocoa/page...
chrome/browser/ui/cocoa/page_info_bubble_controller.mm:95: class
PageInfoModelBubbleBridge : public PageInfoModel::PageInfoModelObserver {
worth adding a DISALLOW_COPY_AND_ASSIGN on this?

[Robert Sesek, 2011-01-24 19:55:17]

Submitting.

http://codereview.chromium.org/6360007/diff/9001/chrome/browser/ui/cocoa/page...
File chrome/browser/ui/cocoa/page_info_bubble_controller.mm (right):

http://codereview.chromium.org/6360007/diff/9001/chrome/browser/ui/cocoa/page...
chrome/browser/ui/cocoa/page_info_bubble_controller.mm:95: class
PageInfoModelBubbleBridge : public PageInfoModel::PageInfoModelObserver {
On 2011/01/24 19:22:23, pink wrote:
> worth adding a DISALLOW_COPY_AND_ASSIGN on this?

Done.