Implement remaining chrome.searchEnginesPrivate methods.

Implement remaining chrome.searchEnginesPrivate methods.

There may be more nuances to the implementation we'll have to fix later, as it's hard to know until I try it with the UI in place. There is also a chance we'll have to make further IDL changes.. But hopefully this is the last major interface change for searchEnginesPrivate.

(Disabling presubmit because it requires a manual confirmation that the histograms changes were OK since I renamed a method. I confirmed this.)

BUG=480043
NOPRESUBMIT=true

Committed: https://crrev.com/229960af7ade46b15e668c4299cb318a52161787
Cr-Commit-Position: refs/heads/master@{#327636}

[Oren Blasberg, 2015-04-28 00:27:42]

orenb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Oren Blasberg, 2015-04-28 00:28:24]

This CL is mostly done, minus a bit of testing related stuff, so thought I'd
send it out for a review at this point. Thanks!

[Oren Blasberg, 2015-04-28 01:58:01]

orenb@chromium.org changed reviewers:
+ mpearson@chromium.org

[Oren Blasberg, 2015-04-28 01:58:02]

+Mark for histogram changes; Thanks in advance!

[Oren Blasberg, 2015-04-28 01:59:11]

orenb@chromium.org changed reviewers:
+ kalman@chromium.org, tbreisacher@chromium.org

[Oren Blasberg, 2015-04-28 01:59:11]

+Ben for idl changes;

+Tyler for externs changes (auto generated).

Thanks!

[Tyler Breisacher (Chromium), 2015-04-28 16:41:29]

If it's really auto-generated then I would think it's fine to TBR it. Maybe add
an OWNERS file in that externs dir that makes everyone an owner. lgtm anyway and
looking forward to using these auto-generated externs in non-Chrome code too!

[not at google - send to devlin, 2015-04-28 16:48:47]

lgtm but consider my comment

https://codereview.chromium.org/1109563003/diff/120001/chrome/common/extensio...
File chrome/common/extensions/api/search_engines_private.idl (right):

https://codereview.chromium.org/1109563003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:65: DOMString guid,
DOMString name, DOMString keyword, DOMString url);
Your API - but consider using single objects with named parameters for these
function calls with some many arguments (this and addOtherSearchEngine),
otherwise the JS can get overwhelming.

[stevenjb, 2015-04-28 17:16:39]

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc
(right):

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:52:
continue;
If the data model differentiates between "default" and "other" with a boolean
(rather than using separate lists), it might make sense for the Extension API to
do the same thing, i.e. put the list separation in the UI. That way if the UX
folks decide to combine the lists we don't have to re-merge the lists. Also it
makes the API a bit simpler.

This is just a thought/suggestion. I don't want to slow things down, but I do
prefer simpler APIs that follow the data model (instead of a particular UX
model).

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:116:
continue;
If we do decide to keep this as two separate APIs we should at least move the
copy/pasted code into a common helper method that takes 'get_default' as a
boolean.

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:251:
hotword_service->DisableHotwordPreferences();
This seems like a strange place to do this; effectively we can end up disabling
hotwording as a side-effect of a GetHotwordState call.

Either this should be removed from here if it isn't necessary, or this entire
clause should be wrapped into a helper function somewhere. You may need to get
in touch with the maintainer(s) of the HotwordService code.

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:257:
return RespondNow(OneArgument(state->ToValue().release()));
It looks like we could replace the clause above with:

if (!default_url || !default_url->HasGoogleBasedURLs(...) ||
!HotwordServiceFactory::IsHotwordAllowed(profile))
  return RespondNow(OneArgument(state->ToValue().release()));
}

and eliminate lines 245-252?

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:285:
state->error_msg = scoped_ptr<std::string>(&error_message);
state->error_msg.reset(new std::string(error_message));

As written... you're passing the address of a local variable to a scoped_ptr.

