[Mac] Implement the search engine manager

[Mac] Implement the search engine manager

* Enable the Manage search engines button in Preferences
* New search engines can be added using the "+" button
* Existing search engines can be edited via double-click
* Existing search engines can be removed with the "-" button
* The "Make Default" button has the same behavior as the list box in the Basics
  preferences "Default Search"

BUG=16187
TEST=Go to Preferences, click "Manage" next to "Default Search" and add/edit/remove engines.

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

[Robert Sesek, 2009-09-09 14:42:59]

CC+pink because he is on the bug
CC+TVL for localization

[rohitrao (ping after 24h), 2009-09-09 16:50:09]

Mostly naming nits below, LGTM otherwise.

Didn't look at the xib files or anything related to l10n.

http://codereview.chromium.org/193040/diff/3001/4004
File chrome/browser/cocoa/edit_search_engine_controller_cocoa.h (right):

http://codereview.chromium.org/193040/diff/3001/4004#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
Not that it really matters, but all of the other files in this dir are named
*_cocoa_controller rather than *_controller_cocoa.

http://codereview.chromium.org/193040/diff/3001/4004#newcode29
Line 29: NSImage* goodImage_;
Can you put "  // strong" after each variable, since that's where I naturally
look for this info now?  I suspect that I would miss the above comment half the
time =)

http://codereview.chromium.org/193040/diff/3001/4005
File chrome/browser/cocoa/edit_search_engine_controller_cocoa.mm (right):

http://codereview.chromium.org/193040/diff/3001/4005#newcode47
Line 47: goodImage_ = [bundle.GetNSImageNamed(IDR_INPUT_GOOD) retain];
It's fine to put a retain in a reset() call.  I'm in favor of converting these
to scoped_nsobjects.

http://codereview.chromium.org/193040/diff/3001/4006
File chrome/browser/cocoa/edit_search_engine_controller_cocoa_unittest.mm
(right):

http://codereview.chromium.org/193040/diff/3001/4006#newcode52
Line 52: scoped_nsobject<FakeEditSearchEngineController> fixture_;
In GTest terms, EditSearchEngineControllerTest is our test "fixture."  It might
be worth renaming fixture_ to controller_ to avoid confusion.

http://codereview.chromium.org/193040/diff/3001/4006#newcode55
Line 55: BrowserTestHelper browser_helper_;
Does destruction order matter here?  Most of our other tests destroy the helpers
last (ie put them first in the member variable list).

http://codereview.chromium.org/193040/diff/3001/4009
File chrome/browser/cocoa/keyword_editor_controller_cocoa_unittest.mm (right):

http://codereview.chromium.org/193040/diff/3001/4009#newcode39
Line 39: scoped_nsobject<FakeKeywordEditorController> fixture_;
Same comments here about "fixture_" and destructor order.

[TVL, 2009-09-09 21:54:07]

lgtm

http://codereview.chromium.org/193040/diff/3001/4004
File chrome/browser/cocoa/edit_search_engine_controller_cocoa.h (right):

http://codereview.chromium.org/193040/diff/3001/4004#newcode19
Line 19: IBOutlet NSTextField* nameField_;
@private?

http://codereview.chromium.org/193040/diff/3001/4004#newcode29
Line 29: NSImage* goodImage_;
On 2009/09/09 16:50:09, rohitrao wrote:
> Can you put "  // strong" after each variable, since that's where I naturally
> look for this info now?  I suspect that I would miss the above comment half
the
> time =)

could also use the cpp helper to own and release these for you on dealloc

http://codereview.chromium.org/193040/diff/3001/4005
File chrome/browser/cocoa/edit_search_engine_controller_cocoa.mm (right):

http://codereview.chromium.org/193040/diff/3001/4005#newcode40
Line 40: [badImage_ release];
could drop if you use the cpp helper.

