[Mac] Convert the page info window to a bubble.

[Mac] Convert the page info window to a bubble.

BUG=52430, 52916
TEST=Enable Page Info Bubble in about:labs and click on the lock icon.

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

[Nico, 2010-09-22 16:04:29]

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

http://codereview.chromium.org/3461016/diff/1/3#newcode56
chrome/browser/cocoa/base_bubble_controller.mm:56: DCHECK(theWindow);
DCHECK([theWindow isKindOfClass:[InfoBubbleWindow class]]);?

[Robert Sesek, 2010-09-28 19:39:47]

This is now ready for review.  It's not quite perfect yet, but this CL is big
enough as-is.  The outstanding issue is that the position and size are not
perfect because of a bug I haven't yet tracked down in
BaseBubbleController/InfoBubbleWindow.

Screen shot: http://cl.ly/cae853d3eaf92a3b7cce

[Nico, 2010-09-28 20:25:50]

Could you use one of the two button styles ( :-/ ) we use in the popup
blocker bubble?

On Tue, Sep 28, 2010 at 12:39 PM,  <rsesek@chromium.org> wrote:
> Reviewers: Nico, pink,
>
> Message:
> This is now ready for review.  It's not quite perfect yet, but this CL is
> big
> enough as-is.  The outstanding issue is that the position and size are not
> perfect because of a bug I haven't yet tracked down in
> BaseBubbleController/InfoBubbleWindow.
>
> Screen shot: http://cl.ly/cae853d3eaf92a3b7cce
>
> Description:
> [Mac] Convert the page info window to a bubble.
>
> BUG=52430,52916
> TEST=Run with --enable-new-page-info-bubble and click the lock icon.
>
> Please review this at http://codereview.chromium.org/3461016/show
>
> SVN Base: http://src.chromium.org/git/chromium.git
>
> Affected files:
>  M chrome/browser/cocoa/base_bubble_controller.h
>  M chrome/browser/cocoa/base_bubble_controller.mm
>  M chrome/browser/cocoa/location_bar/location_bar_view_mac.h
>  M chrome/browser/cocoa/location_bar/location_bar_view_mac.mm
>  M chrome/browser/cocoa/location_bar/location_icon_decoration.h
>  M chrome/browser/cocoa/location_bar/location_icon_decoration.mm
>  A chrome/browser/cocoa/page_info_bubble_controller.h
>  A chrome/browser/cocoa/page_info_bubble_controller.mm
>  A chrome/browser/cocoa/page_info_bubble_controller_unittest.mm
>  M chrome/browser/cocoa/page_info_window_mac.mm
>  M chrome/browser/labs.cc
>  M chrome/browser/page_info_model.h
>  M chrome/chrome_browser.gypi
>  M chrome/chrome_tests.gypi
>
>
>

[Nico, 2010-09-28 22:02:45]

Nice!

What's with the two spaces after periods in comments though? I recommend you
read http://www.amazon.com/Pc-Not-Typewriter-Robin-Williams/dp/0938151495
(actually searching the review comments for "two spaces" is probably enough.
Disclosure: I'm not affiliated with the author of that book and have in fact
never read it myself.)

http://codereview.chromium.org/3461016/diff/4001/5001
File chrome/browser/cocoa/base_bubble_controller.h (right):

http://codereview.chromium.org/3461016/diff/4001/5001#newcode57
chrome/browser/cocoa/base_bubble_controller.h:57: ancoredAt:(NSPoint)ancoredAt;
anchored

http://codereview.chromium.org/3461016/diff/4001/5002
File chrome/browser/cocoa/base_bubble_controller.mm (right):

http://codereview.chromium.org/3461016/diff/4001/5002#newcode59
chrome/browser/cocoa/base_bubble_controller.mm:59: anchor_ = ancoredAt;
dcheck(![theWindow delegate])