Alternatively you could do:
scoped_ptr<std::string> error_message(...(error));
...
state->error_msg = error_message.Pass();

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:354:
DCHECK(profile->GetPrefs()->GetBoolean(prefs::kHotwordAudioLoggingEnabled));
Since this is an API we should set an error and fail if either of these are
false, not crash on Debug and proceed on Release.

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:360:
!profile->GetPrefs()->GetBoolean(prefs::kHotwordAlwaysOnSearchEnabled));
Sane here.

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:364:
!profile->GetPrefs()->GetBoolean(prefs::kHotwordAlwaysOnSearchEnabled));
And here.

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.cc
(right):

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.cc:62:
listening_for_others_ = false;
We could also avoid the added complexity in this class if we pass and update a
single list in the API and move the list separation to the JS.

https://codereview.chromium.org/1109563003/diff/120001/chrome/common/extensio...
File chrome/common/extensions/api/search_engines_private.idl (right):

https://codereview.chromium.org/1109563003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:55: static void
getOtherSearchEngines(SearchEnginesCallback callback);
nit: group this with getDefaultSearchEngines

https://codereview.chromium.org/1109563003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:73: // Opts in to
hotwording; behavior depends on whether retrain is needed.
Could you elaborated on "behavior depends on whether retrain is needed"? I'm not
very familiar with the feature so it is unclear to me what that means.

[Oren Blasberg, 2015-04-28 18:44:01]

I had a couple follow ups so wanted to hit reply though I'm not done addressing
all the comments yet.

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc
(right):

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:285:
state->error_msg = scoped_ptr<std::string>(&error_message);
On 2015/04/28 17:16:38, stevenjb wrote:
> state->error_msg.reset(new std::string(error_message));
> 
> As written... you're passing the address of a local variable to a scoped_ptr.
> 
> Alternatively you could do:
> scoped_ptr<std::string> error_message(...(error));
> ...
> state->error_msg = error_message.Pass();

Done.

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:354:
DCHECK(profile->GetPrefs()->GetBoolean(prefs::kHotwordAudioLoggingEnabled));
On 2015/04/28 17:16:39, stevenjb wrote:
> Since this is an API we should set an error and fail if either of these are
> false, not crash on Debug and proceed on Release.

Done (setting error) -- Q: How do I "fail"? I can't return false since this is a
UIThreadExtensionFunction not an Async one. Also, this function doesn't take a
success or failure callback as argument.

https://codereview.chromium.org/1109563003/diff/120001/chrome/common/extensio...
File chrome/common/extensions/api/search_engines_private.idl (right):

https://codereview.chromium.org/1109563003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:55: static void
getOtherSearchEngines(SearchEnginesCallback callback);
On 2015/04/28 17:16:39, stevenjb wrote:
> nit: group this with getDefaultSearchEngines

Done.

https://codereview.chromium.org/1109563003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:65: DOMString guid,
DOMString name, DOMString keyword, DOMString url);
On 2015/04/28 16:48:46, kalman wrote:
> Your API - but consider using single objects with named parameters for these
> function calls with some many arguments (this and addOtherSearchEngine),
> otherwise the JS can get overwhelming.

That's definitely a fair point. We already have the SearchEngine dictionary and
we could have this function just take one of those instead. However, the reason
I didn't do that is that we won't know at least one major part of its data (the
guid) ahead of time, i.e. when adding a new search engine. I could make the guid
optional on the SearchEngine, but then it's really only optional in some cases
(the guid must be there when we fetch the search engines, but it must not be
there when we set a brand new search engine).

Alternatively I could also just add yet another dictionary which only contains
the "set time" arguments but this would duplicate most of what's in
SearchEngine.

Seems like these are tradeoffs between a few imperfect choices.

https://codereview.chromium.org/1109563003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:73: // Opts in to
hotwording; behavior depends on whether retrain is needed.
On 2015/04/28 17:16:39, stevenjb wrote:
> Could you elaborated on "behavior depends on whether retrain is needed"? I'm
not
> very familiar with the feature so it is unclear to me what that means.

Done.

[Oren Blasberg, 2015-04-28 18:54:26]

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc
(right):

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:251:
hotword_service->DisableHotwordPreferences();
On 2015/04/28 17:16:38, stevenjb wrote:
> This seems like a strange place to do this; effectively we can end up
disabling
> hotwording as a side-effect of a GetHotwordState call.
> 
> Either this should be removed from here if it isn't necessary, or this entire
> clause should be wrapped into a helper function somewhere. You may need to get
> in touch with the maintainer(s) of the HotwordService code.