http://codereview.chromium.org/193040/diff/3001/4005#newcode51
Line 51: IDS_SEARCH_ENGINES_EDITOR_NEW_WINDOW_TITLE)];
if you wanted, you could put one of these as the the title in the xib, and then
simply replace the title if you are in the other case during awake from nib.

http://codereview.chromium.org/193040/diff/3001/4005#newcode108
Line 108: }
might be worth a helper that you pass the string id (or zero) and the two fields
to so you can remove the repeated code blocks.

http://codereview.chromium.org/193040/diff/3001/4007
File chrome/browser/cocoa/keyword_editor_controller_cocoa.h (right):

http://codereview.chromium.org/193040/diff/3001/4007#newcode44
Line 44: IBOutlet NSTableView* tableView_;
@private?

[Scott Hess - ex-Googler, 2009-09-10 19:25:37]

Sorry for the review delay.  Unfortunately, I've gotta get going, so I'll have
to add more later.

http://codereview.chromium.org/193040/diff/3001/4004
File chrome/browser/cocoa/edit_search_engine_controller_cocoa.h (right):

http://codereview.chromium.org/193040/diff/3001/4004#newcode29
Line 29: NSImage* goodImage_;
On 2009/09/09 21:54:07, TVL wrote:
> On 2009/09/09 16:50:09, rohitrao wrote:
> > Can you put "  // strong" after each variable, since that's where I
naturally
> > look for this info now?  I suspect that I would miss the above comment half
> the
> > time =)
> 
> could also use the cpp helper to own and release these for you on dealloc

scoped_nsobject<> ++.  Self-documenting, and does the right thing.

http://codereview.chromium.org/193040/diff/3001/4005
File chrome/browser/cocoa/edit_search_engine_controller_cocoa.mm (right):

http://codereview.chromium.org/193040/diff/3001/4005#newcode61
Line 61: [self validateFields];
What does it mean for a field to be invalid, here?  Is it possible you could
edit something which currently exists and find that you cannot immediately click
Save?  You could click Cancel, but it seems weird.

http://codereview.chromium.org/193040/diff/3001/4005#newcode108
Line 108: }
On 2009/09/09 21:54:07, TVL wrote:
> might be worth a helper that you pass the string id (or zero) and the two
fields
> to so you can remove the repeated code blocks.

Or expand or turn -setIsValid:forImageView: into it.

http://codereview.chromium.org/193040/diff/3001/4006
File chrome/browser/cocoa/edit_search_engine_controller_cocoa_unittest.mm
(right):

http://codereview.chromium.org/193040/diff/3001/4006#newcode62
Line 62: EXPECT_TRUE([fixture_ badImage]);
Suggest similar EXPECT_FALSE() before calling [fixture_ window].  Or, if your
goal is to have valid images wherever they come from, EXPECT_TRUE([[fixture_
goodImage] isKindOfClass:[NSImage class]]);.  That's probably more
straight-forward.

