Persist browserAction settings to the extension StateStore.

Persist browserAction settings to the extension StateStore.

BUG=140293
TBR=ben@chromium.org

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

[Yoyo Zhou, 2012-08-08 01:21:06]

mostly nits

http://codereview.chromium.org/10837155/diff/10/chrome/browser/extensions/api...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

http://codereview.chromium.org/10837155/diff/10/chrome/browser/extensions/api...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:15:
#include "chrome/browser/extensions/tab_helper.h"
nit: not in sorted order

http://codereview.chromium.org/10837155/diff/10/chrome/browser/extensions/api...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:50:
base::StringToUint64(str, &value);
Why not check that this returns true?

http://codereview.chromium.org/10837155/diff/10/chrome/browser/extensions/api...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:52:
DCHECK(value == color);
This seems like overkill.

http://codereview.chromium.org/10837155/diff/10/chrome/browser/extensions/api...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:63: //
TODO(mpcomplete): Our state store can't hold binary data.
nit: rephrase this as an action you could do.

http://codereview.chromium.org/10837155/diff/10/chrome/browser/extensions/api...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:85: void
DefaultsFromValue(const base::DictionaryValue* dict,
"SetDefaultsFromValue"?

http://codereview.chromium.org/10837155/diff/10/chrome/browser/extensions/api...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:131: }
// namespace

http://codereview.chromium.org/10837155/diff/10/chrome/common/extensions/exte...
File chrome/common/extensions/extension_action.h (right):

http://codereview.chromium.org/10837155/diff/10/chrome/common/extensions/exte...
chrome/common/extensions/extension_action.h:323: // True if the
ExtensionAction's defaults have changed from what was
Isn't it that the action has changed its foobar from its defaults? You can't
change the defaults.

If so, this name doesn't totally make sense.

[Matt Perry, 2012-08-08 01:40:08]

http://codereview.chromium.org/10837155/diff/10/chrome/browser/extensions/api...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

http://codereview.chromium.org/10837155/diff/10/chrome/browser/extensions/api...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:15:
#include "chrome/browser/extensions/tab_helper.h"
On 2012/08/08 01:21:06, Yoyo Zhou wrote:
> nit: not in sorted order

Done.

http://codereview.chromium.org/10837155/diff/10/chrome/browser/extensions/api...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:50:
base::StringToUint64(str, &value);
On 2012/08/08 01:21:06, Yoyo Zhou wrote:
> Why not check that this returns true?

We'll return 0 either way.

http://codereview.chromium.org/10837155/diff/10/chrome/browser/extensions/api...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:52:
DCHECK(value == color);
On 2012/08/08 01:21:06, Yoyo Zhou wrote:
> This seems like overkill.

SkColor is 32-bit. I want to make sure the int conversion fits.

http://codereview.chromium.org/10837155/diff/10/chrome/browser/extensions/api...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:63: //
TODO(mpcomplete): Our state store can't hold binary data.
On 2012/08/08 01:21:06, Yoyo Zhou wrote:
> nit: rephrase this as an action you could do.

Done.

http://codereview.chromium.org/10837155/diff/10/chrome/browser/extensions/api...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:85: void
DefaultsFromValue(const base::DictionaryValue* dict,
On 2012/08/08 01:21:06, Yoyo Zhou wrote:
> "SetDefaultsFromValue"?

Done.

http://codereview.chromium.org/10837155/diff/10/chrome/browser/extensions/api...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:131: }
On 2012/08/08 01:21:06, Yoyo Zhou wrote:
> // namespace

Done.

http://codereview.chromium.org/10837155/diff/10/chrome/common/extensions/exte...
File chrome/common/extensions/extension_action.h (right):

http://codereview.chromium.org/10837155/diff/10/chrome/common/extensions/exte...
chrome/common/extensions/extension_action.h:323: // True if the
ExtensionAction's defaults have changed from what was
On 2012/08/08 01:21:06, Yoyo Zhou wrote:
> Isn't it that the action has changed its foobar from its defaults? You can't
> change the defaults.
> 
> If so, this name doesn't totally make sense.

"default" is sort of an overloaded term here. I'm referring to the settings for
kDefaultTabId. But I realize now that this variable doesn't actually reflect
that, so I'll just rename it to "has_changed_"

[Yoyo Zhou, 2012-08-08 06:02:59]

LGTM

[Matt Perry, 2012-08-08 22:07:25]

ben: not sure why, but presubmit wants me to add a chrome/ OWNER.

[Ben Goodger (Google), 2012-08-09 15:38:19]

Probably for common. LGTM.