disclaimer: Lots of this code is copied/adapted from BrowserOptionsHandler.

Yeah, I can see why the getter having mutation effects would be weird.
FWIW, My intent here was that we would call GetHotwordState whenever any of the
default search engines changes. That way if it changes from google to something
else, this code will disable the hotword prefs for us neatly.

Does that seem like something that should happen in the JS instead? We'd need a
method for disabling hotword preferences (it's not quite as straightforward as
"turn off these 2 prefs"; else we'd just use settings private. Check out [1]  )

We'd also need a js method for determining if the current search engine 'has
google base urls' (we could add an API method for that too)

Thoughts?


[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sea...

[stevenjb, 2015-04-28 19:06:03]

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc
(right):

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:354:
DCHECK(profile->GetPrefs()->GetBoolean(prefs::kHotwordAudioLoggingEnabled));
On 2015/04/28 18:44:00, Oren Blasberg wrote:
> On 2015/04/28 17:16:39, stevenjb wrote:
> > Since this is an API we should set an error and fail if either of these are
> > false, not crash on Debug and proceed on Release.
> 
> Done (setting error) -- Q: How do I "fail"? I can't return false since this is
a
> UIThreadExtensionFunction not an Async one. Also, this function doesn't take a
> success or failure callback as argument.

I think you just set 'error_ = errordesc' then SendResponse(false). That's what
we do for async methods in networking_private_api.cc. You might need to look
around for the right synchronous pattern.

[Oren Blasberg, 2015-04-28 21:58:30]

PTAL

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc
(right):

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:52:
continue;
On 2015/04/28 17:16:38, stevenjb wrote:
> If the data model differentiates between "default" and "other" with a boolean
> (rather than using separate lists), it might make sense for the Extension API
to
> do the same thing, i.e. put the list separation in the UI. That way if the UX
> folks decide to combine the lists we don't have to re-merge the lists. Also it
> makes the API a bit simpler.
> 
> This is just a thought/suggestion. I don't want to slow things down, but I do
> prefer simpler APIs that follow the data model (instead of a particular UX
> model).

Done.

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:116:
continue;
On 2015/04/28 17:16:39, stevenjb wrote:
> If we do decide to keep this as two separate APIs we should at least move the
> copy/pasted code into a common helper method that takes 'get_default' as a
> boolean.

Ended up combining them.

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:257:
return RespondNow(OneArgument(state->ToValue().release()));
On 2015/04/28 17:16:38, stevenjb wrote:
> It looks like we could replace the clause above with:
> 
> if (!default_url || !default_url->HasGoogleBasedURLs(...) ||
> !HotwordServiceFactory::IsHotwordAllowed(profile))
>   return RespondNow(OneArgument(state->ToValue().release()));
> }
> 
> and eliminate lines 245-252?

I don't quite see how this is equivalent. Which clause are you proposing to
replace? I think maybe I'm confused because having a bunch of !'s in a row make
it a bit harder to reasona about the logic.

In any case I removed the "else" above because we decided to pull disabling
hotword preferences out of the purview of the extension api code.

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:354:
DCHECK(profile->GetPrefs()->GetBoolean(prefs::kHotwordAudioLoggingEnabled));
On 2015/04/28 19:06:03, stevenjb wrote:
> On 2015/04/28 18:44:00, Oren Blasberg wrote:
> > On 2015/04/28 17:16:39, stevenjb wrote:
> > > Since this is an API we should set an error and fail if either of these
are
> > > false, not crash on Debug and proceed on Release.
> > 
> > Done (setting error) -- Q: How do I "fail"? I can't return false since this
is
> a
> > UIThreadExtensionFunction not an Async one. Also, this function doesn't take
a
> > success or failure callback as argument.
> 
> I think you just set 'error_ = errordesc' then SendResponse(false). That's
what
> we do for async methods in networking_private_api.cc. You might need to look
> around for the right synchronous pattern.
> 

Done.

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:360:
!profile->GetPrefs()->GetBoolean(prefs::kHotwordAlwaysOnSearchEnabled));
On 2015/04/28 17:16:38, stevenjb wrote:
> Sane here.

Done.