http://codereview.chromium.org/3461016/diff/4001/5002#newcode72
chrome/browser/cocoa/base_bubble_controller.mm:72: }
do you need to do the stuff that's done in awakeFromNib? even if not, should you
somehow make sure that when someone adds stuff to awakeFromNib later, that this
stuff is done in the non-nib path too?

http://codereview.chromium.org/3461016/diff/4001/5006
File chrome/browser/cocoa/location_bar/location_icon_decoration.mm (right):

http://codereview.chromium.org/3461016/diff/4001/5006#newcode20
chrome/browser/cocoa/location_bar/location_icon_decoration.mm:20: const CGFloat
kBubblePointYOffset = 2.0;
no need to put consts into namespaces, they have implicit static linkage already

http://codereview.chromium.org/3461016/diff/4001/5006#newcode58
chrome/browser/cocoa/location_bar/location_icon_decoration.mm:58: NSPoint
LocationIconDecoration::GetBubblePointInFrame(NSRect frame) {
maybe add a comment "// This is all in view coordinates."

http://codereview.chromium.org/3461016/diff/4001/5007
File chrome/browser/cocoa/page_info_bubble_controller.h (right):

http://codereview.chromium.org/3461016/diff/4001/5007#newcode36
chrome/browser/cocoa/page_info_bubble_controller.h:36: // controller will
release itself when the bubble is closed.
nice comment

http://codereview.chromium.org/3461016/diff/4001/5008
File chrome/browser/cocoa/page_info_bubble_controller.mm (right):

http://codereview.chromium.org/3461016/diff/4001/5008#newcode45
chrome/browser/cocoa/page_info_bubble_controller.mm:45: // BrowserWindow, and
gets the proper anchor point for the bubble.
// The returned point is in screen coordinates.

http://codereview.chromium.org/3461016/diff/4001/5008#newcode47
chrome/browser/cocoa/page_info_bubble_controller.mm:47: BrowserWindowController*
bwc = [parent windowController];
s/bwc/controller/

http://codereview.chromium.org/3461016/diff/4001/5008#newcode50
chrome/browser/cocoa/page_info_bubble_controller.mm:50: LocationBarViewMac* lbvm
= [bwc locationBarBridge];
s/lbvm/location_bar/

http://codereview.chromium.org/3461016/diff/4001/5008#newcode162
chrome/browser/cocoa/page_info_bubble_controller.mm:162: - (void)performLayout {
"this is too long"

http://codereview.chromium.org/3461016/diff/4001/5008#newcode164
chrome/browser/cocoa/page_info_bubble_controller.mm:164: // |offset| is the Y
position that should be drawn at next.
// Layout is done in increasing y direction, i.e. from bottom to top.

http://codereview.chromium.org/3461016/diff/4001/5008#newcode181
chrome/browser/cocoa/page_info_bubble_controller.mm:181: // lower left.
ah, here it is. nevermind the last comment :-)

http://codereview.chromium.org/3461016/diff/4001/5008#newcode203
chrome/browser/cocoa/page_info_bubble_controller.mm:203: [certButton
setAction:@selector(showCertWindow:)];
i think this block would look awesome in a helper function

http://codereview.chromium.org/3461016/diff/4001/5008#newcode232
chrome/browser/cocoa/page_info_bubble_controller.mm:232: [textField
setStringValue:base::SysUTF16ToNSString(info.description)];
i believe there's a "CreateLabel()" function or similar somewhere in c/b/cocoa.
can you use that?

http://codereview.chromium.org/3461016/diff/4001/5008#newcode243
chrome/browser/cocoa/page_info_bubble_controller.mm:243: offset += 2;  // A
little spacing between title and content.
use a constant, then you don't need the comment

http://codereview.chromium.org/3461016/diff/4001/5008#newcode253
chrome/browser/cocoa/page_info_bubble_controller.mm:253: [titleField
setFont:titleFont];
see comment about the other text field

http://codereview.chromium.org/3461016/diff/4001/5008#newcode283
chrome/browser/cocoa/page_info_bubble_controller.mm:283: }
this switch could be moved into a helper too

