[Mac] Enable "Edit Search Engines" in Omnibox context menu.

[Mac] Enable "Edit Search Engines" in Omnibox context menu.

Side effect of making the editor a singleton.

http://crbug.com/22512
http://crbug.com/22648
TEST=Right-click in Omnibox, "Edit Search Engines" should bring up the editor.
TEST=Right-click in a popu's Omnibox, no such option on menu.
TEST=Open prefs, select Basic/Manage, close prefs.  Repeat, should not get a second window.

[Scott Hess - ex-Googler, 2009-09-24 23:31:31]

Mike, I don't really like going through [NSApp delegate] to get the app
controller, but the preferences and about panel examples seem pretty close to
the right pattern.  I'm specifically refering to the browser_window_cocoa.mm
change.

[Scott Hess - ex-Googler, 2009-09-24 23:39:47]

On 2009/09/24 23:31:31, shess wrote:
> Mike, I don't really like going through [NSApp delegate] to get the app
> controller, but the preferences and about panel examples seem pretty close to
> the right pattern.  I'm specifically refering to the browser_window_cocoa.mm
> change.

A decent alternative appears to be:
  [NSApp sendAction:@selector(showSearchEngines:) to:nil from:nil];
then I can just wire up the Preferences button appropriately.  I'm not sure
about having -commandDispatch: forward to
BrowserWindowCocoa::ShowSearchEnginesDialog() and then back into the responder
chain.

[Robert Sesek, 2009-09-24 23:55:10]

http://codereview.chromium.org/220040/diff/1/3
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/220040/diff/1/3#newcode712
Line 712: return;
When would this happen? NOTREACHED()?

http://codereview.chromium.org/220040/diff/1/3#newcode719
Line 719: selector:@selector(prefsWindowClosed:)
|-searchEnginesWindowClosed:|?

http://codereview.chromium.org/220040/diff/1/7
File chrome/browser/cocoa/keyword_editor_cocoa_controller.h (right):

http://codereview.chromium.org/220040/diff/1/7#newcode68
Line 68: extern NSString* const kUserDoneEditingKeywordsNotification;
static?

http://codereview.chromium.org/220040/diff/1/8
File chrome/browser/cocoa/keyword_editor_cocoa_controller.mm (right):

http://codereview.chromium.org/220040/diff/1/8#newcode96
Line 96: // Window is closed, no point to continuing to observe.
More than that; we crash if we don't ;)

http://codereview.chromium.org/220040/diff/1/8#newcode98
Line 98: [[self retain] autorelease];  // Paranoid.
It'd be nice to explain why we do this (crbug#?).

[Scott Hess - ex-Googler, 2009-09-25 19:15:39]

Ready for another pass...

http://codereview.chromium.org/220040/diff/1/3
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/220040/diff/1/3#newcode712
Line 712: return;
On 2009/09/24 23:55:10, rsesek wrote:
> When would this happen? NOTREACHED()?

