Modify extension omnibox keywords to be user-configurable

Modify extension omnibox keywords to be user-configurable
Allows users to change extension omnibox keywords through the chrome://settings/searchEngines page, and prevents extensions from registering the same keywords.

Images demonstrating UI change (new on top, old on bottom):
http://imgur.com/a/SDTvD

BUG=74643
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203855

[Aaron Jacobs, 2013-05-23 03:38:35]

Images demonstrating UI change (new on top, old on bottom):
http://imgur.com/a/SDTvD

[Devlin, 2013-05-24 02:42:54]

A test for this would also be cool.

https://codereview.chromium.org/15805002/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/15805002/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_service.cc:2126:
existing_keyword_turl->short_name(), new_keyword,
indentation

https://codereview.chromium.org/15805002/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_service.cc:2263: // for extensions,
as their keywords are not assocaited with their URLs).
*associated

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/search_engi...
File chrome/browser/ui/search_engines/template_url_table_model.cc (right):

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/search_engi...
chrome/browser/ui/search_engines/template_url_table_model.cc:175: i !=
urls.end(); ++i) {
This code is now duplicated in three places... maybe time for a function?

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/search_engi...
chrome/browser/ui/search_engines/template_url_table_model.cc:269:
last_other_engine_index_--;
nit: prefer pre-decrement unless there's a good reason (please also update the
one above).

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/search_engi...
File chrome/browser/ui/search_engines/template_url_table_model.h (right):

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/search_engi...
chrome/browser/ui/search_engines/template_url_table_model.h:100: // Returns the
index of the last entry shown in the other engines group.
maybe make this say "... in the 'other search engines' group"?  "in the other
engines group" sounds a bit odd to me.

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
File chrome/browser/ui/webui/options/search_engine_manager_handler.cc (right):

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:129:
defaults_list.Append(CreateDictionaryForEngine(i, i == default_index,
each param on own line, or all three on newline.

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:130: false));
please document the anonymous boolean (it's not too bad, since it's local to the
file, but is still good practice).

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:140:
others_list.Append(CreateDictionaryForEngine(i, i == default_index,
see 129.

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:151: true));
see 129

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
File chrome/browser/ui/webui/options/search_engine_manager_handler.h (right):

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/search_engine_manager_handler.h:76: bool
is_extension);
each parameter on own line, or all three on a newline.

[Aaron Jacobs, 2013-05-24 03:06:13]

https://codereview.chromium.org/15805002/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/15805002/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_service.cc:2126:
existing_keyword_turl->short_name(), new_keyword,
On 2013/05/24 02:42:54, D Cronin wrote:
> indentation

Done.

https://codereview.chromium.org/15805002/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_service.cc:2263: // for extensions,
as their keywords are not assocaited with their URLs).
On 2013/05/24 02:42:54, D Cronin wrote:
> *associated

Done.

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/search_engi...
File chrome/browser/ui/search_engines/template_url_table_model.cc (right):

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/search_engi...
chrome/browser/ui/search_engines/template_url_table_model.cc:175: i !=
urls.end(); ++i) {
On 2013/05/24 02:42:54, D Cronin wrote:
> This code is now duplicated in three places... maybe time for a function?

The condition within the loop is different for each version, would you still
like me to create a function?

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/search_engi...
chrome/browser/ui/search_engines/template_url_table_model.cc:269:
last_other_engine_index_--;
On 2013/05/24 02:42:54, D Cronin wrote:
> nit: prefer pre-decrement unless there's a good reason (please also update the
> one above).

Done.

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/search_engi...
File chrome/browser/ui/search_engines/template_url_table_model.h (right):

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/search_engi...
chrome/browser/ui/search_engines/template_url_table_model.h:100: // Returns the
index of the last entry shown in the other engines group.
On 2013/05/24 02:42:54, D Cronin wrote:
> maybe make this say "... in the 'other search engines' group"?  "in the other
> engines group" sounds a bit odd to me.

