Persist the extension omnibox default suggestion to Prefs.

Persist the extension omnibox default suggestion to Prefs.

BUG=123366
TEST=no

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

[Yoyo Zhou, 2012-05-04 18:50:51]

LGTM

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right):

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/api/omnibox/omnibox_api.cc:158: // This version is
used by messages from the extension.
Would it be clearer to write "comes from" instead of "is used by"? (Also at
165.)

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/api/omnibox/omnibox_api.cc:168: return false;
Also we should return false if styles is an empty list.

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/omnibox/omnibox_api.h (right):

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/api/omnibox/omnibox_api.h:83: bool Populate(const
base::DictionaryValue& value, bool require_content);
The JSON schema generator would generate a Populate function like
static bool Populate(value, suggestion*)
but you do have another parameter here. However, I think these forms aren't
different enough that one is better than the other except possibly
consistencywise.

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1024:
UpdateExtensionPref(extension_id, kOmniboxDefaultSuggestion, dict.release());
release might be unnecessary here. dict goes out of scope at the end of this
function.

[Matt Perry, 2012-05-04 20:10:11]

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right):

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/api/omnibox/omnibox_api.cc:158: // This version is
used by messages from the extension.
On 2012/05/04 18:50:52, Yoyo Zhou wrote:
> Would it be clearer to write "comes from" instead of "is used by"? (Also at
> 165.)

Done.

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/api/omnibox/omnibox_api.cc:168: return false;
On 2012/05/04 18:50:52, Yoyo Zhou wrote:
> Also we should return false if styles is an empty list.

The previous code didn't... I think it might be valid to have an empty styles
list.

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/omnibox/omnibox_api.h (right):

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/api/omnibox/omnibox_api.h:83: bool Populate(const
base::DictionaryValue& value, bool require_content);
On 2012/05/04 18:50:52, Yoyo Zhou wrote:
> The JSON schema generator would generate a Populate function like
> static bool Populate(value, suggestion*)
> but you do have another parameter here. However, I think these forms aren't
> different enough that one is better than the other except possibly
> consistencywise.

Agreed. Maybe one day someone will convert the omnibox API to use the schema
generator.

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1024:
UpdateExtensionPref(extension_id, kOmniboxDefaultSuggestion, dict.release());
On 2012/05/04 18:50:52, Yoyo Zhou wrote:
> release might be unnecessary here. dict goes out of scope at the end of this
> function.

release() transfers ownership to UpdateExtensionPref. Without it, the value
would be deleted when dict goes out of scope, with the extension pref still
holding onto the deleted value.

[commit-bot: I haz the power, 2012-05-04 20:14:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/10306015/10001

[Yoyo Zhou, 2012-05-04 20:21:23]

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right):

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/api/omnibox/omnibox_api.cc:168: return false;
On 2012/05/04 20:10:12, Matt Perry wrote:
> On 2012/05/04 18:50:52, Yoyo Zhou wrote:
> > Also we should return false if styles is an empty list.
> 
> The previous code didn't... I think it might be valid to have an empty styles
> list.

Well, if you have an empty list you'd want to push 0, NONE (which does not
happen in this case).

[Matt Perry, 2012-05-04 20:23:13]

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right):

http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/api/omnibox/omnibox_api.cc:168: return false;
On 2012/05/04 20:21:23, Yoyo Zhou wrote:
> On 2012/05/04 20:10:12, Matt Perry wrote:
> > On 2012/05/04 18:50:52, Yoyo Zhou wrote:
> > > Also we should return false if styles is an empty list.
> > 
> > The previous code didn't... I think it might be valid to have an empty
styles
> > list.
> 
> Well, if you have an empty list you'd want to push 0, NONE (which does not
> happen in this case).

Oh, good call. Thanks. And now that I look more carefully, I think you're right
that the "raw" version should always have a non-empty list.

[Yoyo Zhou, 2012-05-04 20:24:34]

On 2012/05/04 20:23:13, Matt Perry wrote:
>
http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/a...
> File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right):
> 
>
http://codereview.chromium.org/10306015/diff/6001/chrome/browser/extensions/a...
> chrome/browser/extensions/api/omnibox/omnibox_api.cc:168: return false;
> On 2012/05/04 20:21:23, Yoyo Zhou wrote:
> > On 2012/05/04 20:10:12, Matt Perry wrote:
> > > On 2012/05/04 18:50:52, Yoyo Zhou wrote:
> > > > Also we should return false if styles is an empty list.
> > > 
> > > The previous code didn't... I think it might be valid to have an empty
> styles
> > > list.
> > 
> > Well, if you have an empty list you'd want to push 0, NONE (which does not
> > happen in this case).
> 
> Oh, good call. Thanks. And now that I look more carefully, I think you're
right
> that the "raw" version should always have a non-empty list.

Right, ToValue never creating a raw version with an empty list is why I
suggested returning false (error).