http://codereview.chromium.org/193040/diff/3001/4006#newcode73
Line 73: TEST_F(EditSearchEngineControllerTest, TestValidateFields) {
Might be nice to have a short comment for each stanza of test, so that if
someone later comes in and adds functionality they don't have to
reverse-engineer.

AFAICT, you're testing that things are invalid until you've set the name,
keyword, and url.  But your test doesn't verify that any missing field is
invalid, it only verifies that missing url is invalid (I think your test could
pass if -validateFields only checked the url).

http://codereview.chromium.org/193040/diff/3001/4007
File chrome/browser/cocoa/keyword_editor_controller_cocoa.h (right):

http://codereview.chromium.org/193040/diff/3001/4007#newcode27
Line 27: // edits. NOTE: |template_url| is the value supplied to
NOTE: is redundant, documenting your input is a reasonable expectation of a
function comment.

http://codereview.chromium.org/193040/diff/3001/4007#newcode49
Line 49: Profile* profile_;
// weak

http://codereview.chromium.org/193040/diff/3001/4008
File chrome/browser/cocoa/keyword_editor_controller_cocoa.mm (right):

http://codereview.chromium.org/193040/diff/3001/4008#newcode10
Line 10: #import "chrome/browser/cocoa/edit_search_engine_controller_cocoa.h"
alphabetize.

[Robert Sesek, 2009-09-10 20:52:03]

http://codereview.chromium.org/193040/diff/3001/4004
File chrome/browser/cocoa/edit_search_engine_controller_cocoa.h (right):

http://codereview.chromium.org/193040/diff/3001/4004#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
On 2009/09/09 16:50:09, rohitrao wrote:
> Not that it really matters, but all of the other files in this dir are named
> *_cocoa_controller rather than *_controller_cocoa.

I'll change this before I commit so I don't muck with the review.

http://codereview.chromium.org/193040/diff/3001/4004#newcode19
Line 19: IBOutlet NSTextField* nameField_;
On 2009/09/09 21:54:07, TVL wrote:
> @private?

All ivars are @protected by default, which we need for subclassing in the unit
test.

http://codereview.chromium.org/193040/diff/3001/4004#newcode29
Line 29: NSImage* goodImage_;
On 2009/09/09 21:54:07, TVL wrote:
> On 2009/09/09 16:50:09, rohitrao wrote:
> > Can you put "  // strong" after each variable, since that's where I
naturally
> > look for this info now?  I suspect that I would miss the above comment half
> the
> > time =)
> 
> could also use the cpp helper to own and release these for you on dealloc

I've switched these to be scopers and commented appropriately.

http://codereview.chromium.org/193040/diff/3001/4005
File chrome/browser/cocoa/edit_search_engine_controller_cocoa.mm (right):

http://codereview.chromium.org/193040/diff/3001/4005#newcode51
Line 51: IDS_SEARCH_ENGINES_EDITOR_NEW_WINDOW_TITLE)];
On 2009/09/09 21:54:07, TVL wrote:
> if you wanted, you could put one of these as the the title in the xib, and
then
> simply replace the title if you are in the other case during awake from nib.

Sure.

http://codereview.chromium.org/193040/diff/3001/4005#newcode61
Line 61: [self validateFields];
On 2009/09/10 19:25:38, shess wrote:
> What does it mean for a field to be invalid, here?  Is it possible you could
> edit something which currently exists and find that you cannot immediately
click
> Save?  You could click Cancel, but it seems weird.

When creating a new keyword, this will set images to the "alert" variant, set
appropriate tooltips, and disable the done button. When editing, this will set
all the images to the "good" variant. Commented about such.

http://codereview.chromium.org/193040/diff/3001/4005#newcode108
Line 108: }
On 2009/09/09 21:54:07, TVL wrote:
> might be worth a helper that you pass the string id (or zero) and the two
fields
> to so you can remove the repeated code blocks.

Agreed; much cleaner.

http://codereview.chromium.org/193040/diff/3001/4006
File chrome/browser/cocoa/edit_search_engine_controller_cocoa_unittest.mm
(right):

http://codereview.chromium.org/193040/diff/3001/4006#newcode52
Line 52: scoped_nsobject<FakeEditSearchEngineController> fixture_;
On 2009/09/09 16:50:09, rohitrao wrote:
> In GTest terms, EditSearchEngineControllerTest is our test "fixture."  It
might
> be worth renaming fixture_ to controller_ to avoid confusion.

Done.

http://codereview.chromium.org/193040/diff/3001/4006#newcode55
Line 55: BrowserTestHelper browser_helper_;
On 2009/09/09 16:50:09, rohitrao wrote:
> Does destruction order matter here?  Most of our other tests destroy the
helpers
> last (ie put them first in the member variable list).

It doesn't seem to, but I've made it match what we do elsewhere.