Done.

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
File chrome/browser/ui/webui/options/search_engine_manager_handler.cc (right):

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:129:
defaults_list.Append(CreateDictionaryForEngine(i, i == default_index,
On 2013/05/24 02:42:54, D Cronin wrote:
> each param on own line, or all three on newline.

Done.

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:130: false));
On 2013/05/24 02:42:54, D Cronin wrote:
> please document the anonymous boolean (it's not too bad, since it's local to
the
> file, but is still good practice).

Done.

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:140:
others_list.Append(CreateDictionaryForEngine(i, i == default_index,
On 2013/05/24 02:42:54, D Cronin wrote:
> see 129.

Done.

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:151: true));
On 2013/05/24 02:42:54, D Cronin wrote:
> see 129

Done.

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
File chrome/browser/ui/webui/options/search_engine_manager_handler.h (right):

https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/search_engine_manager_handler.h:76: bool
is_extension);
On 2013/05/24 02:42:54, D Cronin wrote:
> each parameter on own line, or all three on a newline.

Done.

[Devlin, 2013-05-28 21:20:18]

On 2013/05/24 03:06:13, Aaron Jacobs wrote:
>
https://codereview.chromium.org/15805002/diff/1/chrome/browser/search_engines...
> File chrome/browser/search_engines/template_url_service.cc (right):
> 
>
https://codereview.chromium.org/15805002/diff/1/chrome/browser/search_engines...
> chrome/browser/search_engines/template_url_service.cc:2126:
> existing_keyword_turl->short_name(), new_keyword,
> On 2013/05/24 02:42:54, D Cronin wrote:
> > indentation
> 
> Done.
> 
>
https://codereview.chromium.org/15805002/diff/1/chrome/browser/search_engines...
> chrome/browser/search_engines/template_url_service.cc:2263: // for extensions,
> as their keywords are not assocaited with their URLs).
> On 2013/05/24 02:42:54, D Cronin wrote:
> > *associated
> 
> Done.
> 
>
https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/search_engi...
> File chrome/browser/ui/search_engines/template_url_table_model.cc (right):
> 
>
https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/search_engi...
> chrome/browser/ui/search_engines/template_url_table_model.cc:175: i !=
> urls.end(); ++i) {
> On 2013/05/24 02:42:54, D Cronin wrote:
> > This code is now duplicated in three places... maybe time for a function?
> 
> The condition within the loop is different for each version, would you still
> like me to create a function?
> 
>
https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/search_engi...
> chrome/browser/ui/search_engines/template_url_table_model.cc:269:
> last_other_engine_index_--;
> On 2013/05/24 02:42:54, D Cronin wrote:
> > nit: prefer pre-decrement unless there's a good reason (please also update
the
> > one above).
> 
> Done.
> 
>
https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/search_engi...
> File chrome/browser/ui/search_engines/template_url_table_model.h (right):
> 
>
https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/search_engi...
> chrome/browser/ui/search_engines/template_url_table_model.h:100: // Returns
the
> index of the last entry shown in the other engines group.
> On 2013/05/24 02:42:54, D Cronin wrote:
> > maybe make this say "... in the 'other search engines' group"?  "in the
other
> > engines group" sounds a bit odd to me.
> 
> Done.
> 
>
https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
> File chrome/browser/ui/webui/options/search_engine_manager_handler.cc (right):
> 
>
https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
> chrome/browser/ui/webui/options/search_engine_manager_handler.cc:129:
> defaults_list.Append(CreateDictionaryForEngine(i, i == default_index,
> On 2013/05/24 02:42:54, D Cronin wrote:
> > each param on own line, or all three on newline.
> 
> Done.
> 
>
https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
> chrome/browser/ui/webui/options/search_engine_manager_handler.cc:130: false));
> On 2013/05/24 02:42:54, D Cronin wrote:
> > please document the anonymous boolean (it's not too bad, since it's local to
> the
> > file, but is still good practice).
> 
> Done.
> 
>
https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
> chrome/browser/ui/webui/options/search_engine_manager_handler.cc:140:
> others_list.Append(CreateDictionaryForEngine(i, i == default_index,
> On 2013/05/24 02:42:54, D Cronin wrote:
> > see 129.
> 
> Done.
> 
>
https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
> chrome/browser/ui/webui/options/search_engine_manager_handler.cc:151: true));
> On 2013/05/24 02:42:54, D Cronin wrote:
> > see 129
> 
> Done.
> 
>
https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
> File chrome/browser/ui/webui/options/search_engine_manager_handler.h (right):
> 
>
https://codereview.chromium.org/15805002/diff/1/chrome/browser/ui/webui/optio...
> chrome/browser/ui/webui/options/search_engine_manager_handler.h:76: bool
> is_extension);
> On 2013/05/24 02:42:54, D Cronin wrote:
> > each parameter on own line, or all three on a newline.
> 
> Done.

