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

chrome/browser/ui/cocoa/page_info_bubble_controller.mm

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "chrome/browser/ui/cocoa/page_info_bubble_controller.h"
 
 #include "app/l10n_util.h"
 #include "app/l10n_util_mac.h"
 #include "base/message_loop.h"
 #include "base/sys_string_conversions.h"
 #include "base/task.h"
@@ skipping 34 matching lines @@
 - (CGFloat)addSeparatorToSubviews:(NSMutableArray*)subviews
                          atOffset:(CGFloat)offset;
 - (NSPoint)anchorPointForWindowWithHeight:(CGFloat)bubbleHeight
                              parentWindow:(NSWindow*)parent;
 @end
 
 // This simple NSView subclass is used as the single subview of the page info
 // bubble's window's contentView. Drawing is flipped so that layout of the
 // sections is easier. Apple recommends flipping the coordinate origin when
 // doing a lot of text layout because it's more natural.
 @interface PageInfoContentView : NSView {

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

you can omit the braces in this case.

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

Done.

 }
 @end
 @implementation PageInfoContentView
 - (BOOL)isFlipped {
   return YES;
 }
 @end
 
 namespace {
 
 // The width of the window, in view coordinates. The height will be determined
 // by the content.
 const NSInteger kWindowWidth = 380;

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

since these are effectively coordinates, should they all be CGFloat's as well?

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

Done.

 
 // Spacing in between sections.
 const NSInteger kVerticalSpacing = 10;
 
 // Padding along on the X-axis between the window frame and content.
 const NSInteger kFramePadding = 10;
 
 // Spacing between the optional headline and description text views.
 const NSInteger kHeadlineSpacing = 2;
 
@@ skipping 307 matching lines @@
 
   return NSHeight([certButton frame]);
 }
 
 // Adds the state image at a pre-determined x position and the given y. This
 // does not affect the next Y position because the image is placed next to
 // a text field that is larger and accounts for the image's size.
 - (void)addImageViewForInfo:(const PageInfoModel::SectionInfo&)info
                  toSubviews:(NSMutableArray*)subviews
                    atOffset:(CGFloat)offset {
   NSRect frame = NSMakeRect(kFramePadding, offset, kImageSize,

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

i'd probably break this as:

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

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

Done.

       kImageSize);
   scoped_nsobject<NSImageView> imageView(
       [[NSImageView alloc] initWithFrame:frame]);
   [imageView setImageFrameStyle:NSImageFrameNone];
   [imageView setImage:model_->GetIconImage(info.icon_id)];
   [subviews addObject:imageView.get()];
 }
 
 // Adds the help center button that explains the icons. Returns the y position
 // delta for the next offset.
@@ skipping 44 matching lines @@
     LocationBarViewMac* locationBar = [controller locationBarBridge];
     if (locationBar) {
       NSPoint bubblePoint = locationBar->GetPageInfoBubblePoint();
       origin = [parent convertBaseToScreen:bubblePoint];
     }
   }
   return origin;
 }
 
 @end