Never, but if/when it does, following the about-box pattern (which is "don't do
more stuff when things go wrong").  I'll drop in a comment and mark
NOTREACHED().

http://codereview.chromium.org/220040/diff/1/3#newcode719
Line 719: selector:@selector(prefsWindowClosed:)
On 2009/09/24 23:55:10, rsesek wrote:
> |-searchEnginesWindowClosed:|?

Done.  Darn it, I thought I had run through all the new code...

http://codereview.chromium.org/220040/diff/1/7
File chrome/browser/cocoa/keyword_editor_cocoa_controller.h (right):

http://codereview.chromium.org/220040/diff/1/7#newcode68
Line 68: extern NSString* const kUserDoneEditingKeywordsNotification;
On 2009/09/24 23:55:10, rsesek wrote:
> static?

No, it's extern so that app_controller_mac.mm can see it.

http://codereview.chromium.org/220040/diff/1/8
File chrome/browser/cocoa/keyword_editor_cocoa_controller.mm (right):

http://codereview.chromium.org/220040/diff/1/8#newcode98
Line 98: [[self retain] autorelease];  // Paranoid.
On 2009/09/24 23:55:10, rsesek wrote:
> It'd be nice to explain why we do this (crbug#?).

Done.

[Robert Sesek, 2009-09-26 15:23:28]

LGTM

http://codereview.chromium.org/220040/diff/4001/5005
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/220040/diff/4001/5005#newcode260
Line 260: #endif
I'll leave it to Mike to comment on which one of these is best, but this #if
shouldn't be in the final patch.

[pink (ping after 24hrs), 2009-09-28 18:21:07]

http://codereview.chromium.org/220040/diff/4001/5002
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/220040/diff/4001/5002#newcode697
Line 697: // Called when the search-engins window is closed. We use this to
"search-engines"

http://codereview.chromium.org/220040/diff/4001/5005
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/220040/diff/4001/5005#newcode259
Line 259: [NSApp sendAction:@selector(showSearchEngines:) to:nil from:nil];
how about broadcasting a message rather than calling a method on a specific
object? That frees the BWC from knowing who actually handles this.

[Scott Hess - ex-Googler, 2009-09-30 00:25:47]

Sorry for the delay.  Was thinking on the msg-to-nil versus NSNotification, and
looked at the Windows code.  The Windows code stores a global.  I think this
code is similar in spirit, concentrates the knowledge in one place, and the
compiler should provide some enforcement (you can't forget to catch the message,
which was something bugging me about the other solutions).

As a bonus, I got to spend some quality time understanding NSWindowController.

http://codereview.chromium.org/220040/diff/4001/5002
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/220040/diff/4001/5002#newcode697
Line 697: // Called when the search-engins window is closed. We use this to
On 2009/09/28 18:21:07, pink wrote:
> "search-engines"

What are they teachin young-uns these days?

http://codereview.chromium.org/220040/diff/4001/5005
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/220040/diff/4001/5005#newcode259
Line 259: [NSApp sendAction:@selector(showSearchEngines:) to:nil from:nil];
On 2009/09/28 18:21:07, pink wrote:
> how about broadcasting a message rather than calling a method on a specific
> object? That frees the BWC from knowing who actually handles this.

In the msg-to-nil case, it already doesn't know who's handling it.  Abstracting
this via NSNotification probably doesn't improve things (the notification's
constant name has to live somewhere, after all).

[Scott Hess - ex-Googler, 2009-09-30 21:21:26]

On 2009/09/30 00:25:47, shess wrote:
> As a bonus, I got to spend some quality time understanding NSWindowController.

The trybots complained about my change because there were 64 windows in the
window list when my test ran, which is a few more than I was really expecting. 
I've got some changes to cocoa_test_helper to clean that up, but that kind of
thing seems out-of-scope for this CL.

[Scott Hess - ex-Googler, 2009-10-02 18:39:52]

On 2009/09/30 21:21:26, shess wrote:
> On 2009/09/30 00:25:47, shess wrote:
> > As a bonus, I got to spend some quality time understanding
NSWindowController.
> 
> The trybots complained about my change because there were 64 windows in the
> window list when my test ran, which is a few more than I was really expecting.

> I've got some changes to cocoa_test_helper to clean that up, but that kind of
> thing seems out-of-scope for this CL.

Mike, any love on the +[KeywordEditorCocoaController showKeywordEditor:]
approach to handling a single global window?  Want to retire myself a bug.

Thanks,
scott

[pink (ping after 24hrs), 2009-10-05 16:01:19]

http://codereview.chromium.org/220040/diff/12002/11002
File chrome/browser/cocoa/autocomplete_text_field_editor.mm (right):

http://codereview.chromium.org/220040/diff/12002/11002#newcode111
Line 111: label = l10n_util::GetNSStringWithFixup(IDS_EDIT_SEARCH_ENGINES);
is it worth checking that |label| is not empty before adding the separator?

http://codereview.chromium.org/220040/diff/12002/11006
File chrome/browser/cocoa/keyword_editor_cocoa_controller.mm (right):