http://codereview.chromium.org/3461016/diff/4001/5009
File chrome/browser/cocoa/page_info_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/3461016/diff/4001/5009#newcode30
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:30: type));
might fit into one line?

http://codereview.chromium.org/3461016/diff/4001/5009#newcode41
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:41: virtual void
SetUp() {
if you do this in the constructor, you don't have that iffy explicit
superclass::call

http://codereview.chromium.org/3461016/diff/4001/5009#newcode49
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:49:
CocoaTest::TearDown();
same

http://codereview.chromium.org/3461016/diff/4001/5009#newcode62
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:62: // Checks the
controller's window for the requisite subviews in the given
cheap foreign labor doesn't understand words like "requisite" (just saying.
don't do anything here. now that i looked it up, it's kinda obvious what it
means though)

http://codereview.chromium.org/3461016/diff/4001/5009#newcode67
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:67: int
button_count) {
hm, the call sites of this will look pretty sad. can you create a small helper
class that has setters for the 4 counts and does the test in its destructor (or
a Check() method) so that the calling sites look like this

CheckCountsOf().Texts(4).Images(2).Spacers(1).Buttons(1);

?

("Dear reviewer, this is overkill" is a valid reply I think)

http://codereview.chromium.org/3461016/diff/4001/5009#newcode79
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:79:
EXPECT_TRUE(false) << "Unknown subview: " <<
isn't there some predefined thing for EXPECT_TRUE(false)? FAILS()?

http://codereview.chromium.org/3461016/diff/4001/5009#newcode101
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:101: 
kill newline?

[pink (ping after 24hrs), 2010-09-29 13:06:25]

On Tue, Sep 28, 2010 at 6:02 PM,  <thakis@chromium.org> wrote:
> What's with the two spaces after periods in comments though? I recommend you
> read http://www.amazon.com/Pc-Not-Typewriter-Robin-Williams/dp/0938151495
> (actually searching the review comments for "two spaces" is probably enough.
> Disclosure: I'm not affiliated with the author of that book and have in fact
> never read it myself.)

While I wholly agree with you, note that the Xcode plug-in comment
wrapper that we have in our standard google mac tool collection
re-formats with two spaces after periods. As much as it bugs me,
sometimes I just have to let it go....

-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[Robert Sesek, 2010-09-29 13:52:35]

Addressed all of thakis' comments.  Thanks!

On 2010/09/29 13:06:25, pink wrote:
> On Tue, Sep 28, 2010 at 6:02 PM,  <mailto:thakis@chromium.org> wrote:
> > What's with the two spaces after periods in comments though? I recommend you
> > read http://www.amazon.com/Pc-Not-Typewriter-Robin-Williams/dp/0938151495
> > (actually searching the review comments for "two spaces" is probably enough.
> > Disclosure: I'm not affiliated with the author of that book and have in fact
> > never read it myself.)
> 
> While I wholly agree with you, note that the Xcode plug-in comment
> wrapper that we have in our standard google mac tool collection
> re-formats with two spaces after periods. As much as it bugs me,
> sometimes I just have to let it go....

I consulted The Elements of Typographic Style, p28, which recommends to use a
single space between sentences, so I changed my CL. They say that "your typing
as well as your typesetting will benefit from unlearning this quaint Victorian
habit."

http://codereview.chromium.org/3461016/diff/4001/5001
File chrome/browser/cocoa/base_bubble_controller.h (right):

http://codereview.chromium.org/3461016/diff/4001/5001#newcode57
chrome/browser/cocoa/base_bubble_controller.h:57: ancoredAt:(NSPoint)ancoredAt;
On 2010/09/28 22:02:45, Nico wrote:
> anchored

This is |ancoredAt| to be consistent with the first initializer; I also think
it's more correct.

http://codereview.chromium.org/3461016/diff/4001/5002
File chrome/browser/cocoa/base_bubble_controller.mm (right):

http://codereview.chromium.org/3461016/diff/4001/5002#newcode59
chrome/browser/cocoa/base_bubble_controller.mm:59: anchor_ = ancoredAt;
On 2010/09/28 22:02:45, Nico wrote:
> dcheck(![theWindow delegate])

Done.

http://codereview.chromium.org/3461016/diff/4001/5002#newcode72
chrome/browser/cocoa/base_bubble_controller.mm:72: }
On 2010/09/28 22:02:45, Nico wrote:
> do you need to do the stuff that's done in awakeFromNib? even if not, should
you
> somehow make sure that when someone adds stuff to awakeFromNib later, that
this
> stuff is done in the non-nib path too?

