Add chrome.searchEnginesPrivate API and Search Settings page.

Add chrome.searchEnginesPrivate API for setting and getting the default search engines.

Also add the first parts of the Search settings UI in MD-settings, namely the 'default search engine' dropdown. Follow-up CLs will fill in the remaining functionality.

(Disabling presubmit because it requires a manual confirmation that permission_message.h looks ok. I confirmed this.)

BUG=479359
NOPRESUBMIT=true

Committed: https://crrev.com/e733e8f9066efccf0dd05dbeaa94ed62828b22a5
Cr-Commit-Position: refs/heads/master@{#326844}

[Oren Blasberg, 2015-04-21 23:15:14]

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

[Oren Blasberg, 2015-04-22 16:45:21]

Oops; forgot to hit 'send message' yesterday

[Oren Blasberg, 2015-04-22 17:19:30]

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

[Oren Blasberg, 2015-04-22 17:19:37]

+Ben for interface changes.

[Jeremy Klein, 2015-04-22 17:37:46]

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.h
(right):

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.h:36:
// Implements the chrome.searchEnginesPrivate.getAllPrefs method.
s/getAllPrefs/setDefaultSearchEngine/

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.html (right):

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.html:6: <link
rel="import"
href="chrome://md-settings/settings_page/settings_page_header.html">
Not needed.

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.html:8: <link
rel="import" href="search_page_style.html">
Don't we also need core-style?

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.html:15: <span
i18n-content="searchExplanation"></span>
nit: I'm not sure a span is really the right choice here. Should it be a <p>?
Maybe a <div>?

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.html:23: <paper-item
value="{{engine.guid}}">{{engine.name}}</paper-item>
Let's switch "value" to "guid" to be more specific.

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.js (right):

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:57: * Name of the
'core-icon' to show.
nit: ... in the page header.

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:69: * @type Array
I know all of our types don't have {} right now, but let's get in the habit of
adding them.

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:69: * @type Array
!Array<!chrome.searchEnginesPrivate.SearchEngine>, right?

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:72: searchEngines:
[],
Array and Object properties should be initialized in the created callback. See
https://www.polymer-project.org/0.5/docs/polymer/polymer.html#default-propert...

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:84: domReady:
function() {
I think this could just be done in attached or ready, but it doesn't really
matter.

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:84: domReady:
function() {
@override

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:91:
defaultEngineGuidChanged: function() {
Can you add a jsdoc just to clarify that this is called when the user selects a
new default?

https://codereview.chromium.org/1096143003/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/search_engines_private.idl (right):

https://codereview.chromium.org/1096143003/diff/80001/chrome/common/extension...
chrome/common/extensions/api/search_engines_private.idl:24: static void
getDefaultSearchEngines(SearchEnginesCallback callback);
nit: It seems weird to have the word "Default" in here since really it's just
all search engines.

[Oren Blasberg, 2015-04-22 17:41:22]

https://codereview.chromium.org/1096143003/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/search_engines_private.idl (right):

https://codereview.chromium.org/1096143003/diff/80001/chrome/common/extension...
chrome/common/extensions/api/search_engines_private.idl:24: static void
getDefaultSearchEngines(SearchEnginesCallback callback);
On 2015/04/22 17:37:46, Jeremy Klein wrote:
> nit: It seems weird to have the word "Default" in here since really it's just
> all search engines.

In the future there will also be "custom" / "other" search engines which pertain
to those lower down in the 'manage search engines' dialog. You can see this at
chrome://settings/searchEngines.  Hence I think "default" is a good specifier.
WDYT? Maybe I could add a comment explaining that?

[Jeremy Klein, 2015-04-22 17:43:33]

On 2015/04/22 17:41:22, Oren Blasberg wrote:
>
https://codereview.chromium.org/1096143003/diff/80001/chrome/common/extension...
> File chrome/common/extensions/api/search_engines_private.idl (right):
> 
>
https://codereview.chromium.org/1096143003/diff/80001/chrome/common/extension...
> chrome/common/extensions/api/search_engines_private.idl:24: static void
> getDefaultSearchEngines(SearchEnginesCallback callback);
> On 2015/04/22 17:37:46, Jeremy Klein wrote:
> > nit: It seems weird to have the word "Default" in here since really it's
just
> > all search engines.
> 
> In the future there will also be "custom" / "other" search engines which
pertain
> to those lower down in the 'manage search engines' dialog. You can see this at
> chrome://settings/searchEngines.  Hence I think "default" is a good specifier.
> WDYT? Maybe I could add a comment explaining that?

Hmm ok, that makes sense. Can you just add the word "Default" to the comment
then?

[Oren Blasberg, 2015-04-22 17:45:13]

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

[Oren Blasberg, 2015-04-22 17:45:14]

mpearson@chromium.org: Please review changes in histogram related files; thanks!

[Mark P, 2015-04-22 17:52:41]

histograms.xml lgtm

[Oren Blasberg, 2015-04-22 18:25:40]

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

[Oren Blasberg, 2015-04-22 18:25:41]

jhawkins@chromium.org: Please review changes in the API implementation. Thanks!

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.h
(right):

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.h:36:
// Implements the chrome.searchEnginesPrivate.getAllPrefs method.
On 2015/04/22 17:37:45, Jeremy Klein wrote:
> s/getAllPrefs/setDefaultSearchEngine/

Done.

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.html (right):

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.html:6: <link
rel="import"
href="chrome://md-settings/settings_page/settings_page_header.html">
On 2015/04/22 17:37:46, Jeremy Klein wrote:
> Not needed.

Done.

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.html:8: <link
rel="import" href="search_page_style.html">
On 2015/04/22 17:37:46, Jeremy Klein wrote:
> Don't we also need core-style?

search_page_style.html pulls that in.

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.html:15: <span
i18n-content="searchExplanation"></span>
On 2015/04/22 17:37:46, Jeremy Klein wrote:
> nit: I'm not sure a span is really the right choice here. Should it be a <p>?
> Maybe a <div>? 

Done.

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.html:23: <paper-item
value="{{engine.guid}}">{{engine.name}}</paper-item>
On 2015/04/22 17:37:45, Jeremy Klein wrote:
> Let's switch "value" to "guid" to be more specific.

Done.

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.js (right):

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:57: * Name of the
'core-icon' to show.
On 2015/04/22 17:37:46, Jeremy Klein wrote:
> nit: ... in the page header.

Done.

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:69: * @type Array
On 2015/04/22 17:37:46, Jeremy Klein wrote:
> !Array<!chrome.searchEnginesPrivate.SearchEngine>, right?

Done.

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:72: searchEngines:
[],
On 2015/04/22 17:37:46, Jeremy Klein wrote:
> Array and Object properties should be initialized in the created callback. See
>
https://www.polymer-project.org/0.5/docs/polymer/polymer.html#default-propert...

Done.

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:84: domReady:
function() {
On 2015/04/22 17:37:46, Jeremy Klein wrote:
> @override

Done.

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.js:91:
defaultEngineGuidChanged: function() {
On 2015/04/22 17:37:46, Jeremy Klein wrote:
> Can you add a jsdoc just to clarify that this is called when the user selects
a
> new default?

Done.

[James Hawkins, 2015-04-22 18:46:01]

lgtm

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

https://codereview.chromium.org/1096143003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:5: // Use the
<code>chrome.searchEnginesPrivate</code> API to get or set preferences
80 cols.

[Jeremy Klein, 2015-04-22 18:51:14]

lgtm

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.html (right):

https://codereview.chromium.org/1096143003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.html:8: <link
rel="import" href="search_page_style.html">
On 2015/04/22 18:25:41, Oren Blasberg wrote:
> On 2015/04/22 17:37:46, Jeremy Klein wrote:
> > Don't we also need core-style?
> 
> search_page_style.html pulls that in.

This should too. It directly has a core-style element below so it needs to
depend on it. We shouldn't rely on transitive dependencies.

https://codereview.chromium.org/1096143003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/search_page/search_page.js (right):

https://codereview.chromium.org/1096143003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/search_page/search_page.js:60: * @type string
Please wrap all types in {}.

[Oren Blasberg, 2015-04-22 19:00:25]

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

[Oren Blasberg, 2015-04-22 19:04:18]

+Tyler for externs.

https://codereview.chromium.org/1096143003/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/search_engines_private.idl (right):

https://codereview.chromium.org/1096143003/diff/80001/chrome/common/extension...
chrome/common/extensions/api/search_engines_private.idl:24: static void
getDefaultSearchEngines(SearchEnginesCallback callback);
On 2015/04/22 17:41:22, Oren Blasberg wrote:
> On 2015/04/22 17:37:46, Jeremy Klein wrote:
> > nit: It seems weird to have the word "Default" in here since really it's
just
> > all search engines.
> 
> In the future there will also be "custom" / "other" search engines which
pertain
> to those lower down in the 'manage search engines' dialog. You can see this at
> chrome://settings/searchEngines.  Hence I think "default" is a good specifier.
> WDYT? Maybe I could add a comment explaining that?

Done.

https://codereview.chromium.org/1096143003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/search_page/search_page.js (right):

https://codereview.chromium.org/1096143003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/search_page/search_page.js:60: * @type string
On 2015/04/22 18:51:14, Jeremy Klein wrote:
> Please wrap all types in {}.

Done.

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

https://codereview.chromium.org/1096143003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:5: // Use the
<code>chrome.searchEnginesPrivate</code> API to get or set preferences
On 2015/04/22 18:46:01, James Hawkins wrote:
> 80 cols.

Done.

[Tyler Breisacher (Chromium), 2015-04-22 19:08:21]

https://codereview.chromium.org/1096143003/diff/140001/third_party/closure_co...
File third_party/closure_compiler/externs/search_engines_private.js (right):

https://codereview.chromium.org/1096143003/diff/140001/third_party/closure_co...
third_party/closure_compiler/externs/search_engines_private.js:1: // Copyright
2015 The Chromium Authors. All rights reserved.
lgtm given that this is autogenerated (at some point we should stop checking
them in and have them come from a 'genrule', but anyway...)

[not at google - send to devlin, 2015-04-22 19:32:59]

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/_permission_features.json:875: "platforms":
["chromeos", "mac", "win", "linux"],
I see that settingsPrivate did enumerate all of these; but would "all" be
enough?

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/_permission_features.json:876: "whitelist": []
An empty whitelist is just parsed as no whitelist, i.e. it has no effect and
this means that any extension on trunk is able to call these APIs. It's fine
since it's trunk, but it could be a disaster if that was accidentally changed
(it's also misleading like this).

Is the goal to have the settings app be a component extension? In that case, you
could put "location": "component" here instead, and then add a TODO to whitelist
the specific ID rather than all component extensions, potentially.

Same for settingsPrivate.

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

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:7: namespace
searchEnginesPrivate {
How will this interact with the search engine overrides API? I would have
thought (?) that you may need an extension ID in here as well, if it was
installed from an extension.

https://codereview.chromium.org/1096143003/diff/140001/third_party/closure_co...
File third_party/closure_compiler/externs/search_engines_private.js (right):

https://codereview.chromium.org/1096143003/diff/140001/third_party/closure_co...
third_party/closure_compiler/externs/search_engines_private.js:1: // Copyright
2015 The Chromium Authors. All rights reserved.
On 2015/04/22 19:08:20, Tyler Breisacher (Chromium) wrote:
> lgtm given that this is autogenerated (at some point we should stop checking
> them in and have them come from a 'genrule', but anyway...)

That's the plan!

[stevenjb, 2015-04-22 19:38:03]

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

[stevenjb, 2015-04-22 19:38:03]

Sorry to make this request after a bunch of l-g-t-ms, but I really think we
should separate the searchEngingPrivate implementation from the settings UI
changes.

[Jeremy Klein, 2015-04-22 19:43:43]

On 2015/04/22 19:38:03, stevenjb wrote:
> Sorry to make this request after a bunch of l-g-t-ms, but I really think we
> should separate the searchEngingPrivate implementation from the settings UI
> changes.

The JS is relatively light here and I think it helps to have the full picture
for this API including its use. As a reviewer, I didn't find the cognitive load
too high in this case. A huge chunk of this CL is just boilerplate.

[stevenjb, 2015-04-22 19:46:30]

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

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:10: // The unique ID of
the engine in the list.
'The unique identifier for the search engine.'

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:16: // Whether the
engine is default.
'Whether the engine is the default search engine'
(i.e. not whether or not it is in the default list, that is implicit).

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:27: static void
setDefaultSearchEngine(DOMString guid);
See discussion in API doc

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:32: static void
onSearchEnginesChanged(SearchEngine[] engines);
See discussion in API doc

[stevenjb, 2015-04-22 19:49:22]

On 2015/04/22 19:43:43, Jeremy Klein wrote:
> On 2015/04/22 19:38:03, stevenjb wrote:
> > Sorry to make this request after a bunch of l-g-t-ms, but I really think we
> > should separate the searchEngingPrivate implementation from the settings UI
> > changes.
> 
> The JS is relatively light here and I think it helps to have the full picture
> for this API including its use. As a reviewer, I didn't find the cognitive
load
> too high in this case. A huge chunk of this CL is just boilerplate.

"Light" or not, about half the files are settings page specific, and having
those changes interlaced with API changes is going to make this review take
longer in my experience. It is pretty straightforward to have two CLs, one
dependent on the other and each linked to the other.

[Oren Blasberg, 2015-04-22 22:36:11]

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/_permission_features.json:875: "platforms":
["chromeos", "mac", "win", "linux"],
On 2015/04/22 19:32:59, kalman wrote:
> I see that settingsPrivate did enumerate all of these; but would "all" be
> enough?

I didn't know there was an "all" since there is no "all" in this file anywhere
-- do I need to put "all" explicitly or does omitting the "platforms" altogether
yield that functionality?

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/_permission_features.json:876: "whitelist": []
On 2015/04/22 19:32:59, kalman wrote:
> An empty whitelist is just parsed as no whitelist, i.e. it has no effect and
> this means that any extension on trunk is able to call these APIs. It's fine
> since it's trunk, but it could be a disaster if that was accidentally changed
> (it's also misleading like this).
> 
> Is the goal to have the settings app be a component extension? In that case,
you
> could put "location": "component" here instead, and then add a TODO to
whitelist
> the specific ID rather than all component extensions, potentially.
> 
> Same for settingsPrivate.

Oh wow, didn't realize that! Thanks. 

Our original plan was for the new Settings to be a Chrome app that's bundled
with the browser. Is that what you meant by "component extension"? 

However, we've decided to defer that approach due to some technical issues we
ran into. Instead we are implementing it using just a WebUI that's hosted at
chrome://md-settings (for now, and later at chrome://settings when it's done).
Hence the "matches" section in _api_features.json.

Given that that's the case, how do we achieve having the API only working from
those whitelisted URLs and nowhere else?

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

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:7: namespace
searchEnginesPrivate {
On 2015/04/22 19:32:59, kalman wrote:
> How will this interact with the search engine overrides API? I would have
> thought (?) that you may need an extension ID in here as well, if it was
> installed from an extension.

Hmm, where is that API located? I can't find it anywhere in the code (though it
sounds vaguely familiar).

https://codereview.chromium.org/1096143003/diff/140001/third_party/closure_co...
File third_party/closure_compiler/externs/search_engines_private.js (right):

https://codereview.chromium.org/1096143003/diff/140001/third_party/closure_co...
third_party/closure_compiler/externs/search_engines_private.js:1: // Copyright
2015 The Chromium Authors. All rights reserved.
On 2015/04/22 19:32:59, kalman wrote:
> On 2015/04/22 19:08:20, Tyler Breisacher (Chromium) wrote:
> > lgtm given that this is autogenerated (at some point we should stop checking
> > them in and have them come from a 'genrule', but anyway...)
> 
> That's the plan!

Nice :)
I've updated this file after using the compiler.py to autogenerate it. It made a
few small tweaks. Mainly it changed chrome.searchEnginesPrivate.SearchEngine to
just SearchEngine.

[not at google - send to devlin, 2015-04-22 22:43:52]

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/_permission_features.json:875: "platforms":
["chromeos", "mac", "win", "linux"],
On 2015/04/22 22:36:11, Oren Blasberg wrote:
> On 2015/04/22 19:32:59, kalman wrote:
> > I see that settingsPrivate did enumerate all of these; but would "all" be
> > enough?
> 
> I didn't know there was an "all" since there is no "all" in this file anywhere
> -- do I need to put "all" explicitly or does omitting the "platforms"
altogether
> yield that functionality?

Oh right, yes, probably omitting it is even better :-)

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/_permission_features.json:876: "whitelist": []
On 2015/04/22 22:36:10, Oren Blasberg wrote:
> On 2015/04/22 19:32:59, kalman wrote:
> > An empty whitelist is just parsed as no whitelist, i.e. it has no effect and
> > this means that any extension on trunk is able to call these APIs. It's fine
> > since it's trunk, but it could be a disaster if that was accidentally
changed
> > (it's also misleading like this).
> > 
> > Is the goal to have the settings app be a component extension? In that case,
> you
> > could put "location": "component" here instead, and then add a TODO to
> whitelist
> > the specific ID rather than all component extensions, potentially.
> > 
> > Same for settingsPrivate.
> 
> Oh wow, didn't realize that! Thanks. 
> 
> Our original plan was for the new Settings to be a Chrome app that's bundled
> with the browser. Is that what you meant by "component extension"? 
> 
> However, we've decided to defer that approach due to some technical issues we
> ran into. Instead we are implementing it using just a WebUI that's hosted at
> chrome://md-settings (for now, and later at chrome://settings when it's done).
> Hence the "matches" section in _api_features.json.
> 
> Given that that's the case, how do we achieve having the API only working from
> those whitelisted URLs and nowhere else?

Ah right. In that case you don't even need a permission at all - just have the
entry in _api_features.json contain that single rule, exposing it to WebUI. Same
for settingsPrivate.

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

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:7: namespace
searchEnginesPrivate {
On 2015/04/22 22:36:11, Oren Blasberg wrote:
> On 2015/04/22 19:32:59, kalman wrote:
> > How will this interact with the search engine overrides API? I would have
> > thought (?) that you may need an extension ID in here as well, if it was
> > installed from an extension.
> 
> Hmm, where is that API located? I can't find it anywhere in the code (though
it
> sounds vaguely familiar).

It's called "chrome_settings_overrides", implemented in
settings_overrides_handler.h.

[Tyler Breisacher (Chromium), 2015-04-22 22:45:46]

tbreisacher@chromium.org changed reviewers:
- tbreisacher@chromium.org

[Oren Blasberg, 2015-04-22 22:59:19]

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/_permission_features.json:875: "platforms":
["chromeos", "mac", "win", "linux"],
On 2015/04/22 22:43:51, kalman wrote:
> On 2015/04/22 22:36:11, Oren Blasberg wrote:
> > On 2015/04/22 19:32:59, kalman wrote:
> > > I see that settingsPrivate did enumerate all of these; but would "all" be
> > > enough?
> > 
> > I didn't know there was an "all" since there is no "all" in this file
anywhere
> > -- do I need to put "all" explicitly or does omitting the "platforms"
> altogether
> > yield that functionality?
> 
> Oh right, yes, probably omitting it is even better :-)

Removed altogether.

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/_permission_features.json:876: "whitelist": []
On 2015/04/22 22:43:51, kalman wrote:
> On 2015/04/22 22:36:10, Oren Blasberg wrote:
> > On 2015/04/22 19:32:59, kalman wrote:
> > > An empty whitelist is just parsed as no whitelist, i.e. it has no effect
and
> > > this means that any extension on trunk is able to call these APIs. It's
fine
> > > since it's trunk, but it could be a disaster if that was accidentally
> changed
> > > (it's also misleading like this).
> > > 
> > > Is the goal to have the settings app be a component extension? In that
case,
> > you
> > > could put "location": "component" here instead, and then add a TODO to
> > whitelist
> > > the specific ID rather than all component extensions, potentially.
> > > 
> > > Same for settingsPrivate.
> > 
> > Oh wow, didn't realize that! Thanks. 
> > 
> > Our original plan was for the new Settings to be a Chrome app that's bundled
> > with the browser. Is that what you meant by "component extension"? 
> > 
> > However, we've decided to defer that approach due to some technical issues
we
> > ran into. Instead we are implementing it using just a WebUI that's hosted at
> > chrome://md-settings (for now, and later at chrome://settings when it's
done).
> > Hence the "matches" section in _api_features.json.
> > 
> > Given that that's the case, how do we achieve having the API only working
from
> > those whitelisted URLs and nowhere else?
> 
> Ah right. In that case you don't even need a permission at all - just have the
> entry in _api_features.json contain that single rule, exposing it to WebUI.
Same
> for settingsPrivate.

Cool, thanks. Removed from here, and will remove for settingsPrivate in a
separate CL as well.

BTW, how does removing this impact the JS side of our browser test? Is there a
way to simulate being on the whitelisted host like chrome://settings from the
test.js so that it has access to the chrome.settingsPrivate API?

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

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:7: namespace
searchEnginesPrivate {
On 2015/04/22 22:43:51, kalman wrote:
> On 2015/04/22 22:36:11, Oren Blasberg wrote:
> > On 2015/04/22 19:32:59, kalman wrote:
> > > How will this interact with the search engine overrides API? I would have
> > > thought (?) that you may need an extension ID in here as well, if it was
> > > installed from an extension.
> > 
> > Hmm, where is that API located? I can't find it anywhere in the code (though
> it
> > sounds vaguely familiar).
> 
> It's called "chrome_settings_overrides", implemented in
> settings_overrides_handler.h.

Ah, I see. Yes, we will likely iterate on the searchEnginesPrivate API to add
more things, including things like (optional) extension ID, as well as methods
for hotword search availability, and custom search engines.

[not at google - send to devlin, 2015-04-22 23:01:26]

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/_permission_features.json:876: "whitelist": []
On 2015/04/22 22:59:19, Oren Blasberg wrote:
> On 2015/04/22 22:43:51, kalman wrote:
> > On 2015/04/22 22:36:10, Oren Blasberg wrote:
> > > On 2015/04/22 19:32:59, kalman wrote:
> > > > An empty whitelist is just parsed as no whitelist, i.e. it has no effect
> and
> > > > this means that any extension on trunk is able to call these APIs. It's
> fine
> > > > since it's trunk, but it could be a disaster if that was accidentally
> > changed
> > > > (it's also misleading like this).
> > > > 
> > > > Is the goal to have the settings app be a component extension? In that
> case,
> > > you
> > > > could put "location": "component" here instead, and then add a TODO to
> > > whitelist
> > > > the specific ID rather than all component extensions, potentially.
> > > > 
> > > > Same for settingsPrivate.
> > > 
> > > Oh wow, didn't realize that! Thanks. 
> > > 
> > > Our original plan was for the new Settings to be a Chrome app that's
bundled
> > > with the browser. Is that what you meant by "component extension"? 
> > > 
> > > However, we've decided to defer that approach due to some technical issues
> we
> > > ran into. Instead we are implementing it using just a WebUI that's hosted
at
> > > chrome://md-settings (for now, and later at chrome://settings when it's
> done).
> > > Hence the "matches" section in _api_features.json.
> > > 
> > > Given that that's the case, how do we achieve having the API only working
> from
> > > those whitelisted URLs and nowhere else?
> > 
> > Ah right. In that case you don't even need a permission at all - just have
the
> > entry in _api_features.json contain that single rule, exposing it to WebUI.
> Same
> > for settingsPrivate.
> 
> Cool, thanks. Removed from here, and will remove for settingsPrivate in a
> separate CL as well.
> 
> BTW, how does removing this impact the JS side of our browser test? Is there a
> way to simulate being on the whitelisted host like chrome://settings from the
> test.js so that it has access to the chrome.settingsPrivate API?

If your browser test is implemented as an extension... then yeah, you'll have a
problem, and you'll need to add back rules.

Of course, it's fairly easy to implement tests based in WebUI, you'll just get a
different test framework. I don't know anything about WebUI's.

[Oren Blasberg, 2015-04-22 23:39:57]

Thanks Ben; do things look good now? I'll send out an api proposal doc on apps
dev as well, and I will iterate on this API with a forthcoming CL.

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/_permission_features.json:876: "whitelist": []
On 2015/04/22 23:01:26, kalman wrote:
> On 2015/04/22 22:59:19, Oren Blasberg wrote:
> > On 2015/04/22 22:43:51, kalman wrote:
> > > On 2015/04/22 22:36:10, Oren Blasberg wrote:
> > > > On 2015/04/22 19:32:59, kalman wrote:
> > > > > An empty whitelist is just parsed as no whitelist, i.e. it has no
effect
> > and
> > > > > this means that any extension on trunk is able to call these APIs.
It's
> > fine
> > > > > since it's trunk, but it could be a disaster if that was accidentally
> > > changed
> > > > > (it's also misleading like this).
> > > > > 
> > > > > Is the goal to have the settings app be a component extension? In that
> > case,
> > > > you
> > > > > could put "location": "component" here instead, and then add a TODO to
> > > > whitelist
> > > > > the specific ID rather than all component extensions, potentially.
> > > > > 
> > > > > Same for settingsPrivate.
> > > > 
> > > > Oh wow, didn't realize that! Thanks. 
> > > > 
> > > > Our original plan was for the new Settings to be a Chrome app that's
> bundled
> > > > with the browser. Is that what you meant by "component extension"? 
> > > > 
> > > > However, we've decided to defer that approach due to some technical
issues
> > we
> > > > ran into. Instead we are implementing it using just a WebUI that's
hosted
> at
> > > > chrome://md-settings (for now, and later at chrome://settings when it's
> > done).
> > > > Hence the "matches" section in _api_features.json.
> > > > 
> > > > Given that that's the case, how do we achieve having the API only
working
> > from
> > > > those whitelisted URLs and nowhere else?
> > > 
> > > Ah right. In that case you don't even need a permission at all - just have
> the
> > > entry in _api_features.json contain that single rule, exposing it to
WebUI.
> > Same
> > > for settingsPrivate.
> > 
> > Cool, thanks. Removed from here, and will remove for settingsPrivate in a
> > separate CL as well.
> > 
> > BTW, how does removing this impact the JS side of our browser test? Is there
a
> > way to simulate being on the whitelisted host like chrome://settings from
the
> > test.js so that it has access to the chrome.settingsPrivate API?
> 
> If your browser test is implemented as an extension... then yeah, you'll have
a
> problem, and you'll need to add back rules.
> 
> Of course, it's fairly easy to implement tests based in WebUI, you'll just get
a
> different test framework. I don't know anything about WebUI's.

OK. In order to be a bit more future proof I'm going to do the location:
component approach with the TODO for now. Thanks for the tip.

[Oren Blasberg, 2015-04-23 17:05:08]

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

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:10: // The unique ID of
the engine in the list.
On 2015/04/22 19:46:30, stevenjb wrote:
> 'The unique identifier for the search engine.'

Done.

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:16: // Whether the
engine is default.
On 2015/04/22 19:46:30, stevenjb wrote:
> 'Whether the engine is the default search engine'
> (i.e. not whether or not it is in the default list, that is implicit).

Done.

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:27: static void
setDefaultSearchEngine(DOMString guid);
On 2015/04/22 19:46:30, stevenjb wrote:
> See discussion in API doc

Done.

https://codereview.chromium.org/1096143003/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:32: static void
onSearchEnginesChanged(SearchEngine[] engines);
On 2015/04/22 19:46:30, stevenjb wrote:
> See discussion in API doc

Done.

[stevenjb, 2015-04-23 17:26:01]

I only reviewed the API changes.

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

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:20:
////////////////////////////////////////////////////////////////////////////////
nit: Not sure if this was copied from somewhere, but seems overly verbose. At
miniumum I would eliminate the bottom ///////... line

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:40:
for (size_t i = 0; i < urls.size(); i++) {
nit: for (TemplateURL* url : urls)

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

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.cc:22:
: context_(context), listening_(false) {
Initialize template_url_service_

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.cc:28:
if (event_router) {
Early exit

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.cc:52:
}
no {}

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.cc:77:
for (size_t i = 0; i < urls.size(); i++) {
for (TemplateURL* url : urls)

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.h
(right):

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.h:22:
// and notify listeners on the JavaScript searchEnginesPrivate API.
nit: Clarify "observe listeners to search engines changes"

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.h:35:
SearchEnginesPrivateEventRouter() {}
Don't inline the constructor, and please initialize members.

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.h:36:
explicit SearchEnginesPrivateEventRouter(content::BrowserContext* context);
Ugh. Avoid multiple constructors. Default constructor above shouldn't be
necessary with DISALLOW_...

https://codereview.chromium.org/1096143003/diff/230001/chrome/common/extensio...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/1096143003/diff/230001/chrome/common/extensio...
chrome/common/extensions/api/_permission_features.json:875: // TODO(orenb):
Whitelist the settings app once it's an app.
nit: We will do this for all settings related APIs if/when we create a settings
app; I don't think we should add a TODO for each of them.

https://codereview.chromium.org/1096143003/diff/230001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/search_engines_private/test.js
(right):

https://codereview.chromium.org/1096143003/diff/230001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/search_engines_private/test.js:25: });
We should test the changed event also.

[not at google - send to devlin, 2015-04-23 19:42:35]

I think you'll need to update
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.h
(right):

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.h:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
Please add OWNERS for this directory.

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

https://codereview.chromium.org/1096143003/diff/230001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:17: boolean isSelected;
consider making this optional, you might as well, it's hardly ever going to be
set and no point sending down a whole bunch of "isSelected:false" all the time.

[Oren Blasberg, 2015-04-23 20:48:34]

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.h
(right):

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.h:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/04/23 19:42:35, kalman wrote:
> Please add OWNERS for this directory.

Done.

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

https://codereview.chromium.org/1096143003/diff/230001/chrome/common/extensio...
chrome/common/extensions/api/search_engines_private.idl:17: boolean isSelected;
On 2015/04/23 19:42:35, kalman wrote:
> consider making this optional, you might as well, it's hardly ever going to be
> set and no point sending down a whole bunch of "isSelected:false" all the
time.

Done.

[Oren Blasberg, 2015-04-23 20:48:54]

On 2015/04/23 19:42:35, kalman wrote:
> I think you'll need to update
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...


Done, thanks.

[Oren Blasberg, 2015-04-23 21:31:51]

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

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:20:
////////////////////////////////////////////////////////////////////////////////
On 2015/04/23 17:26:00, stevenjb wrote:
> nit: Not sure if this was copied from somewhere, but seems overly verbose. At
> miniumum I would eliminate the bottom ///////... line

Done.

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc:40:
for (size_t i = 0; i < urls.size(); i++) {
On 2015/04/23 17:26:00, stevenjb wrote:
> nit: for (TemplateURL* url : urls)

Done.

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

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.cc:22:
: context_(context), listening_(false) {
On 2015/04/23 17:26:01, stevenjb wrote:
> Initialize template_url_service_

Done.

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.cc:28:
if (event_router) {
On 2015/04/23 17:26:01, stevenjb wrote:
> Early exit

Done.

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.cc:52:
}
On 2015/04/23 17:26:01, stevenjb wrote:
> no {}

Done.

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.cc:77:
for (size_t i = 0; i < urls.size(); i++) {
On 2015/04/23 17:26:01, stevenjb wrote:
> for (TemplateURL* url : urls)

Done.

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
File
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.h
(right):

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.h:22:
// and notify listeners on the JavaScript searchEnginesPrivate API.
On 2015/04/23 17:26:01, stevenjb wrote:
> nit: Clarify "observe listeners to search engines changes"

Done.

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.h:35:
SearchEnginesPrivateEventRouter() {}
On 2015/04/23 17:26:01, stevenjb wrote:
> Don't inline the constructor, and please initialize members.

Done.

https://codereview.chromium.org/1096143003/diff/230001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.h:36:
explicit SearchEnginesPrivateEventRouter(content::BrowserContext* context);
On 2015/04/23 17:26:01, stevenjb wrote:
> Ugh. Avoid multiple constructors. Default constructor above shouldn't be
> necessary with DISALLOW_...

Done.

https://codereview.chromium.org/1096143003/diff/230001/chrome/common/extensio...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/1096143003/diff/230001/chrome/common/extensio...
chrome/common/extensions/api/_permission_features.json:875: // TODO(orenb):
Whitelist the settings app once it's an app.
On 2015/04/23 17:26:01, stevenjb wrote:
> nit: We will do this for all settings related APIs if/when we create a
settings
> app; I don't think we should add a TODO for each of them.

Done.

https://codereview.chromium.org/1096143003/diff/230001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/search_engines_private/test.js
(right):

https://codereview.chromium.org/1096143003/diff/230001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/search_engines_private/test.js:25: });
On 2015/04/23 17:26:01, stevenjb wrote:
> We should test the changed event also.

Done.

[stevenjb, 2015-04-23 23:41:41]

API code LGTM with a comment.

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

https://codereview.chromium.org/1096143003/diff/310001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.cc:23:
Profile::FromBrowserContext(context))),
I wouldn't set this twice, initializing to nullptr here is fine.

[Oren Blasberg, 2015-04-24 01:19:05]

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

https://codereview.chromium.org/1096143003/diff/310001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/search_engines_private_event_router.cc:23:
Profile::FromBrowserContext(context))),
On 2015/04/23 23:41:41, stevenjb wrote:
> I wouldn't set this twice, initializing to nullptr here is fine.

Done.

[not at google - send to devlin, 2015-04-24 16:58:03]

lgtm

https://codereview.chromium.org/1096143003/diff/370001/chrome/browser/extensi...
File chrome/browser/extensions/api/search_engines_private/OWNERS (right):

https://codereview.chromium.org/1096143003/diff/370001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/OWNERS:1:
orenb@chromium.org
what about the other people in the settings team? having a single owner can be
frustrating when you're OOO.

[Oren Blasberg, 2015-04-24 17:15:17]

https://codereview.chromium.org/1096143003/diff/370001/chrome/browser/extensi...
File chrome/browser/extensions/api/search_engines_private/OWNERS (right):

https://codereview.chromium.org/1096143003/diff/370001/chrome/browser/extensi...
chrome/browser/extensions/api/search_engines_private/OWNERS:1:
orenb@chromium.org
On 2015/04/24 16:58:02, kalman wrote:
> what about the other people in the settings team? having a single owner can be
> frustrating when you're OOO.

Good call. Done.

[Oren Blasberg, 2015-04-24 17:16:21]

The CQ bit was checked by orenb@chromium.org

[Oren Blasberg, 2015-04-24 17:16:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org, jlklein@chromium.org, jhawkins@chromium.org,
tbreisacher@chromium.org, stevenjb@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1096143003/#ps410001
(title: "Update OWNERS")

[commit-bot: I haz the power, 2015-04-24 17:17:31]

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

[commit-bot: I haz the power, 2015-04-24 18:30:08]

Committed patchset #22 (id:410001)

[commit-bot: I haz the power, 2015-04-24 18:30:59]

Patchset 22 (id:??) landed as
https://crrev.com/e733e8f9066efccf0dd05dbeaa94ed62828b22a5
Cr-Commit-Position: refs/heads/master@{#326844}