https://codereview.chromium.org/1109563003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:364:
!profile->GetPrefs()->GetBoolean(prefs::kHotwordAlwaysOnSearchEnabled));
On 2015/04/28 17:16:38, stevenjb wrote:
> And here.

Done.

[stevenjb, 2015-04-28 22:27:12]

Looking good. Mostly nits.

https://codereview.chromium.org/1109563003/diff/180001/chrome/browser/extensi...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc
(right):

https://codereview.chromium.org/1109563003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:246:
base::ASCIIToUTF16(chrome::kHotwordLearnMoreURL);
Since this is only used inside the if() below, we should move it there (or
eliminate the local and inline it since it's only used once).

https://codereview.chromium.org/1109563003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:260:
DCHECK(!user_display_name.empty());
I realize this was mostly copied from browser_options, but we should generally
avoid DCHECK in extension API code. In this case an empty display name would be
bad from a UI perspective, but not fatal. We can trust that other code, i.e.
SigninManager will ensure this value is valid.

https://codereview.chromium.org/1109563003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:263:
base::ASCIIToUTF16(user_display_name));
Here also, declare where it is used or inline.

Note: Before clang format, maintaining the formatting of lengthy function calls
like the one below was kind of a pain, so devs used local variables more
frequently. With clang format, inlining is generally preferred unless the
variable is used more than once or the variable name provides some added
clarity.

Again, I realize the code was probably copied from elsewhere, but always good to
make things a little cleaner as we go.

https://codereview.chromium.org/1109563003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:289:
state->audio_history_state = make_scoped_ptr(&audio_history_state_str);
state->audio_history_state(new
std::string(base::UTF16ToASCII(audio_history_state));

https://codereview.chromium.org/1109563003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:320:
!profile->GetPrefs()->GetBoolean(
nit: we call profile->GetPrefs() a bunch in this method; a local might clean
this up a bit.

https://codereview.chromium.org/1109563003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:323:
"Cannot retrain without always search and audio logging enabled"));
For error messages passed in an API we should use const char[] variables at the
top of the file.

https://codereview.chromium.org/1109563003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:343:
if (hotword_service)
nit: if (!hotword_service) all of the above is pointless, so we should declare
it towards the top and early exit with an error if null.

https://codereview.chromium.org/1109563003/diff/180001/chrome/common/extensio...
File chrome/common/extensions/api/search_engines_private.idl (right):

https://codereview.chromium.org/1109563003/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:78: // Opts in to
hotwording; |retrain| indicates whether the user just wants to
s/just// ?

https://codereview.chromium.org/1109563003/diff/200001/chrome/browser/extensi...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc
(right):

https://codereview.chromium.org/1109563003/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:216:
return RespondNow(OneArgument(state->ToValue().release()));
We can now eliminate is_search_provider_google and combine this if with the one
above.

[Oren Blasberg, 2015-04-29 00:24:59]

https://codereview.chromium.org/1109563003/diff/180001/chrome/browser/extensi...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc
(right):

https://codereview.chromium.org/1109563003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:246:
base::ASCIIToUTF16(chrome::kHotwordLearnMoreURL);
On 2015/04/28 22:27:11, stevenjb wrote:
> Since this is only used inside the if() below, we should move it there (or
> eliminate the local and inline it since it's only used once).
> 

Done.

https://codereview.chromium.org/1109563003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:260:
DCHECK(!user_display_name.empty());
On 2015/04/28 22:27:11, stevenjb wrote:
> I realize this was mostly copied from browser_options, but we should generally
> avoid DCHECK in extension API code. In this case an empty display name would
be
> bad from a UI perspective, but not fatal. We can trust that other code, i.e.
> SigninManager will ensure this value is valid.

Thanks for reviewing this code thoroughly :) Done.

https://codereview.chromium.org/1109563003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:263:
base::ASCIIToUTF16(user_display_name));
On 2015/04/28 22:27:11, stevenjb wrote:
> Here also, declare where it is used or inline.
> 
> Note: Before clang format, maintaining the formatting of lengthy function
calls
> like the one below was kind of a pain, so devs used local variables more
> frequently. With clang format, inlining is generally preferred unless the
> variable is used more than once or the variable name provides some added
> clarity.
> 
> Again, I realize the code was probably copied from elsewhere, but always good
to
> make things a little cleaner as we go.
> 