http://codereview.chromium.org/220040/diff/12002/11006#newcode59
Line 59: + (void)showKeywordEditor:(Profile*)profile {
You need to comment that this "leaks" the keyword controller and it will clean
itself up later.

http://codereview.chromium.org/220040/diff/12002/11006#newcode72
Line 72: for (NSWindow* window in [NSApp windows]) {
rather than scraping the window list, why not just have a static hash map from
profile to KeywordEditorController? That seems much cleaner and not reliant on
the concrete implementation classes.

http://codereview.chromium.org/220040/diff/12002/11007
File chrome/browser/cocoa/keyword_editor_cocoa_controller_unittest.mm (right):

http://codereview.chromium.org/220040/diff/12002/11007#newcode49
Line 49: NSUInteger c = 0;
if it's a count, call it |count|. Letters are inexpensive.

http://codereview.chromium.org/220040/diff/12002/11007#newcode78
Line 78: const NSUInteger initial_window_count_([[NSApp windows] count]);
why the trailing underscore? this is a local variable

http://codereview.chromium.org/220040/diff/12002/11007#newcode89
Line 89: EXPECT_EQ([controller_.get() retainCount], 2U);
this all seems really fragile. Are we sure it's necessary?

[Scott Hess - ex-Googler, 2009-10-05 20:50:45]

OK, so I hear +1 to having the class manage itself, but -1 to using the window
list as an implicit data structure.  Converted to a Singleton std::map, which is
probably overkill, but I seem to remember there was a reason to prefer Singleton
to a bare static for complex objects.

http://codereview.chromium.org/220040/diff/12002/11002
File chrome/browser/cocoa/autocomplete_text_field_editor.mm (right):

http://codereview.chromium.org/220040/diff/12002/11002#newcode111
Line 111: label = l10n_util::GetNSStringWithFixup(IDS_EDIT_SEARCH_ENGINES);
On 2009/10/05 16:01:19, pink wrote:
> is it worth checking that |label| is not empty before adding the separator?

Done.  Also added a DCHECK, as this shouldn't happen.

http://codereview.chromium.org/220040/diff/12002/11006
File chrome/browser/cocoa/keyword_editor_cocoa_controller.mm (right):

http://codereview.chromium.org/220040/diff/12002/11006#newcode59
Line 59: + (void)showKeywordEditor:(Profile*)profile {
On 2009/10/05 16:01:19, pink wrote:
> You need to comment that this "leaks" the keyword controller and it will clean
> itself up later.

Done.

http://codereview.chromium.org/220040/diff/12002/11006#newcode72
Line 72: for (NSWindow* window in [NSApp windows]) {
On 2009/10/05 16:01:19, pink wrote:
> rather than scraping the window list, why not just have a static hash map from
> profile to KeywordEditorController? That seems much cleaner and not reliant on
> the concrete implementation classes.

For some reason, I thought that rooting complex structures with globals was
frowned upon (because it made it hard to do artificial things, such as releasing
everything between unit tests).  Rewrote in terms of Singleton<>.

http://codereview.chromium.org/220040/diff/12002/11007
File chrome/browser/cocoa/keyword_editor_cocoa_controller_unittest.mm (right):

http://codereview.chromium.org/220040/diff/12002/11007#newcode49
Line 49: NSUInteger c = 0;
On 2009/10/05 16:01:19, pink wrote:
> if it's a count, call it |count|. Letters are inexpensive.

Done.  Don't blame me if we run out of bytes.

http://codereview.chromium.org/220040/diff/12002/11007#newcode78
Line 78: const NSUInteger initial_window_count_([[NSApp windows] count]);
On 2009/10/05 16:01:19, pink wrote:
> why the trailing underscore? this is a local variable

Done.  I don't know why - muscle memory or something?

http://codereview.chromium.org/220040/diff/12002/11007#newcode89
Line 89: EXPECT_EQ([controller_.get() retainCount], 2U);
On 2009/10/05 16:01:19, pink wrote:
> this all seems really fragile. Are we sure it's necessary?

The goal of this test is to verify that -close correctly gets rid of the window,
and that -windowWillClose: is doing the right thing.  The code is kinda weird
because controller_ is supposed to manage itself.

Also, it's kinda weird because I'm verifying that the window gets closed.  That
was more important when the code was using the window list to find existing
controllers.  Now it's less important, but I do notice that our unit tests leak
windows.  I've simplified things a bit, and left a TODO around the window leak
(which I expect to submit some CLs around later).

[pink (ping after 24hrs), 2009-10-06 13:39:49]

LGTM

http://codereview.chromium.org/220040/diff/14004/4018
File chrome/browser/cocoa/autocomplete_text_field_editor.mm (right):

http://codereview.chromium.org/220040/diff/14004/4018#newcode112
Line 112: if (label) {
is it also worth checking [label length]?