Still waiting on that test?

[Aaron Jacobs, 2013-05-29 20:42:48]

Dan:
Owners for chrome/browser/resources/options/*, chrome/browser/ui/webui/options/*

Peter:
Owners for chrome/browser/search_engines/*, chrome/browser/ui/search_engines/*

[Peter Kasting, 2013-05-30 21:22:50]

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.cc:2112: } else {
Nit: We shouldn't have else after return.

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.cc:2115: // the new
TemplateURL the newly generated keyword.
I'm kind of unclear on why we don't just always do this.  Why is it better to
re-keyword the existing URL in non-extension cases?

(It's been at least a year since I thought about most of this code.)

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/ui/search_e...
File chrome/browser/ui/search_engines/template_url_table_model.cc (right):

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/ui/search_e...
chrome/browser/ui/search_engines/template_url_table_model.cc:147: // Keywords
that can be made the default first.
Nit: Since you have the same block of code three times, what about something
like:

...
for (...) {
  ...
  if (template_url->show_in_default_list())
    default_entries.push_back(...);
  else if (template_url->IsExtensionKeyword())
    extension_entries.push_back(...);
  else
    other_entries.push_back(...);
}

Then you can construct |entries_| by just appending the vectors together, and
their sizes give you the indexes you want.

(You could also elect to replace |entries_| with three separate vectors like
this and do operations on them directly, but I don't know whether that'd be a
win.)

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/ui/search_e...
chrome/browser/ui/search_engines/template_url_table_model.cc:253: if (row <
last_other_engine_index_)
Nit: You could use ?: on this last pair

[Dan Beam, 2013-05-30 22:13:57]

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/resources/o...
File chrome/browser/resources/options/search_engine_manager.js (right):

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/resources/o...
chrome/browser/resources/options/search_engine_manager.js:121:
SearchEngineManager.getInstance().extensionList_.validationComplete(
uh... why are we call through to private properties?

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/search_engine_manager_handler.cc (right):

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:197:
dict->SetString("isExtension", "1");
dict->SetBoolean

[Aaron Jacobs, 2013-05-31 03:07:23]

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/resources/o...
File chrome/browser/resources/options/search_engine_manager.js (right):

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/resources/o...
chrome/browser/resources/options/search_engine_manager.js:121:
SearchEngineManager.getInstance().extensionList_.validationComplete(
On 2013/05/30 22:13:57, Dan Beam wrote:
> uh... why are we call through to private properties?

Done.

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.cc:2112: } else {
On 2013/05/30 21:22:50, Peter Kasting wrote:
> Nit: We shouldn't have else after return.

Done.

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.cc:2115: // the new
TemplateURL the newly generated keyword.
On 2013/05/30 21:22:50, Peter Kasting wrote:
> I'm kind of unclear on why we don't just always do this.  Why is it better to
> re-keyword the existing URL in non-extension cases?
> 
> (It's been at least a year since I thought about most of this code.)

I was wondering about this - unfortunately, I don't know the reason.

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/ui/search_e...
File chrome/browser/ui/search_engines/template_url_table_model.cc (right):

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/ui/search_e...
chrome/browser/ui/search_engines/template_url_table_model.cc:147: // Keywords
that can be made the default first.
On 2013/05/30 21:22:50, Peter Kasting wrote:
> Nit: Since you have the same block of code three times, what about something
> like:
> 
> ...
> for (...) {
>   ...
>   if (template_url->show_in_default_list())
>     default_entries.push_back(...);
>   else if (template_url->IsExtensionKeyword())
>     extension_entries.push_back(...);
>   else
>     other_entries.push_back(...);
> }
> 
> Then you can construct |entries_| by just appending the vectors together, and
> their sizes give you the indexes you want.
> 
> (You could also elect to replace |entries_| with three separate vectors like
> this and do operations on them directly, but I don't know whether that'd be a
> win.)

Done.

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/ui/search_e...
chrome/browser/ui/search_engines/template_url_table_model.cc:253: if (row <
last_other_engine_index_)
On 2013/05/30 21:22:50, Peter Kasting wrote:
> Nit: You could use ?: on this last pair

Done.

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/search_engine_manager_handler.cc (right):

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:197:
dict->SetString("isExtension", "1");
On 2013/05/30 22:13:57, Dan Beam wrote:
> dict->SetBoolean

Done.
Also fixed those just above.

https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/search_...
File chrome/browser/ui/search_engines/keyword_editor_controller.cc (right):

https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/search_...
chrome/browser/ui/search_engines/keyword_editor_controller.cc:42: const int
new_index = table_model_->last_other_engine_index();
I'm not sure if the mentioned bug was officially closed, as I can not find it
either on codereview.chromium.org or crbug.com, but I need to change the value,
and it seems to be working.

[Dan Beam, 2013-05-31 03:33:56]

lgtm

https://codereview.chromium.org/15805002/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/options/search_engine_manager.js (right):

https://codereview.chromium.org/15805002/diff/21001/chrome/browser/resources/...
chrome/browser/resources/options/search_engine_manager.js:29: defaultsList_:
null,
I didn't see that it was in the same class, sorry about that... you can leave it
as it was if you'd like.

If you like this way, move each ES5 getter under the property itself, i.e.

  defaultsList_: null,
  get defaultsList() {
    return this.defaultsList_;
  },

sorry for the mix up and thanks for the work!

https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/search_engine_manager_handler.cc (right):

https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:193: 
nit: no \n

https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:194:
dict->SetBoolean("isExtension", is_extension);
awesome, thanks

[Peter Kasting, 2013-06-01 01:38:03]

LGTM with one substantive question.

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/15805002/diff/9001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.cc:2115: // the new
TemplateURL the newly generated keyword.
On 2013/05/31 03:07:24, Aaron Jacobs wrote:
> On 2013/05/30 21:22:50, Peter Kasting wrote:
> > I'm kind of unclear on why we don't just always do this.  Why is it better
to
> > re-keyword the existing URL in non-extension cases?
> > 
> > (It's been at least a year since I thought about most of this code.)
> 
> I was wondering about this - unfortunately, I don't know the reason.

OK, after staring at this for a while, now I'm wondering the opposite: Why do we
want to give the new TURL the new keyword if the existing one is from an
extension?  I believe the old code was the way it was because I believed that,
in this situation, a newer TURL was more reflective of "what the user wants as
of now" and thus more important to preserve as-is.  Not totally sure though.

Honestly I'm not even sure this case can ever happen.  I thought we protected
against this.  Maybe this is reachable via sync?

https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/search_...
File chrome/browser/ui/search_engines/keyword_editor_controller.cc (right):

https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/search_...
chrome/browser/ui/search_engines/keyword_editor_controller.cc:42: const int
new_index = table_model_->last_other_engine_index();
On 2013/05/31 03:07:24, Aaron Jacobs wrote:
> I'm not sure if the mentioned bug was officially closed, as I can not find it
> either on http://codereview.chromium.org or http://crbug.com, but I need to
change the value,
> and it seems to be working.

This was an old internal Google bug that was covered publicly on
http://crbug.com/95 .  I think it's safe to not worry about :)

https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/search_...
File chrome/browser/ui/search_engines/template_url_table_model.cc (right):

https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/search_...
chrome/browser/ui/search_engines/template_url_table_model.cc:164:
static_cast<int>(other_entries.size());
Bonus points if you nuke these casts by making the indexes size_t, but that
might end up being a worse tradeoff if it adds too many casts elsewhere.

[Aaron Jacobs, 2013-06-01 02:37:47]

On 2013/06/01 01:38:03, Peter Kasting wrote:
> LGTM with one substantive question.
> 
>
https://codereview.chromium.org/15805002/diff/9001/chrome/browser/search_engi...
> File chrome/browser/search_engines/template_url_service.cc (right):
> 
>
https://codereview.chromium.org/15805002/diff/9001/chrome/browser/search_engi...
> chrome/browser/search_engines/template_url_service.cc:2115: // the new
> TemplateURL the newly generated keyword.
> On 2013/05/31 03:07:24, Aaron Jacobs wrote:
> > On 2013/05/30 21:22:50, Peter Kasting wrote:
> > > I'm kind of unclear on why we don't just always do this.  Why is it better
> to
> > > re-keyword the existing URL in non-extension cases?
> > > 
> > > (It's been at least a year since I thought about most of this code.)
> > 
> > I was wondering about this - unfortunately, I don't know the reason.
> 
> OK, after staring at this for a while, now I'm wondering the opposite: Why do
we
> want to give the new TURL the new keyword if the existing one is from an
> extension?  I believe the old code was the way it was because I believed that,
> in this situation, a newer TURL was more reflective of "what the user wants as
> of now" and thus more important to preserve as-is.  Not totally sure though.
> 
> Honestly I'm not even sure this case can ever happen.  I thought we protected
> against this.  Maybe this is reachable via sync?
> 
>
https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/search_...
> File chrome/browser/ui/search_engines/keyword_editor_controller.cc (right):
> 
>
https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/search_...
> chrome/browser/ui/search_engines/keyword_editor_controller.cc:42: const int
> new_index = table_model_->last_other_engine_index();
> On 2013/05/31 03:07:24, Aaron Jacobs wrote:
> > I'm not sure if the mentioned bug was officially closed, as I can not find
it
> > either on http://codereview.chromium.org or http://crbug.com, but I need to
> change the value,
> > and it seems to be working.
> 
> This was an old internal Google bug that was covered publicly on
> http://crbug.com/95 .  I think it's safe to not worry about :)
> 
>
https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/search_...
> File chrome/browser/ui/search_engines/template_url_table_model.cc (right):
> 
>
https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/search_...
> chrome/browser/ui/search_engines/template_url_table_model.cc:164:
> static_cast<int>(other_entries.size());
> Bonus points if you nuke these casts by making the indexes size_t, but that
> might end up being a worse tradeoff if it adds too many casts elsewhere.

I'm currently in the process of adjusting some of the tests to work with syncing
extension keywords, but I don't want to progress much further until we decide
how TURL keyword precedence should be handled.

Whether new or old keywords should be uniquified seems rather subjective, and
I'm fine going either way with it. My original thought was that if you had some
extension already installed that was registered with a keyword, it would not be
user-friendly for a new extension install to suddenly cause that keyword to no
longer work as it had before - this is just my opinion however, and I'm fine
with either option.

The question I'm more concerned about relates to what we should do in the case
that a user installs an extension that tries to register a keyword that already
exists, and is non-replaceable. With the layout of the code from before, the new
keyword would simply not be added, though I feel like it would be a better
option to simply uniquify the new keyword in that case.

As for the overall possibility of this happening, I believe it should only occur
either through syncing or if a user installs an extension that has a keyword
defined in its manifest that matches an already existing keyword.

[Peter Kasting, 2013-06-01 08:25:15]

In the interests of continuity, why don't we just leave the code as-is and not
try and change it for the extension case.

Also, we need to ensure that if you install an extension, and then its
keyword(s) get automatically or manually renamed, and then you uninstall, that
those keywords are still successfully removed.  I am not actually sure where
this happens.

[Aaron Jacobs, 2013-06-03 22:48:33]

https://codereview.chromium.org/15805002/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/options/search_engine_manager.js (right):

https://codereview.chromium.org/15805002/diff/21001/chrome/browser/resources/...
chrome/browser/resources/options/search_engine_manager.js:29: defaultsList_:
null,
On 2013/05/31 03:33:56, Dan Beam wrote:
> I didn't see that it was in the same class, sorry about that... you can leave
it
> as it was if you'd like.
> 
> If you like this way, move each ES5 getter under the property itself, i.e.
> 
>   defaultsList_: null,
>   get defaultsList() {
>     return this.defaultsList_;
>   },
> 
> sorry for the mix up and thanks for the work!

Done.

https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/search_engine_manager_handler.cc (right):

https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:193: 
On 2013/05/31 03:33:56, Dan Beam wrote:
> nit: no \n

Done.

[Aaron Jacobs, 2013-06-03 23:07:21]

On 2013/06/03 22:48:33, Aaron Jacobs wrote:
>
https://codereview.chromium.org/15805002/diff/21001/chrome/browser/resources/...
> File chrome/browser/resources/options/search_engine_manager.js (right):
> 
>
https://codereview.chromium.org/15805002/diff/21001/chrome/browser/resources/...
> chrome/browser/resources/options/search_engine_manager.js:29: defaultsList_:
> null,
> On 2013/05/31 03:33:56, Dan Beam wrote:
> > I didn't see that it was in the same class, sorry about that... you can
leave
> it
> > as it was if you'd like.
> > 
> > If you like this way, move each ES5 getter under the property itself, i.e.
> > 
> >   defaultsList_: null,
> >   get defaultsList() {
> >     return this.defaultsList_;
> >   },
> > 
> > sorry for the mix up and thanks for the work!
> 
> Done.
> 
>
https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/webui/o...
> File chrome/browser/ui/webui/options/search_engine_manager_handler.cc (right):
> 
>
https://codereview.chromium.org/15805002/diff/21001/chrome/browser/ui/webui/o...
> chrome/browser/ui/webui/options/search_engine_manager_handler.cc:193: 
> On 2013/05/31 03:33:56, Dan Beam wrote:
> > nit: no \n
> 
> Done.

Peter: When NOTIFICATION_EXTENSION_UNLOADED is sent,
TemplateURLService::UnregisterExtensionKeyword is called, which uses
TemplateURLService::GetTemplateURLForExtension to determine the TURL to be
removed via the extension's id - the actual keyword the extension is using does
not matter. From my testing, it appears that the keywords are being properly
removed.

[Peter Kasting, 2013-06-03 23:08:38]

Sounds good then.

[commit-bot: I haz the power, 2013-06-03 23:10:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/Samusaaron3@gmail.com/15805002/43001

[commit-bot: I haz the power, 2013-06-04 05:55:30]

Change committed as 203855

[Dan Beam, 2013-06-05 03:55:53]

https://chromiumcodereview.appspot.com/15805002/diff/43001/chrome/browser/sea...
File chrome/browser/search_engines/template_url_service.cc (right):

https://chromiumcodereview.appspot.com/15805002/diff/43001/chrome/browser/sea...
chrome/browser/search_engines/template_url_service.cc:709: i =
entries_to_process.erase(i);
you may need to also explicitly delete the entries you're erasing from the
std::vector here if this class owns this data.  heapcheck is whining.