Good call on the |-awakeFromNib|.

http://codereview.chromium.org/3461016/diff/4001/5006
File chrome/browser/cocoa/location_bar/location_icon_decoration.mm (right):

http://codereview.chromium.org/3461016/diff/4001/5006#newcode20
chrome/browser/cocoa/location_bar/location_icon_decoration.mm:20: const CGFloat
kBubblePointYOffset = 2.0;
On 2010/09/28 22:02:45, Nico wrote:
> no need to put consts into namespaces, they have implicit static linkage
already

Done.

http://codereview.chromium.org/3461016/diff/4001/5006#newcode58
chrome/browser/cocoa/location_bar/location_icon_decoration.mm:58: NSPoint
LocationIconDecoration::GetBubblePointInFrame(NSRect frame) {
On 2010/09/28 22:02:45, Nico wrote:
> maybe add a comment "// This is all in view coordinates."

Done in the .h.

http://codereview.chromium.org/3461016/diff/4001/5008
File chrome/browser/cocoa/page_info_bubble_controller.mm (right):

http://codereview.chromium.org/3461016/diff/4001/5008#newcode45
chrome/browser/cocoa/page_info_bubble_controller.mm:45: // BrowserWindow, and
gets the proper anchor point for the bubble.
On 2010/09/28 22:02:45, Nico wrote:
> // The returned point is in screen coordinates.

Done.

http://codereview.chromium.org/3461016/diff/4001/5008#newcode47
chrome/browser/cocoa/page_info_bubble_controller.mm:47: BrowserWindowController*
bwc = [parent windowController];
On 2010/09/28 22:02:45, Nico wrote:
> s/bwc/controller/

Done.

http://codereview.chromium.org/3461016/diff/4001/5008#newcode50
chrome/browser/cocoa/page_info_bubble_controller.mm:50: LocationBarViewMac* lbvm
= [bwc locationBarBridge];
On 2010/09/28 22:02:45, Nico wrote:
> s/lbvm/location_bar/

Done.

http://codereview.chromium.org/3461016/diff/4001/5008#newcode203
chrome/browser/cocoa/page_info_bubble_controller.mm:203: [certButton
setAction:@selector(showCertWindow:)];
On 2010/09/28 22:02:45, Nico wrote:
> i think this block would look awesome in a helper function

Done.

http://codereview.chromium.org/3461016/diff/4001/5008#newcode232
chrome/browser/cocoa/page_info_bubble_controller.mm:232: [textField
setStringValue:base::SysUTF16ToNSString(info.description)];
On 2010/09/28 22:02:45, Nico wrote:
> i believe there's a "CreateLabel()" function or similar somewhere in
c/b/cocoa.
> can you use that?

That's in the translate_infobar_base.h utilities. I just created a helper
function to configure the text fields.

http://codereview.chromium.org/3461016/diff/4001/5008#newcode243
chrome/browser/cocoa/page_info_bubble_controller.mm:243: offset += 2;  // A
little spacing between title and content.
On 2010/09/28 22:02:45, Nico wrote:
> use a constant, then you don't need the comment

Done.

http://codereview.chromium.org/3461016/diff/4001/5008#newcode283
chrome/browser/cocoa/page_info_bubble_controller.mm:283: }
On 2010/09/28 22:02:45, Nico wrote:
> this switch could be moved into a helper too

