Finish the gtk search engine manager.

Finish the gtk search engine manager.

BUG=13326
TEST=Open options, click search engines manage button, try adding, removing, making default, etc.

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

[Evan Stade, 2009-06-26 23:07:55]

mostly nits

http://codereview.chromium.org/147243/diff/1002/6
File chrome/browser/gtk/keyword_editor_view.cc (right):

http://codereview.chromium.org/147243/diff/1002/6#newcode86
Line 86: DCHECK(controller_->url_model());
this DCHECK isn't very useful it seems, as you will still crash in release.

http://codereview.chromium.org/147243/diff/1002/6#newcode306
Line 306: CHECK(groups.size() == 2);
should probably do if-NOTREACHED-return

http://codereview.chromium.org/147243/diff/1002/6#newcode310
Line 310: gtk_list_store_set(
can you comment these rows

http://codereview.chromium.org/147243/diff/1002/6#newcode442
Line 442: editor->OnEditButtonClicked(NULL, editor);
nit: OnEditButtonClicked(NULL, editor);

http://codereview.chromium.org/147243/diff/1002/7
File chrome/browser/gtk/keyword_editor_view.h (right):

http://codereview.chromium.org/147243/diff/1002/7#newcode84
Line 84: // Callback checking whether a row may be selected.
Maybe explain why certain rows can't be selected?

http://codereview.chromium.org/147243/diff/1002/7#newcode134
Line 134: int model_second_group_index;
trailing underscore

[mattm, 2009-06-26 23:20:03]

http://codereview.chromium.org/147243/diff/1002/6
File chrome/browser/gtk/keyword_editor_view.cc (right):

http://codereview.chromium.org/147243/diff/1002/6#newcode86
Line 86: DCHECK(controller_->url_model());
On 2009/06/26 23:07:56, Evan Stade wrote:
> this DCHECK isn't very useful it seems, as you will still crash in release.

Done.

http://codereview.chromium.org/147243/diff/1002/6#newcode306
Line 306: CHECK(groups.size() == 2);
On 2009/06/26 23:07:56, Evan Stade wrote:
> should probably do if-NOTREACHED-return

Done.

http://codereview.chromium.org/147243/diff/1002/6#newcode310
Line 310: gtk_list_store_set(
On 2009/06/26 23:07:56, Evan Stade wrote:
> can you comment these rows

Done.

http://codereview.chromium.org/147243/diff/1002/6#newcode442
Line 442: editor->OnEditButtonClicked(NULL, editor);
On 2009/06/26 23:07:56, Evan Stade wrote:
> nit: OnEditButtonClicked(NULL, editor);

Done.

http://codereview.chromium.org/147243/diff/1002/7
File chrome/browser/gtk/keyword_editor_view.h (right):

http://codereview.chromium.org/147243/diff/1002/7#newcode84
Line 84: // Callback checking whether a row may be selected.
On 2009/06/26 23:07:56, Evan Stade wrote:
> Maybe explain why certain rows can't be selected?

Done.

http://codereview.chromium.org/147243/diff/1002/7#newcode134
Line 134: int model_second_group_index;
On 2009/06/26 23:07:56, Evan Stade wrote:
> trailing underscore

Done.

[Evan Stade, 2009-06-26 23:24:22]

lgtm

http://codereview.chromium.org/147243/diff/1009/1010
File chrome/browser/gtk/keyword_editor_view.cc (right):

http://codereview.chromium.org/147243/diff/1009/1010#newcode326
Line 326: // Add the headers for the second group.
I was thinking more along the lines of

// First group title.
...
// First group separator.
...
// Blank row.
...