Mac: Fill in geolocation bubble from model.

Mac: Fill in geolocation bubble from model.

xib changes: Removed everything above the horizontal line and put an empty NSView there, as container for the geo stuff (maybe later, there should be only one xib file for all bubble types and the container should be filled from the bubble model in all cases. Not in this CL.)

http://imgur.com/AtXiq

BUG=11246
TEST=Start chromium with --enable-geolocation. Go to http://maxheapsize.com/static/html5geolocationdemo.html , click "Allow" or "Deny". Bubble should appear. It should behave like on windows or linux (except that it has buttons instead of links. We only use links for stuff that opens web pages on clicking on OS X.)

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

[Nico, 2010-03-28 22:13:49]

After this, only the exceptions dialog should be missing.

[viettrungluu, 2010-03-29 00:37:35]

From the screenshot, the top margin looks too small.

http://codereview.chromium.org/1542001/diff/2002/6003
File chrome/browser/cocoa/content_blocked_bubble_controller.mm (right):

http://codereview.chromium.org/1542001/diff/2002/6003#newcode69
chrome/browser/cocoa/content_blocked_bubble_controller.mm:69: float fontSize =
[NSFont systemFontSizeForControlSize:controlSize];
CGFloat

http://codereview.chromium.org/1542001/diff/2002/6003#newcode78
chrome/browser/cocoa/content_blocked_bubble_controller.mm:78: NSTextField*
LabelWithFrame(NSString* text, const NSRect& frame) {
"LabelWithFrame" isn't a respectable C++ function name. Try "MakeLabel" or some
such.

http://codereview.chromium.org/1542001/diff/2002/6003#newcode287
chrome/browser/cocoa/content_blocked_bubble_controller.mm:287: // order of
operations is the other way than on linux/windows.
Linux/Windows

http://codereview.chromium.org/1542001/diff/2002/6003#newcode310
chrome/browser/cocoa/content_blocked_bubble_controller.mm:310: typedef
I'm unconvinced this typedef is needed.

http://codereview.chromium.org/1542001/diff/2002/6003#newcode314
chrome/browser/cocoa/content_blocked_bubble_controller.mm:314: i !=
content.domain_lists.rend(); ++i) {
Indent one more space to align along (.

http://codereview.chromium.org/1542001/diff/2002/6003#newcode317
chrome/browser/cocoa/content_blocked_bubble_controller.mm:317: j !=
i->hosts.rend(); ++j) {
Ditto.

http://codereview.chromium.org/1542001/diff/2002/6003#newcode324
chrome/browser/cocoa/content_blocked_bubble_controller.mm:324: frame.origin.y =
NSMaxY(frame) + kGeoHostPadding;
What I said below.

http://codereview.chromium.org/1542001/diff/2002/6003#newcode326
chrome/browser/cocoa/content_blocked_bubble_controller.mm:326: frame.origin.y +=
kGeoPadding - kGeoHostPadding;
What I said further below -- is there a chance that no kGeoHostPadding was
added?

http://codereview.chromium.org/1542001/diff/2002/6003#newcode336
chrome/browser/cocoa/content_blocked_bubble_controller.mm:336: frame.origin.y =
NSMaxY(frame) + kGeoPadding;
The juxtaposition of this line with the previous is confusing. Hmm.

http://codereview.chromium.org/1542001/diff/2002/6003#newcode338
chrome/browser/cocoa/content_blocked_bubble_controller.mm:338: CGFloat
containerHeight = frame.origin.y - kGeoPadding;  // Undo last padding.
Is there a chance that there was no last padding?

http://codereview.chromium.org/1542001/diff/2002/6003#newcode359
chrome/browser/cocoa/content_blocked_bubble_controller.mm:359: ==
CONTENT_SETTINGS_TYPE_POPUPS)
Style guide dictates that == should be on previous line. I'd personally indent
this line a further 4 spaces.

http://codereview.chromium.org/1542001/diff/2002/6003#newcode361
chrome/browser/cocoa/content_blocked_bubble_controller.mm:361: if
(contentSettingBubbleModel_->content_type()
Ditto.

[Nico, 2010-03-29 01:28:40]

Thanks!

http://codereview.chromium.org/1542001/diff/2002/6003
File chrome/browser/cocoa/content_blocked_bubble_controller.mm (right):

http://codereview.chromium.org/1542001/diff/2002/6003#newcode69
chrome/browser/cocoa/content_blocked_bubble_controller.mm:69: float fontSize =
[NSFont systemFontSizeForControlSize:controlSize];
On 2010/03/29 00:37:35, viettrungluu wrote:
> CGFloat

Done. (This is from
http://developer.apple.com/Mac/library/documentation/Cocoa/Conceptual/Control...
, which uses float. But probably cause that page predates CGFloat.)

http://codereview.chromium.org/1542001/diff/2002/6003#newcode78
chrome/browser/cocoa/content_blocked_bubble_controller.mm:78: NSTextField*
LabelWithFrame(NSString* text, const NSRect& frame) {
On 2010/03/29 00:37:35, viettrungluu wrote:
> "LabelWithFrame" isn't a respectable C++ function name. Try "MakeLabel" or
some
> such.

But then it's not clear if the returned object is autoreleased or not. It's
clearer this way imho.

http://codereview.chromium.org/1542001/diff/2002/6003#newcode287
chrome/browser/cocoa/content_blocked_bubble_controller.mm:287: // order of
operations is the other way than on linux/windows.
On 2010/03/29 00:37:35, viettrungluu wrote:
> Linux/Windows

Done.

http://codereview.chromium.org/1542001/diff/2002/6003#newcode310
chrome/browser/cocoa/content_blocked_bubble_controller.mm:310: typedef
On 2010/03/29 00:37:35, viettrungluu wrote:
> I'm unconvinced this typedef is needed.

I want to declare the iterator in the for loop header, and without the typedef
this gets very wacky wrt 80 cols.

http://codereview.chromium.org/1542001/diff/2002/6003#newcode314
chrome/browser/cocoa/content_blocked_bubble_controller.mm:314: i !=
content.domain_lists.rend(); ++i) {
On 2010/03/29 00:37:35, viettrungluu wrote:
> Indent one more space to align along (.

Done.

http://codereview.chromium.org/1542001/diff/2002/6003#newcode317
chrome/browser/cocoa/content_blocked_bubble_controller.mm:317: j !=
i->hosts.rend(); ++j) {
On 2010/03/29 00:37:35, viettrungluu wrote:
> Ditto.

Done.

http://codereview.chromium.org/1542001/diff/2002/6003#newcode326
chrome/browser/cocoa/content_blocked_bubble_controller.mm:326: frame.origin.y +=
kGeoPadding - kGeoHostPadding;
On 2010/03/29 00:37:35, viettrungluu wrote:
> What I said further below -- is there a chance that no kGeoHostPadding was
> added?

Shouldn't happen either, but it's less clear cut. Added an if.

http://codereview.chromium.org/1542001/diff/2002/6003#newcode336
chrome/browser/cocoa/content_blocked_bubble_controller.mm:336: frame.origin.y =
NSMaxY(frame) + kGeoPadding;
On 2010/03/29 00:37:35, viettrungluu wrote:
> The juxtaposition of this line with the previous is confusing. Hmm.

You think? How would you write it?

http://codereview.chromium.org/1542001/diff/2002/6003#newcode338
chrome/browser/cocoa/content_blocked_bubble_controller.mm:338: CGFloat
containerHeight = frame.origin.y - kGeoPadding;  // Undo last padding.
On 2010/03/29 00:37:35, viettrungluu wrote:
> Is there a chance that there was no last padding?

Only if there's no group, which shouldn't happen. But good point. Since just
adding an if is as long as expanding the comment, I just added the if.

http://codereview.chromium.org/1542001/diff/2002/6003#newcode359
chrome/browser/cocoa/content_blocked_bubble_controller.mm:359: ==
CONTENT_SETTINGS_TYPE_POPUPS)
On 2010/03/29 00:37:35, viettrungluu wrote:
> Style guide dictates that == should be on previous line. I'd personally indent
> this line a further 4 spaces.

Moved ==. Tried the additional indent, thought it looked weird, did not change
it.

http://codereview.chromium.org/1542001/diff/2002/6003#newcode361
chrome/browser/cocoa/content_blocked_bubble_controller.mm:361: if
(contentSettingBubbleModel_->content_type()
On 2010/03/29 00:37:35, viettrungluu wrote:
> Ditto.

Done.

[Nico, 2010-03-29 01:32:32]

Oh, re top margin: I somewhat agree, but it matches the other bubbles and Cole
didn't complain about that in the bubble UI review. I wouldn't mind making it a
bit larger for all bubbles, but not in this CL.

[viettrungluu, 2010-03-29 01:46:05]

LGTM.

http://codereview.chromium.org/1542001/diff/2002/6003
File chrome/browser/cocoa/content_blocked_bubble_controller.mm (right):

http://codereview.chromium.org/1542001/diff/2002/6003#newcode69
chrome/browser/cocoa/content_blocked_bubble_controller.mm:69: float fontSize =
[NSFont systemFontSizeForControlSize:controlSize];
On 2010/03/29 01:28:40, Nico wrote:
> On 2010/03/29 00:37:35, viettrungluu wrote:
> > CGFloat
> 
> Done. (This is from
>
http://developer.apple.com/Mac/library/documentation/Cocoa/Conceptual/Control...
> , which uses float. But probably cause that page predates CGFloat.)

File a bug with Apple. :-P

http://codereview.chromium.org/1542001/diff/2002/6003#newcode78
chrome/browser/cocoa/content_blocked_bubble_controller.mm:78: NSTextField*
LabelWithFrame(NSString* text, const NSRect& frame) {
On 2010/03/29 01:28:40, Nico wrote:
> On 2010/03/29 00:37:35, viettrungluu wrote:
> > "LabelWithFrame" isn't a respectable C++ function name. Try "MakeLabel" or
> some
> > such.
> 
> But then it's not clear if the returned object is autoreleased or not. It's
> clearer this way imho.

Maybe. <shrug>

http://codereview.chromium.org/1542001/diff/2002/6003#newcode336
chrome/browser/cocoa/content_blocked_bubble_controller.mm:336: frame.origin.y =
NSMaxY(frame) + kGeoPadding;
On 2010/03/29 01:28:40, Nico wrote:
> On 2010/03/29 00:37:35, viettrungluu wrote:
> > The juxtaposition of this line with the previous is confusing. Hmm.
> 
> You think? How would you write it?

It's mainly confusing because it kind of looks like you're doing a pointless +=
on the previous line (which isn't actually the case. I might just write it as
one big line.

[Nico, 2010-03-29 01:58:11]

Submitted. Thanks again.

http://codereview.chromium.org/1542001/diff/2002/6003
File chrome/browser/cocoa/content_blocked_bubble_controller.mm (right):

http://codereview.chromium.org/1542001/diff/2002/6003#newcode336
chrome/browser/cocoa/content_blocked_bubble_controller.mm:336: frame.origin.y =
NSMaxY(frame) + kGeoPadding;
On 2010/03/29 01:46:05, viettrungluu wrote:
> On 2010/03/29 01:28:40, Nico wrote:
> > On 2010/03/29 00:37:35, viettrungluu wrote:
> > > The juxtaposition of this line with the previous is confusing. Hmm.
> > 
> > You think? How would you write it?
> 
> It's mainly confusing because it kind of looks like you're doing a pointless
+=
> on the previous line (which isn't actually the case. I might just write it as
> one big line.

Duh! Done. Much better, thanks.

[joth, 2010-03-29 16:22:11]

Thanks for implementing this.

One question: is it intentional that the host name is not indented and not
in bold, or would you do that as a follow up?
(see http://mocks/glen/chrome/spec/89_geo2/4/#07_trackingbubble.png or the
wave about it)

Cheers,
Joth



On 29 March 2010 02:58, <thakis@chromium.org> wrote:

> Submitted. Thanks again.
>
>
>
> http://codereview.chromium.org/1542001/diff/2002/6003
> File chrome/browser/cocoa/content_blocked_bubble_controller.mm (right):
>
> http://codereview.chromium.org/1542001/diff/2002/6003#newcode336
> chrome/browser/cocoa/content_blocked_bubble_controller.mm:336:
> frame.origin.y = NSMaxY(frame) + kGeoPadding;
> On 2010/03/29 01:46:05, viettrungluu wrote:
>
>> On 2010/03/29 01:28:40, Nico wrote:
>> > On 2010/03/29 00:37:35, viettrungluu wrote:
>> > > The juxtaposition of this line with the previous is confusing.
>>
> Hmm.
>
>> >
>> > You think? How would you write it?
>>
>
>  It's mainly confusing because it kind of looks like you're doing a
>>
> pointless +=
>
>> on the previous line (which isn't actually the case. I might just
>>
> write it as
>
>> one big line.
>>
>
> Duh! Done. Much better, thanks.
>
>
> http://codereview.chromium.org/1542001
>

[Nico, 2010-03-29 16:56:28]

I guess I can add that. Looked weird to me, and the popup bubble
doesn't do that either. I didn't realize it was in the mocks.

Nico

ps: Did you try showing the current form of the bubble to some
non-chromies? Is anyone able to make any sense of it? It took me quite
a while to understand what this bubble tries to tell me (I looked
through the source, what I guess most people won't do) :-/

On Mon, Mar 29, 2010 at 9:21 AM, Jonathan Dixon <joth@chromium.org> wrote:
> Thanks for implementing this.
> One question: is it intentional that the host name is not indented and not
> in bold, or would you do that as a follow up?
> (see http://mocks/glen/chrome/spec/89_geo2/4/#07_trackingbubble.png or the
> wave about it)
> Cheers,
> Joth
>
>
> On 29 March 2010 02:58, <thakis@chromium.org> wrote:
>>
>> Submitted. Thanks again.
>>
>>
>> http://codereview.chromium.org/1542001/diff/2002/6003
>> File chrome/browser/cocoa/content_blocked_bubble_controller.mm (right):
>>
>> http://codereview.chromium.org/1542001/diff/2002/6003#newcode336
>> chrome/browser/cocoa/content_blocked_bubble_controller.mm:336:
>> frame.origin.y = NSMaxY(frame) + kGeoPadding;
>> On 2010/03/29 01:46:05, viettrungluu wrote:
>>>
>>> On 2010/03/29 01:28:40, Nico wrote:
>>> > On 2010/03/29 00:37:35, viettrungluu wrote:
>>> > > The juxtaposition of this line with the previous is confusing.
>>
>> Hmm.
>>>
>>> >
>>> > You think? How would you write it?
>>
>>> It's mainly confusing because it kind of looks like you're doing a
>>
>> pointless +=
>>>
>>> on the previous line (which isn't actually the case. I might just
>>
>> write it as
>>>
>>> one big line.
>>
>> Duh! Done. Much better, thanks.
>>
>> http://codereview.chromium.org/1542001
>
>