http://codereview.chromium.org/193040/diff/3001/4006#newcode62
Line 62: EXPECT_TRUE([fixture_ badImage]);
On 2009/09/10 19:25:38, shess wrote:
> Suggest similar EXPECT_FALSE() before calling [fixture_ window].  Or, if your
> goal is to have valid images wherever they come from, EXPECT_TRUE([[fixture_
> goodImage] isKindOfClass:[NSImage class]]);.  That's probably more
> straight-forward.
> 

Done.

http://codereview.chromium.org/193040/diff/3001/4006#newcode73
Line 73: TEST_F(EditSearchEngineControllerTest, TestValidateFields) {
On 2009/09/10 19:25:38, shess wrote:
> Might be nice to have a short comment for each stanza of test, so that if
> someone later comes in and adds functionality they don't have to
> reverse-engineer.
> 
> AFAICT, you're testing that things are invalid until you've set the name,
> keyword, and url.  But your test doesn't verify that any missing field is
> invalid, it only verifies that missing url is invalid (I think your test could
> pass if -validateFields only checked the url).

You're right. I've broken validation of each field into a separate test. I've
also added a test to validate creating a new keyword and editing an existing
one.

http://codereview.chromium.org/193040/diff/3001/4007
File chrome/browser/cocoa/keyword_editor_controller_cocoa.h (right):

http://codereview.chromium.org/193040/diff/3001/4007#newcode27
Line 27: // edits. NOTE: |template_url| is the value supplied to
On 2009/09/10 19:25:38, shess wrote:
> NOTE: is redundant, documenting your input is a reasonable expectation of a
> function comment.

The comment is copied from the definition of EditSearchEngineControllerDelegate.

[Scott Hess - ex-Googler, 2009-09-11 18:42:07]

I'm semi-concerned about your use of +alloc/-init then a separate
-makeKeyAndOrderFront:, with -autorelease on close.  I've gotten so used to
doing +alloc/-init/-autorelease to make sure that things are cleaned up in case
an exception interjects itself.  Don't know, I don't actually feel strongly
enough about it to ask for a change, just makes me slightly uncomfortable.

http://codereview.chromium.org/193040/diff/9002/2005
File chrome/browser/cocoa/edit_search_engine_controller_cocoa.h (right):

http://codereview.chromium.org/193040/diff/9002/2005#newcode30
Line 30: // ResourceBundle pak, but they are retained.
I don't think you need to document whether the scoper is the sole owner, or not,
unless that's somehow relevant.  Also, scoped_nsobject implies // STRONG, so
drop those comments.  Hmm, and who owns profile_ and templateURL_?

http://codereview.chromium.org/193040/diff/9002/2006
File chrome/browser/cocoa/edit_search_engine_controller_cocoa.mm (right):

http://codereview.chromium.org/193040/diff/9002/2006#newcode11
Line 11: #import "chrome/browser/cocoa/edit_search_engine_controller_cocoa.h"
This belongs at the top.  It should replace the #import Cocoa line (which is not
needed because this import implies it).

For instance, see bookmark_bar_bridge.mm.

http://codereview.chromium.org/193040/diff/9002/2006#newcode12
Line 12: #import "chrome/browser/search_engines/template_url_model.h"
#include on this one.

http://codereview.chromium.org/193040/diff/9002/2006#newcode21
Line 21: imageView:(NSImageView*)imageView;
Unless you're going to export these for testing, suggest just putting the
implementations about -validateFields and dropping the @interface.