Done.

http://codereview.chromium.org/3461016/diff/4001/5009
File chrome/browser/cocoa/page_info_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/3461016/diff/4001/5009#newcode30
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:30: type));
On 2010/09/28 22:02:45, Nico wrote:
> might fit into one line?

nope

http://codereview.chromium.org/3461016/diff/4001/5009#newcode41
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:41: virtual void
SetUp() {
On 2010/09/28 22:02:45, Nico wrote:
> if you do this in the constructor, you don't have that iffy explicit
> superclass::call

Done.

http://codereview.chromium.org/3461016/diff/4001/5009#newcode49
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:49:
CocoaTest::TearDown();
On 2010/09/28 22:02:45, Nico wrote:
> same

This actually needs to remain in TearDown().

http://codereview.chromium.org/3461016/diff/4001/5009#newcode67
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:67: int
button_count) {
On 2010/09/28 22:02:45, Nico wrote:
> hm, the call sites of this will look pretty sad. can you create a small helper
> class that has setters for the 4 counts and does the test in its destructor
(or
> a Check() method) so that the calling sites look like this
> 
> CheckCountsOf().Texts(4).Images(2).Spacers(1).Buttons(1);
> 
> ?
> 
> ("Dear reviewer, this is overkill" is a valid reply I think)

That is a nice idea, but serious overkill.  I just added some in-line comments
for each parameter when calling this method.

http://codereview.chromium.org/3461016/diff/4001/5009#newcode101
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:101: 
On 2010/09/28 22:02:45, Nico wrote:
> kill newline?

Done.

[Nico, 2010-09-29 18:09:56]

http://codereview.chromium.org/3461016/diff/4001/5008
File chrome/browser/cocoa/page_info_bubble_controller.mm (right):

http://codereview.chromium.org/3461016/diff/4001/5008#newcode232
chrome/browser/cocoa/page_info_bubble_controller.mm:232: [textField
setStringValue:base::SysUTF16ToNSString(info.description)];
On 2010/09/29 13:52:35, rsesek wrote:
> On 2010/09/28 22:02:45, Nico wrote:
> > i believe there's a "CreateLabel()" function or similar somewhere in
> c/b/cocoa.
> > can you use that?
> 
> That's in the translate_infobar_base.h utilities. I just created a helper
> function to configure the text fields.

There's also LabelWithFrame in content_bubble_cocoa. Maybe we should have a
cocoa_utils file for functions like these?

http://codereview.chromium.org/3461016/diff/4001/5009
File chrome/browser/cocoa/page_info_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/3461016/diff/4001/5009#newcode49
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:49:
CocoaTest::TearDown();
On 2010/09/29 13:52:35, rsesek wrote:
> On 2010/09/28 22:02:45, Nico wrote:
> > same
> 
> This actually needs to remain in TearDown().

Why? Because TearDown() is called before the constructor is, and
CocoaTest::TearDown() expects stuff to disappear?

http://codereview.chromium.org/3461016/diff/4003/17008
File chrome/browser/cocoa/page_info_bubble_controller.mm (right):

http://codereview.chromium.org/3461016/diff/4003/17008#newcode128
chrome/browser/cocoa/page_info_bubble_controller.mm:128:
ancoredAt:anchorPoint])) {
spello

http://codereview.chromium.org/3461016/diff/4003/17008#newcode203
chrome/browser/cocoa/page_info_bubble_controller.mm:203: [self
createCertificateButtonWithFrame:buttonRect];  // autoreleased
kill comment, see below

http://codereview.chromium.org/3461016/diff/4003/17008#newcode219
chrome/browser/cocoa/page_info_bubble_controller.mm:219: }
this block could be a function -addCertButtonToSubviews:subviews atOffset:offset
(could return the new offset)

