Mac: First run bubble.

Mac: First run bubble.

Add a BaseBubbleController. Move ContentBlockedBubbleController to use it (other bubbles will follow in later CLs), add FIrstRunBubbleController which uses it.

Move some l10n stuff to l10n_util and use that, too.

Update first run code to actually call the bubble code. Fix a double free while I'm at it (the scoped_ptr<> in DoFirstRun() already does the freeing, no need to do it in FirstRunDone())

BUG=27489, 36366
TEST=Firstrun bubble shows up when starting chrome with --first-run, doesn't if starting chrome without that flag. Content blocked bubbles still work.

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

[Nico, 2010-06-26 00:09:11]

Jeremy: Maybe you can take a quick look at the first_run_mac.mm change.

Miranda / Trung: Whoever gets to it first.

[viettrungluu, 2010-06-26 01:21:17]

Part 1

http://codereview.chromium.org/2822026/diff/18001/19002
File chrome/browser/cocoa/base_bubble_controller.h (right):

http://codereview.chromium.org/2822026/diff/18001/19002#newcode24
chrome/browser/cocoa/base_bubble_controller.h:24: @interface
BaseBubbleController : NSWindowController<NSWindowDelegate> {
Why "Base"?

http://codereview.chromium.org/2822026/diff/18001/19002#newcode51
chrome/browser/cocoa/base_bubble_controller.h:51: - (InfoBubbleView*)bubble;
Why not a @property?

http://codereview.chromium.org/2822026/diff/18001/19003
File chrome/browser/cocoa/base_bubble_controller.mm (right):

http://codereview.chromium.org/2822026/diff/18001/19003#newcode10
chrome/browser/cocoa/base_bubble_controller.mm:10: #include "base/string_util.h"
Some large number of these #includes are unnecessary.

[Nico, 2010-06-26 01:29:32]

Response 1!

http://codereview.chromium.org/2822026/diff/18001/19002
File chrome/browser/cocoa/base_bubble_controller.h (right):

http://codereview.chromium.org/2822026/diff/18001/19002#newcode24
chrome/browser/cocoa/base_bubble_controller.h:24: @interface
BaseBubbleController : NSWindowController<NSWindowDelegate> {
On 2010/06/26 01:21:17, viettrungluu wrote:
> Why "Base"?

Because it's a base class. Like BaseView. If you have a better name, I'm all
ears.

http://codereview.chromium.org/2822026/diff/18001/19002#newcode51
chrome/browser/cocoa/base_bubble_controller.h:51: - (InfoBubbleView*)bubble;
On 2010/06/26 01:21:17, viettrungluu wrote:
> Why not a @property?

*shrug*

Done

http://codereview.chromium.org/2822026/diff/18001/19003
File chrome/browser/cocoa/base_bubble_controller.mm (right):

http://codereview.chromium.org/2822026/diff/18001/19003#newcode10
chrome/browser/cocoa/base_bubble_controller.mm:10: #include "base/string_util.h"
On 2010/06/26 01:21:17, viettrungluu wrote:
> Some large number of these #includes are unnecessary.

Oh, right. In BlockedContentBubbleController too I guess. Done.

[Miranda Callahan, 2010-06-26 16:42:49]

Looks good, just a couple questions...

http://codereview.chromium.org/2822026/diff/1004/22003
File chrome/browser/cocoa/base_bubble_controller.mm (right):

http://codereview.chromium.org/2822026/diff/1004/22003#newcode59
chrome/browser/cocoa/base_bubble_controller.mm:59: [bubble_
setArrowLocation:info_bubble::kTopRight];
Will we need to think about having the arrow on the left in RTL languages?

http://codereview.chromium.org/2822026/diff/1004/22003#newcode88
chrome/browser/cocoa/base_bubble_controller.mm:88:
info_bubble::kBubbleArrowWidth / 2.0, 0);
Perhaps a different idea about indentation would be appropriate here.

http://codereview.chromium.org/2822026/diff/1004/22009
File chrome/browser/cocoa/first_run_bubble_controller.mm (right):

http://codereview.chromium.org/2822026/diff/1004/22009#newcode18
chrome/browser/cocoa/first_run_bubble_controller.mm:18: string16
GetDefaultSearchEngineName(Profile* profile) {
This function seems like it should be somewhere else, and common to all
operating systems.

http://codereview.chromium.org/2822026/diff/1004/22013
File chrome/browser/cocoa/location_bar/location_bar_view_mac.h (right):

http://codereview.chromium.org/2822026/diff/1004/22013#newcode480
chrome/browser/cocoa/location_bar/location_bar_view_mac.h:480: // Used schedule
a task for the first run info bubble.
s/Used/Used to/

[Nico, 2010-06-26 19:57:40]

http://codereview.chromium.org/2822026/diff/1004/22003
File chrome/browser/cocoa/base_bubble_controller.mm (right):

http://codereview.chromium.org/2822026/diff/1004/22003#newcode59
chrome/browser/cocoa/base_bubble_controller.mm:59: [bubble_
setArrowLocation:info_bubble::kTopRight];
On 2010/06/26 16:42:49, Miranda Callahan wrote:
> Will we need to think about having the arrow on the left in RTL languages?

I'm told that OS X has no support for RTL on 10.5 and bad support on 10.6.
Hence, we don't bother with it (yet?) anywhere.

http://codereview.chromium.org/2822026/diff/1004/22003#newcode88
chrome/browser/cocoa/base_bubble_controller.mm:88:
info_bubble::kBubbleArrowWidth / 2.0, 0);
On 2010/06/26 16:42:49, Miranda Callahan wrote:
> Perhaps a different idea about indentation would be appropriate here.

It's like this in e.g. extension_installed_bubble too. It kinda makes sense:
it's 4 spac^U

Done.

http://codereview.chromium.org/2822026/diff/1004/22009
File chrome/browser/cocoa/first_run_bubble_controller.mm (right):

http://codereview.chromium.org/2822026/diff/1004/22009#newcode18
chrome/browser/cocoa/first_run_bubble_controller.mm:18: string16
GetDefaultSearchEngineName(Profile* profile) {
On 2010/06/26 16:42:49, Miranda Callahan wrote:
> This function seems like it should be somewhere else, and common to all
> operating systems.

Hrm. Done?

http://codereview.chromium.org/2822026/diff/1004/22013
File chrome/browser/cocoa/location_bar/location_bar_view_mac.h (right):

http://codereview.chromium.org/2822026/diff/1004/22013#newcode480
chrome/browser/cocoa/location_bar/location_bar_view_mac.h:480: // Used schedule
a task for the first run info bubble.
On 2010/06/26 16:42:49, Miranda Callahan wrote:
> s/Used/Used to/

Done.

[Miranda Callahan, 2010-06-26 21:37:39]

LGTM!

[jeremy, 2010-06-27 12:46:42]

LGTM

http://codereview.chromium.org/2822026/diff/31001/7004
File chrome/browser/cocoa/base_bubble_controller.mm (right):

http://codereview.chromium.org/2822026/diff/31001/7004#newcode84
chrome/browser/cocoa/base_bubble_controller.mm:84: NSWindow* window = [self
window];  // completes nib load
nit: Completes nib load.

http://codereview.chromium.org/2822026/diff/31001/7004#newcode120
chrome/browser/cocoa/base_bubble_controller.mm:120: // undone. That's ok.
Could you expand this comment: which radio group?  What's the context here?