http://codereview.chromium.org/193040/diff/9002/2006#newcode42
Line 42: - (void)awakeFromNib {
Since you're relying on window close cleaning things up, and the relevant
settings are in the nib file (and thus not obvious), suggest that you DCHECK
here that the window delegate is self, and the window is set to free-on-close. 
I think the unit tests would catch that leak, too, more I'm suggesting the
DCHECK to document the expectation.

http://codereview.chromium.org/193040/diff/9002/2006#newcode46
Line 46: if (templateURL_)  // Defaults to |..._NEW_WINDOW_TITLE|.
Fold the two if() clauses together.  Since you're changing the title based on
templateURL_, could you add unit tests that it took effect?  I think in
TestInvalidState and TestEditTemplateURL are probably the right places.

http://codereview.chromium.org/193040/diff/9002/2006#newcode97
Line 97: std::wstring title = base::SysNSStringToWide([nameField_ stringValue]);
I wonder if we should someday add a category to provide -[NSControl
wstringValue] :-).

http://codereview.chromium.org/193040/diff/9002/2006#newcode101
Line 101: onTextField:nameField_ imageView:nameImage_];
I think the style guidelines call for this to be four lines with the ':'
aligned.  Same below.

http://codereview.chromium.org/193040/diff/9002/2006#newcode121
Line 121: - (void)setIsValid:(BOOL)valid forImageView:(NSImageView*)imageView {
Any reason not to fold this into -setValid:toolTip:onTextField:imageView:?

http://codereview.chromium.org/193040/diff/9002/2006#newcode129
Line 129: onTextField:(NSTextField*)textField
onTextField: rings oddly, suggest textField:

http://codereview.chromium.org/193040/diff/9002/2007
File chrome/browser/cocoa/edit_search_engine_controller_cocoa_unittest.mm
(right):

http://codereview.chromium.org/193040/diff/9002/2007#newcode44
Line 44: 
Can you put everything from here to the end of file in an anonymous namespace? 
Can't put the Objective-C stuff above in there, unfortunately.

http://codereview.chromium.org/193040/diff/9002/2007#newcode62
Line 62: TEST_F(EditSearchEngineControllerTest, TestValidImageOriginals) {
Drop the Test from the test name here and below, it's redundant (the full name
of the test is EditSearchEngineControllerTest.TestValidImageOriginals).

http://codereview.chromium.org/193040/diff/9002/2007#newcode66
Line 66: [controller_ window];  // Call |-awakeFromNib|.
This one's comment differs from the other calls.

So long as we're in here ... maybe EXPECT_TRUE([controller_ window]); all
around?

http://codereview.chromium.org/193040/diff/9002/2007#newcode200
Line 200: EXPECT_TRUE([@"Foobar" isEqualToString:[[controller_ nameField]
stringValue]]);
This line is wrapping for me.  Suggest that for this EXPECT and the next two, do
something like:
  NSString* nameString = [[c_ nameField] stringValue];
  EXPECT_TRUE([@"Foobar" isEqualToString:nameString]);
or something like that.  Once you've already broken the line, might as well
rewrite as two clear lines.

http://codereview.chromium.org/193040/diff/9002/2008
File chrome/browser/cocoa/keyword_editor_controller_cocoa.h (right):

http://codereview.chromium.org/193040/diff/9002/2008#newcode16
Line 16: // Very thin bridge that simply pushes notifications from C++ to ObjC.
Can you push this into an anonymous namespace w/in the .mm file?  The only place
outside this class where you reference it is in the unit test, and there you can
reference it as a TemplateURLModelObserver, I think.

If you can do this, then naturally just make it all inline.

http://codereview.chromium.org/193040/diff/9002/2008#newcode20
Line 20: KeywordEditorModelObserver(KeywordEditorControllerCocoa* controller);
explicit keyword here.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Explicit_Const...

http://codereview.chromium.org/193040/diff/9002/2008#newcode34
Line 34: const std::wstring& url);
Line of whitespace between OnEditedKeyword() and private:

http://codereview.chromium.org/193040/diff/9002/2008#newcode36
Line 36: KeywordEditorControllerCocoa* controller_;
  DISALLOW_COPY_AND_ASSIGN(KeywordEditorModelObserver);

http://codereview.chromium.org/193040/diff/9002/2008#newcode60
Line 60: - (KeywordEditorController*)controller;
@property (readonly) KeywordEditorController* controller;
and drop the comment.