Same for all the other subview types. This function is still too long.

http://codereview.chromium.org/3461016/diff/4003/17008#newcode290
chrome/browser/cocoa/page_info_bubble_controller.mm:290: windowFrame.size.height
+= offset;
offset is in view coordinates, windowFrame in window coordinates. this won't
work with resolution independence, you need to convert here
(convertSize:toView:nil)

http://codereview.chromium.org/3461016/diff/4003/17008#newcode303
chrome/browser/cocoa/page_info_bubble_controller.mm:303: -
(NSButton*)createCertificateButtonWithFrame:(NSRect)frame {
functions that return autoreleased stuff shouln't contain "create" in their
name. just s/create//

[Robert Sesek, 2010-09-30 14:52:40]

All comments addressed. I also changed the button appearance:
http://cl.ly/9f42f48712e05baba295

http://codereview.chromium.org/3461016/diff/4001/5009
File chrome/browser/cocoa/page_info_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/3461016/diff/4001/5009#newcode49
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:49:
CocoaTest::TearDown();
On 2010/09/29 18:09:56, Nico wrote:
> On 2010/09/29 13:52:35, rsesek wrote:
> > On 2010/09/28 22:02:45, Nico wrote:
> > > same
> > 
> > This actually needs to remain in TearDown().
> 
> Why? Because TearDown() is called before the constructor is, and
> CocoaTest::TearDown() expects stuff to disappear?

Exactly. CocoaTest::TearDown() attempts to close any open windows, which means
this will double-free the window if this were in the dtor.

http://codereview.chromium.org/3461016/diff/4003/17008
File chrome/browser/cocoa/page_info_bubble_controller.mm (right):

http://codereview.chromium.org/3461016/diff/4003/17008#newcode128
chrome/browser/cocoa/page_info_bubble_controller.mm:128:
ancoredAt:anchorPoint])) {
On 2010/09/29 18:09:56, Nico wrote:
> spello

Done.

http://codereview.chromium.org/3461016/diff/4003/17008#newcode203
chrome/browser/cocoa/page_info_bubble_controller.mm:203: [self
createCertificateButtonWithFrame:buttonRect];  // autoreleased
On 2010/09/29 18:09:56, Nico wrote:
> kill comment, see below

Done.

http://codereview.chromium.org/3461016/diff/4003/17008#newcode219
chrome/browser/cocoa/page_info_bubble_controller.mm:219: }
On 2010/09/29 18:09:56, Nico wrote:
> this block could be a function -addCertButtonToSubviews:subviews
atOffset:offset
> (could return the new offset)
> 
> Same for all the other subview types. This function is still too long.

Done.

http://codereview.chromium.org/3461016/diff/4003/17008#newcode290
chrome/browser/cocoa/page_info_bubble_controller.mm:290: windowFrame.size.height
+= offset;
On 2010/09/29 18:09:56, Nico wrote:
> offset is in view coordinates, windowFrame in window coordinates. this won't
> work with resolution independence, you need to convert here
> (convertSize:toView:nil)

Done.

http://codereview.chromium.org/3461016/diff/4003/17008#newcode303
chrome/browser/cocoa/page_info_bubble_controller.mm:303: -
(NSButton*)createCertificateButtonWithFrame:(NSRect)frame {
On 2010/09/29 18:09:56, Nico wrote:
> functions that return autoreleased stuff shouln't contain "create" in their
> name. just s/create//

Done.

[Nico, 2010-09-30 15:10:18]

LG!

I would indent sections without image the same way you do indent sections with
image, so that all the section texts are aligned to the same left edge.

If the connection isn't secure due to non-https resources on an https site, are
there plans to show a list of the non-secure urls on the page? I heard a few
people asking for this.

http://codereview.chromium.org/3461016/diff/4001/5009
File chrome/browser/cocoa/page_info_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/3461016/diff/4001/5009#newcode30
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:30: type));
On 2010/09/29 13:52:35, rsesek wrote:
> On 2010/09/28 22:02:45, Nico wrote:
> > might fit into one line?
> 
> nope