Done.

https://codereview.chromium.org/1109563003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:289:
state->audio_history_state = make_scoped_ptr(&audio_history_state_str);
On 2015/04/28 22:27:11, stevenjb wrote:
> state->audio_history_state(new
> std::string(base::UTF16ToASCII(audio_history_state));

Done. (assuming you meant to have a .reset in there)

https://codereview.chromium.org/1109563003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:320:
!profile->GetPrefs()->GetBoolean(
On 2015/04/28 22:27:11, stevenjb wrote:
> nit: we call profile->GetPrefs() a bunch in this method; a local might clean
> this up a bit.

Done.

https://codereview.chromium.org/1109563003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:323:
"Cannot retrain without always search and audio logging enabled"));
On 2015/04/28 22:27:11, stevenjb wrote:
> For error messages passed in an API we should use const char[] variables at
the
> top of the file.

Done.

https://codereview.chromium.org/1109563003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:343:
if (hotword_service)
On 2015/04/28 22:27:11, stevenjb wrote:
> nit: if (!hotword_service) all of the above is pointless, so we should declare
> it towards the top and early exit with an error if null.

Done.

https://codereview.chromium.org/1109563003/diff/200001/chrome/browser/extensi...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc
(right):

https://codereview.chromium.org/1109563003/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:216:
return RespondNow(OneArgument(state->ToValue().release()));
On 2015/04/28 22:27:12, stevenjb wrote:
> We can now eliminate is_search_provider_google and combine this if with the
one
> above.

Done.

[stevenjb, 2015-04-29 01:05:26]

LGTM w/ one small fix.

https://codereview.chromium.org/1109563003/diff/220001/chrome/browser/extensi...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc
(right):

https://codereview.chromium.org/1109563003/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:256:
base::ASCIIToUTF16(user_display_name));
elim

[Mark P, 2015-04-29 18:00:19]

https://codereview.chromium.org/1109563003/diff/220001/extensions/browser/ext...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/1109563003/diff/220001/extensions/browser/ext...
extensions/browser/extension_function_histogram_value.h:1072:
SEARCHENGINESPRIVATE_GETSEARCHENGINES,
Does this function do something different than it did before?  If so, then you
may want to consider a new function and simply deprecate the old one.

That said, if no one can cause this function to be emitted currently, then this
change is okay.

[stevenjb, 2015-04-29 18:16:08]

https://codereview.chromium.org/1109563003/diff/220001/extensions/browser/ext...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/1109563003/diff/220001/extensions/browser/ext...
extensions/browser/extension_function_histogram_value.h:1072:
SEARCHENGINESPRIVATE_GETSEARCHENGINES,
On 2015/04/29 18:00:19, Mark P wrote:
> Does this function do something different than it did before?  If so, then you
> may want to consider a new function and simply deprecate the old one.
> 
> That said, if no one can cause this function to be emitted currently, then
this
> change is okay.

This is a private API that is in development and has not been released yet (it's
still behind a flag in Dev), so changing this should be fine.

[Mark P, 2015-04-29 18:16:56]

histograms.xml lgtm

[Oren Blasberg, 2015-04-30 00:27:16]

The CQ bit was checked by orenb@chromium.org

[Oren Blasberg, 2015-04-30 00:27:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
tbreisacher@chromium.org, kalman@chromium.org, stevenjb@chromium.org,
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/1109563003/#ps240001
(title: "Address last comment   sync   fix trybot failure")

[commit-bot: I haz the power, 2015-04-30 00:27:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1109563003/240001

[Oren Blasberg, 2015-04-30 00:43:52]

https://codereview.chromium.org/1109563003/diff/220001/chrome/browser/extensi...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc
(right):

https://codereview.chromium.org/1109563003/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:256:
base::ASCIIToUTF16(user_display_name));
On 2015/04/29 01:05:26, stevenjb wrote:
> elim

Done.

[commit-bot: I haz the power, 2015-04-30 01:28:53]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-04-30 01:29:56]

Patchset 13 (id:??) landed as
https://crrev.com/229960af7ade46b15e668c4299cb318a52161787
Cr-Commit-Position: refs/heads/master@{#327636}