http://codereview.chromium.org/193040/diff/9002/2009
File chrome/browser/cocoa/keyword_editor_controller_cocoa.mm (right):

http://codereview.chromium.org/193040/diff/9002/2009#newcode24
Line 24: controller_ = controller;
    : controller_(controller) unless there's a reason to do the assignment w/in
the constructor proper.

http://codereview.chromium.org/193040/diff/9002/2009#newcode100
Line 100: NSInteger clickedRow = [tableView_ clickedRow];
const would be good practice.  I haven't called all of those out, but it's a
good habit to develop.

http://codereview.chromium.org/193040/diff/9002/2009#newcode114
Line 114: DCHECK([selection count] > 0);
DCHECK_GT will give better diagnostics.

http://codereview.chromium.org/193040/diff/9002/2009#newcode119
Line 119: }
Would be a great place for -enumeratIndexesUsingBlock:!  If only :-).

http://codereview.chromium.org/193040/diff/9002/2009#newcode124
Line 124: DCHECK([selection count] == 1);
DCHECK_EQ.

http://codereview.chromium.org/193040/diff/9002/2009#newcode138
Line 138: int column = -1;
I think column_id would be better, here.  Tells me that the value is a bit
opaque.

http://codereview.chromium.org/193040/diff/9002/2009#newcode144
Line 144: DCHECK(column != -1);
DCHECK_NE.

http://codereview.chromium.org/193040/diff/9002/2009#newcode159
Line 159: static CGFloat kCellFontSize = 12.0;
No, not static, static const.

http://codereview.chromium.org/193040/diff/9002/2009#newcode190
Line 190: selection = [tableView_ selectedRowIndexes];
Why another call to this?

http://codereview.chromium.org/193040/diff/9002/2010
File chrome/browser/cocoa/keyword_editor_controller_cocoa_unittest.mm (right):

http://codereview.chromium.org/193040/diff/9002/2010#newcode29
Line 29: 
Everything from here in an anonymous namespace.

http://codereview.chromium.org/193040/diff/9002/2010#newcode36
Line 36: profile_]);
Can you break this between ( and [?  I don't know what the right style would be,
otherwise, perhaps "init" lined up with second [.

http://codereview.chromium.org/193040/diff/9002/2010#newcode39
Line 39: TestingProfile* profile_;
Do you need profile_?

http://codereview.chromium.org/193040/diff/9002/2010#newcode50
Line 50: }
That's it?  I don't know how hard it is to work KeywordEditorController into a
unit test, but it would be nice to see that the table view gets setup like
expected, and that the right selected items get deleted, etc.  If that's super
hard to do right now, at least drop in a TODOD.

[Robert Sesek, 2009-09-11 21:58:11]

> I'm semi-concerned about your use of +alloc/-init then a separate
> -makeKeyAndOrderFront:, with -autorelease on close.  I've gotten so used to
> doing +alloc/-init/-autorelease to make sure that things are cleaned up in
case
> an exception interjects itself.  Don't know, I don't actually feel strongly
> enough about it to ask for a change, just makes me slightly uncomfortable.

We cannot |-autorelease| right after we create the window because it's not
modal. As soon as we create it, it will then be cleaned up and will disappear.
We have done this elsewhere (c.f. page info window), so I think we're OK.

http://codereview.chromium.org/193040/diff/9002/2006
File chrome/browser/cocoa/edit_search_engine_controller_cocoa.mm (right):

http://codereview.chromium.org/193040/diff/9002/2006#newcode97
Line 97: std::wstring title = base::SysNSStringToWide([nameField_ stringValue]);
On 2009/09/11 18:42:07, shess wrote:
> I wonder if we should someday add a category to provide -[NSControl
> wstringValue] :-).

Would be quite handy, indeed.

http://codereview.chromium.org/193040/diff/9002/2008
File chrome/browser/cocoa/keyword_editor_controller_cocoa.h (right):

http://codereview.chromium.org/193040/diff/9002/2008#newcode16
Line 16: // Very thin bridge that simply pushes notifications from C++ to ObjC.
On 2009/09/11 18:42:07, shess wrote:
> Can you push this into an anonymous namespace w/in the .mm file?  The only
place
> outside this class where you reference it is in the unit test, and there you
can
> reference it as a TemplateURLModelObserver, I think.
> 
> If you can do this, then naturally just make it all inline.

Per IRC, this generates a warning we've decided not to do this:

 warning: 'KeywordEditorControllerCocoa' has a field
'KeywordEditorControllerCocoa::observer_' whose type uses the anonymous
namespace

http://codereview.chromium.org/193040/diff/9002/2009
File chrome/browser/cocoa/keyword_editor_controller_cocoa.mm (right):

http://codereview.chromium.org/193040/diff/9002/2009#newcode114
Line 114: DCHECK([selection count] > 0);
On 2009/09/11 18:42:07, shess wrote:
> DCHECK_GT will give better diagnostics.

Done here and elsewhere, but because |-count| returns an NSUInteger, we have to
cast to int so we don't get warnings...

http://codereview.chromium.org/193040/diff/9002/2010
File chrome/browser/cocoa/keyword_editor_controller_cocoa_unittest.mm (right):

http://codereview.chromium.org/193040/diff/9002/2010#newcode50
Line 50: }
On 2009/09/11 18:42:07, shess wrote:
> That's it?  I don't know how hard it is to work KeywordEditorController into a
> unit test, but it would be nice to see that the table view gets setup like
> expected, and that the right selected items get deleted, etc.  If that's super
> hard to do right now, at least drop in a TODOD.