I meant like this:

    sections_.push_back(SectionInfo(
        state, title, string16(), description, type));

http://codereview.chromium.org/3461016/diff/24001/25008
File chrome/browser/cocoa/page_info_bubble_controller.mm (right):

http://codereview.chromium.org/3461016/diff/24001/25008#newcode43
chrome/browser/cocoa/page_info_bubble_controller.mm:43: // The width of the
window. The height will be determined by the content.
"The width of the window, in view coordinates."

http://codereview.chromium.org/3461016/diff/24001/25008#newcode192
chrome/browser/cocoa/page_info_bubble_controller.mm:192: NSRect windowFrame =
NSMakeRect(0, 0, kWindowWidth, 50);
you don't use this until 50 lines later. can you move the declaration to there?

http://codereview.chromium.org/3461016/diff/24001/25008#newcode253
chrome/browser/cocoa/page_info_bubble_controller.mm:253:
windowFrame.size.height);
NSHeight

http://codereview.chromium.org/3461016/diff/24001/25008#newcode368
chrome/browser/cocoa/page_info_bubble_controller.mm:368: -
(CGFloat)addImageViewForInfo:(const PageInfoModel::SectionInfo&)info
let this return void (and update the comment)?

[Robert Sesek, 2010-09-30 15:21:41]

Thanks. Addressed the nits; will land after green tryjob.

On 2010/09/30 15:10:18, Nico wrote:
> LG!
> 
> I would indent sections without image the same way you do indent sections with
> image, so that all the section texts are aligned to the same left edge.
> 

That's not how the mocks are.

> If the connection isn't secure due to non-https resources on an https site,
are
> there plans to show a list of the non-secure urls on the page? I heard a few
> people asking for this.

I don't know; I agree that would be a nice feature, though.

http://codereview.chromium.org/3461016/diff/4001/5009
File chrome/browser/cocoa/page_info_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/3461016/diff/4001/5009#newcode30
chrome/browser/cocoa/page_info_bubble_controller_unittest.mm:30: type));
On 2010/09/30 15:10:18, Nico wrote:
> On 2010/09/29 13:52:35, rsesek wrote:
> > On 2010/09/28 22:02:45, Nico wrote:
> > > might fit into one line?
> > 
> > nope
> 
> I meant like this:
> 
>     sections_.push_back(SectionInfo(
>         state, title, string16(), description, type));
> 

Done.

http://codereview.chromium.org/3461016/diff/24001/25008
File chrome/browser/cocoa/page_info_bubble_controller.mm (right):

http://codereview.chromium.org/3461016/diff/24001/25008#newcode43
chrome/browser/cocoa/page_info_bubble_controller.mm:43: // The width of the
window. The height will be determined by the content.
On 2010/09/30 15:10:18, Nico wrote:
> "The width of the window, in view coordinates."

Done.

http://codereview.chromium.org/3461016/diff/24001/25008#newcode192
chrome/browser/cocoa/page_info_bubble_controller.mm:192: NSRect windowFrame =
NSMakeRect(0, 0, kWindowWidth, 50);
On 2010/09/30 15:10:18, Nico wrote:
> you don't use this until 50 lines later. can you move the declaration to
there?

Done.

http://codereview.chromium.org/3461016/diff/24001/25008#newcode253
chrome/browser/cocoa/page_info_bubble_controller.mm:253:
windowFrame.size.height);
On 2010/09/30 15:10:18, Nico wrote:
> NSHeight

Done.

http://codereview.chromium.org/3461016/diff/24001/25008#newcode368
chrome/browser/cocoa/page_info_bubble_controller.mm:368: -
(CGFloat)addImageViewForInfo:(const PageInfoModel::SectionInfo&)info
On 2010/09/30 15:10:18, Nico wrote:
> let this return void (and update the comment)?

Done.