I've put in a TODO. If that's okay for now, I'll also file a bug and link it
there.

[Scott Hess - ex-Googler, 2009-09-11 22:24:29]

LGTM, with my comments applied.

MUST double-check chrome.gyp before checking in!  I don't know what you've got
going on, I suspect an automerge-gone-awry.

Also, TVL (or Rohit) had a naming comment to deal with before check-in.

http://codereview.chromium.org/193040/diff/9002/2009
File chrome/browser/cocoa/keyword_editor_controller_cocoa.mm (right):

http://codereview.chromium.org/193040/diff/9002/2009#newcode114
Line 114: DCHECK([selection count] > 0);
On 2009/09/11 21:58:11, rsesek wrote:
> On 2009/09/11 18:42:07, shess wrote:
> > DCHECK_GT will give better diagnostics.
> 
> Done here and elsewhere, but because |-count| returns an NSUInteger, we have
to
> cast to int so we don't get warnings...

That's fine.  Somewhere I have a little bit of code exploring adding appropriate
magic to make some things like this work nicer on Mac (like DCHECK_OBJCEQ
printing out the descriptions when the objects !=).  Anyhow, getting log output
with the specific values (which DCHECK_GT does) generally way outweighs the
minor cost of adding a cast here or there.

http://codereview.chromium.org/193040/diff/14/1024#newcode113
Line 113: NSIndexSet* selection = [tableView_ selectedRowIndexes];
Unfortunately, (int) is frowned on.  Has to be static_cast<int>(...).  So
lovely!

Fortunately, a better solution is:
  DCHECK_GT([selection count], 0U);

http://codereview.chromium.org/193040/diff/14/1024#newcode123
Line 123: NSIndexSet* selection = [tableView_ selectedRowIndexes];
Here too.

http://codereview.chromium.org/193040/diff/14/1028
File chrome/chrome.gyp (right):

http://codereview.chromium.org/193040/diff/14/1028#newcode78
Line 78: 'browser/extensions/extension_i18n_apitest.cc',
WARNING!  You've got something wrong going on with a merge somewhere!