Limit scope of settings API configuration and proxy permission

Limit scope of settings API configuration and proxy permission

The proxy API permission gave an extension super power: It gained complete
incognito permission. This was introduced in r210766 and is reverted here. The
reason for r210766 was that a proxy setting for regular mode was propagated
into incognito mode. This propagation was based on the assumption that
incognito mode inherits most settings from regular mode but has caused lots of
confusion and several bug reports. With this CL, extensions can only affect
settings of incognito mode if they have incognito permission.

BUG=346125, 290423
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256362

[battre, 2014-03-04 09:30:20]

Hi Bernhard,
could you please review this CL?
Thanks,
Dominic

[battre, 2014-03-04 09:45:54]

https://codereview.chromium.org/184043024/diff/1/extensions/browser/extension...
File extensions/browser/extension_pref_value_map.cc (right):

https://codereview.chromium.org/184043024/diff/1/extensions/browser/extension...
extensions/browser/extension_pref_value_map.cc:136: ext_id, context_);
This seems to create a recursive initialization...

#77 0x000055555828852e in extensions::ExtensionPrefs::Get
(context=0x33d4cc069820) at ../../extensions/browser/extension_prefs.cc:336
#78 0x0000555557fe4431 in extensions::util::IsIncognitoEnabled
(extension_id="nmmhkkegccagdldgiimedpiccmgmieda", context=0x33d4cc069820) at
../../chrome/browser/extensions/extension_util.cc:37
#79 0x0000555557f2a9f1 in
extensions::ChromeExtensionsBrowserClient::IsExtensionIncognitoEnabled
(this=0x33d4cc1747a0, extension_id="nmmhkkegccagdldgiimedpiccmgmieda",
context=0x33d4cc069820)
    at ../../chrome/browser/extensions/chrome_extensions_browser_client.cc:92
#80 0x000055555827cae7 in ExtensionPrefValueMap::RegisterExtension
(this=0x33d4cc5c6c60, ext_id="nmmhkkegccagdldgiimedpiccmgmieda",
install_time=..., is_enabled=true) at
../../extensions/browser/extension_pref_value_map.cc:135
#81 0x0000555557dfbd74 in
extensions::PreferenceAPI::InitExtensionControlledPrefs (prefs=0x33d4cd01c7a0,
value_map=0x33d4cc5c6c60) at
../../chrome/browser/extensions/api/preference/preference_api.cc:456
#82 0x00005555582916f6 in extensions::ExtensionPrefs::InitPrefStore
(this=0x33d4cd01c7a0) at ../../extensions/browser/extension_prefs.cc:1618
#83 0x0000555558291f70 in extensions::ExtensionPrefs::ExtensionPrefs
(this=0x33d4cd01c7a0, prefs=0x33d4cc278020, root_dir=...,
extension_pref_value_map=0x33d4cc5c6c60, app_sorting=(extensions::AppSorting *)
0x0, 
    time_provider=(extensions::ExtensionPrefs::TimeProvider *) 0x0,
extensions_disabled=false) at ../../extensions/browser/extension_prefs.cc:1703
#84 0x00005555582883a3 in extensions::ExtensionPrefs::Create
(pref_service=0x33d4cc278020, root_dir=...,
extension_pref_value_map=0x33d4cc5c6c60, app_sorting=(extensions::AppSorting *)
0x0, extensions_disabled=false, 
    time_provider=(extensions::ExtensionPrefs::TimeProvider *) 0x0) at
../../extensions/browser/extension_prefs.cc:323
#85 0x0000555558288277 in extensions::ExtensionPrefs::Create
(prefs=0x33d4cc278020, root_dir=..., extension_pref_value_map=0x33d4cc5c6c60,
app_sorting=..., extensions_disabled=false) at
../../extensions/browser/extension_prefs.cc:307
#86 0x000055555829542f in
extensions::ExtensionPrefsFactory::BuildServiceInstanceFor (this=0x33d4cbf29020,
context=0x33d4cc069820) at
../../extensions/browser/extension_prefs_factory.cc:47
#87 0x00007ffff71c68b5 in
BrowserContextKeyedServiceFactory::GetServiceForBrowserContext
(this=0x33d4cbf29020, context=0x33d4cc069820, create=true) at
../../components/browser_context_keyed_service/browser_context_keyed_service_factory.cc:92
#88 0x0000555558295152 in
extensions::ExtensionPrefsFactory::GetForBrowserContext (context=0x33d4cc069820)
at ../../extensions/browser/extension_prefs_factory.cc:22
#89 0x000055555828852e in extensions::ExtensionPrefs::Get
(context=0x33d4cc069820) at ../../extensions/browser/extension_prefs.cc:336


Looking into this now.

[battre, 2014-03-04 10:29:03]

https://codereview.chromium.org/184043024/diff/1/extensions/browser/extension...
File extensions/browser/extension_pref_value_map.cc (right):

https://codereview.chromium.org/184043024/diff/1/extensions/browser/extension...
extensions/browser/extension_pref_value_map.cc:136: ext_id, context_);
On 2014/03/04 09:45:54, battre wrote:
> This seems to create a recursive initialization...
> 
> #77 0x000055555828852e in extensions::ExtensionPrefs::Get
> (context=0x33d4cc069820) at ../../extensions/browser/extension_prefs.cc:336
> #78 0x0000555557fe4431 in extensions::util::IsIncognitoEnabled
> (extension_id="nmmhkkegccagdldgiimedpiccmgmieda", context=0x33d4cc069820) at
> ../../chrome/browser/extensions/extension_util.cc:37
> #79 0x0000555557f2a9f1 in
> extensions::ChromeExtensionsBrowserClient::IsExtensionIncognitoEnabled
> (this=0x33d4cc1747a0, extension_id="nmmhkkegccagdldgiimedpiccmgmieda",
> context=0x33d4cc069820)
>     at ../../chrome/browser/extensions/chrome_extensions_browser_client.cc:92
> #80 0x000055555827cae7 in ExtensionPrefValueMap::RegisterExtension
> (this=0x33d4cc5c6c60, ext_id="nmmhkkegccagdldgiimedpiccmgmieda",
> install_time=..., is_enabled=true) at
> ../../extensions/browser/extension_pref_value_map.cc:135
> #81 0x0000555557dfbd74 in
> extensions::PreferenceAPI::InitExtensionControlledPrefs (prefs=0x33d4cd01c7a0,
> value_map=0x33d4cc5c6c60) at
> ../../chrome/browser/extensions/api/preference/preference_api.cc:456
> #82 0x00005555582916f6 in extensions::ExtensionPrefs::InitPrefStore
> (this=0x33d4cd01c7a0) at ../../extensions/browser/extension_prefs.cc:1618
> #83 0x0000555558291f70 in extensions::ExtensionPrefs::ExtensionPrefs
> (this=0x33d4cd01c7a0, prefs=0x33d4cc278020, root_dir=...,
> extension_pref_value_map=0x33d4cc5c6c60, app_sorting=(extensions::AppSorting
*)
> 0x0, 
>     time_provider=(extensions::ExtensionPrefs::TimeProvider *) 0x0,
> extensions_disabled=false) at ../../extensions/browser/extension_prefs.cc:1703
> #84 0x00005555582883a3 in extensions::ExtensionPrefs::Create
> (pref_service=0x33d4cc278020, root_dir=...,
> extension_pref_value_map=0x33d4cc5c6c60, app_sorting=(extensions::AppSorting
*)
> 0x0, extensions_disabled=false, 
>     time_provider=(extensions::ExtensionPrefs::TimeProvider *) 0x0) at
> ../../extensions/browser/extension_prefs.cc:323
> #85 0x0000555558288277 in extensions::ExtensionPrefs::Create
> (prefs=0x33d4cc278020, root_dir=..., extension_pref_value_map=0x33d4cc5c6c60,
> app_sorting=..., extensions_disabled=false) at
> ../../extensions/browser/extension_prefs.cc:307
> #86 0x000055555829542f in
> extensions::ExtensionPrefsFactory::BuildServiceInstanceFor
(this=0x33d4cbf29020,
> context=0x33d4cc069820) at
> ../../extensions/browser/extension_prefs_factory.cc:47
> #87 0x00007ffff71c68b5 in
> BrowserContextKeyedServiceFactory::GetServiceForBrowserContext
> (this=0x33d4cbf29020, context=0x33d4cc069820, create=true) at
>
../../components/browser_context_keyed_service/browser_context_keyed_service_factory.cc:92
> #88 0x0000555558295152 in
> extensions::ExtensionPrefsFactory::GetForBrowserContext
(context=0x33d4cc069820)
> at ../../extensions/browser/extension_prefs_factory.cc:22
> #89 0x000055555828852e in extensions::ExtensionPrefs::Get
> (context=0x33d4cc069820) at ../../extensions/browser/extension_prefs.cc:336
> 
> 
> Looking into this now.

Done. I'll point the extensions team reviewer to this to check whether the new
implementation is ok for them.

[battre, 2014-03-04 17:22:49]

+kalman.

Could you please take a look at the extensions related parts? In particular you
may want to take a look at the difference between patchset 1 and 2 where I
changed how I determine whether an extension has incognito mode permission. I
think that the current approach should be fine but you or somebody on your team
may have more insight into this.

Thanks,
Dominic

[Bernhard Bauer, 2014-03-04 17:31:18]

https://codereview.chromium.org/184043024/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/types.json (right):

https://codereview.chromium.org/184043024/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/types.json:49: "description": "One
of<ul><li><var>not_controllable</var>: cannot be controlled by this extension
due to lack of permission or cannot be controlled by any
extension</li><li><var>controlled_by_other_extensions</var>: controlled by
extensions with higher
precedence</li><li><var>controllable_by_this_extension</var>: can be controlled
by this extension</li><li><var>controlled_by_this_extension</var>: controlled by
this extension</li></ul>",
Hm... if an extension doesn't have incognito permissions, should we even allow
reading the proxy settings for incognito? And in that case, it shouldn't happen
that we will tell the extension that it can't change the value because it
doesn't have incognito permissions.

https://codereview.chromium.org/184043024/diff/40001/extensions/browser/exten...
File extensions/browser/extension_pref_value_map.cc (right):

https://codereview.chromium.org/184043024/diff/40001/extensions/browser/exten...
extensions/browser/extension_pref_value_map.cc:275: // preference.
Wait, this method is called with incognito == true to find the effective value
for incognito mode, right? So, if we fall back to the regular preference here,
doesn't that mean that the regular preference will be effective for incognito
windows?

Hmm... Is this branch even reachable? Below you have a check that skips an
extension if we want an incognito value but the extension doesn't have incognito
permissions.

https://codereview.chromium.org/184043024/diff/40001/extensions/browser/exten...
File extensions/browser/extension_pref_value_map_unittest.cc (right):

https://codereview.chromium.org/184043024/diff/40001/extensions/browser/exten...
extensions/browser/extension_pref_value_map_unittest.cc:48: false;  // Extension
has no incognito permission by default.
Nit: I would rather keep the code on a single line, then wrap the comment to fit
(or move it to the line above).

https://codereview.chromium.org/184043024/diff/40001/extensions/browser/exten...
extensions/browser/extension_pref_value_map_unittest.cc:84:
epvm_.RegisterExtension(kExt1, CreateTime(10), kEnabled, kNoIncognito);
Do you ever call this method with different values? If not, would it make sense
to add a convenience method on the test fixture?

https://codereview.chromium.org/184043024/diff/40001/extensions/browser/exten...
extensions/browser/extension_pref_value_map_unittest.cc:141: // Check that value
is not propagated until extension gets incognito
Nit: "the value" and "the extension".

https://codereview.chromium.org/184043024/diff/40001/extensions/browser/exten...
extensions/browser/extension_pref_value_map_unittest.cc:143: EXPECT_EQ("",
GetValue(kPref1, true));
Use std::string() instead of "".

[battre, 2014-03-05 08:34:35]

Please take another look.

https://codereview.chromium.org/184043024/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/types.json (right):

https://codereview.chromium.org/184043024/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/types.json:49: "description": "One
of<ul><li><var>not_controllable</var>: cannot be controlled by this extension
due to lack of permission or cannot be controlled by any
extension</li><li><var>controlled_by_other_extensions</var>: controlled by
extensions with higher
precedence</li><li><var>controllable_by_this_extension</var>: can be controlled
by this extension</li><li><var>controlled_by_this_extension</var>: controlled by
this extension</li></ul>",
On 2014/03/04 17:31:18, Bernhard Bauer wrote:
> Hm... if an extension doesn't have incognito permissions, should we even allow
> reading the proxy settings for incognito? And in that case, it shouldn't
happen
> that we will tell the extension that it can't change the value because it
> doesn't have incognito permissions.

Yes, you are right. In case the extension does not have incognito permission, we
don't allow reading the value. I have reverted the string. Good catch.

Done.

https://codereview.chromium.org/184043024/diff/40001/extensions/browser/exten...
File extensions/browser/extension_pref_value_map.cc (right):

https://codereview.chromium.org/184043024/diff/40001/extensions/browser/exten...
extensions/browser/extension_pref_value_map.cc:275: // preference.
On 2014/03/04 17:31:18, Bernhard Bauer wrote:
> Wait, this method is called with incognito == true to find the effective value
> for incognito mode, right? So, if we fall back to the regular preference here,
> doesn't that mean that the regular preference will be effective for incognito
> windows?
> 
> Hmm... Is this branch even reachable? Below you have a check that skips an
> extension if we want an incognito value but the extension doesn't have
incognito
> permissions.

Done.

https://codereview.chromium.org/184043024/diff/40001/extensions/browser/exten...
File extensions/browser/extension_pref_value_map_unittest.cc (right):

https://codereview.chromium.org/184043024/diff/40001/extensions/browser/exten...
extensions/browser/extension_pref_value_map_unittest.cc:48: false;  // Extension
has no incognito permission by default.
On 2014/03/04 17:31:18, Bernhard Bauer wrote:
> Nit: I would rather keep the code on a single line, then wrap the comment to
fit
> (or move it to the line above).

I have removed these lines.

https://codereview.chromium.org/184043024/diff/40001/extensions/browser/exten...
extensions/browser/extension_pref_value_map_unittest.cc:84:
epvm_.RegisterExtension(kExt1, CreateTime(10), kEnabled, kNoIncognito);
On 2014/03/04 17:31:18, Bernhard Bauer wrote:
> Do you ever call this method with different values? If not, would it make
sense
> to add a convenience method on the test fixture?

Done.

https://codereview.chromium.org/184043024/diff/40001/extensions/browser/exten...
extensions/browser/extension_pref_value_map_unittest.cc:141: // Check that value
is not propagated until extension gets incognito
On 2014/03/04 17:31:18, Bernhard Bauer wrote:
> Nit: "the value" and "the extension".

Done.

https://codereview.chromium.org/184043024/diff/40001/extensions/browser/exten...
extensions/browser/extension_pref_value_map_unittest.cc:143: EXPECT_EQ("",
GetValue(kPref1, true));
On 2014/03/04 17:31:18, Bernhard Bauer wrote:
> Use std::string() instead of "".

Done.

[Bernhard Bauer, 2014-03-05 09:05:34]

https://codereview.chromium.org/184043024/diff/100001/extensions/browser/exte...
File extensions/browser/extension_pref_value_map.cc (right):

https://codereview.chromium.org/184043024/diff/100001/extensions/browser/exte...
extensions/browser/extension_pref_value_map.cc:255: NOTREACHED();
You are handling a DCHECK failure here, which you're not supposed to do (see
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-).
Seeing as you already have a branch for incognito below, could you just add a
DCHECK(winner->second->incognito_enabled) in there?

[battre, 2014-03-05 12:05:44]

Thanks.

https://codereview.chromium.org/184043024/diff/100001/extensions/browser/exte...
File extensions/browser/extension_pref_value_map.cc (right):

https://codereview.chromium.org/184043024/diff/100001/extensions/browser/exte...
extensions/browser/extension_pref_value_map.cc:255: NOTREACHED();
On 2014/03/05 09:05:35, Bernhard Bauer wrote:
> You are handling a DCHECK failure here, which you're not supposed to do (see
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-).
> Seeing as you already have a branch for incognito below, could you just add a
> DCHECK(winner->second->incognito_enabled) in there?

Done.

[Bernhard Bauer, 2014-03-05 12:18:19]

LGTM!

[not at google - send to devlin, 2014-03-05 17:14:05]

lgtm but I realised by the end of this that I'm not confident I can claim to
understand all of the interactions that go on here.

So +yoz for a second opinion.

https://codereview.chromium.org/184043024/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/preference/preference_api.cc (right):

https://codereview.chromium.org/184043024/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/preference/preference_api.cc:456: bool
is_incognito_enabled = prefs->IsIncognitoEnabled(*extension_id);
Perhaps you should be using util::IsIncognitoEnabled:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

https://codereview.chromium.org/184043024/diff/120001/extensions/browser/exte...
File extensions/browser/extension_pref_value_map.cc (right):

https://codereview.chromium.org/184043024/diff/120001/extensions/browser/exte...
extensions/browser/extension_pref_value_map.cc:177:
NotifyPrefValueChanged(keys);
it would be kinda nice to factor this common logic sensibly with
SetExtensionState, but that may be more than it's worth

[battre, 2014-03-05 17:38:43]

https://codereview.chromium.org/184043024/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/preference/preference_api.cc (right):

https://codereview.chromium.org/184043024/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/preference/preference_api.cc:456: bool
is_incognito_enabled = prefs->IsIncognitoEnabled(*extension_id);
On 2014/03/05 17:14:06, kalman wrote:
> Perhaps you should be using util::IsIncognitoEnabled:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

Please take a look at the first version of this CL to see how I tried and failed
with this.

[battre, 2014-03-10 13:50:11]

Yoyo, could you please take a look at this?

Thanks,
Dominic

https://codereview.chromium.org/184043024/diff/120001/extensions/browser/exte...
File extensions/browser/extension_pref_value_map.cc (right):

https://codereview.chromium.org/184043024/diff/120001/extensions/browser/exte...
extensions/browser/extension_pref_value_map.cc:177:
NotifyPrefValueChanged(keys);
On 2014/03/05 17:14:06, kalman wrote:
> it would be kinda nice to factor this common logic sensibly with
> SetExtensionState, but that may be more than it's worth

I see two different ways of achieving this:

1)

void ExtensionPrefValueMap::SetExtensionStateHelper(ExtensionEntry* extension,
                                                    bool* state,
                                                    bool new_state) {
  if (*state == new_state)
    return;
  std::set<std::string> keys;  // keys set by this extension
  GetExtensionControlledKeys(*extension, &keys);
  *state = new_state;
  NotifyPrefValueChanged(keys);
}

void ExtensionPrefValueMap::SetExtensionState(const std::string& ext_id,
                                              bool is_enabled) {
  ExtensionEntryMap::const_iterator i = entries_.find(ext_id);
  // This may happen when sync sets the extension state for an
  // extension that is not installed.
  if (i == entries_.end())
    return;
  SetExtensionStateHelper(i->second, &i->second->enabled, is_enabled);
}

void ExtensionPrefValueMap::SetExtensionIncognitoState(
    const std::string& ext_id,
    bool is_incognito_enabled) {
  ExtensionEntryMap::const_iterator i = entries_.find(ext_id);
  // This may happen when sync sets the extension state for an
  // extension that is not installed.
  if (i == entries_.end())
    return;
  SetExtensionStateHelper(
      i->second, &i->second->incognito_enabled, is_incognito_enabled);
}


2)

void ExtensionPrefValueMap::SetExtensionStateHelper(const std::string& ext_id,
                                                    bool incognito,
                                                    bool new_state) {
  ExtensionEntryMap::const_iterator i = entries_.find(ext_id);
  // This may happen when sync sets the extension state for an
  // extension that is not installed.
  if (i == entries_.end())
    return;
  bool old_state =
      incognito ? i->second->incognito_enabled : i->second->enabled;
  if (old_state == new_state)
    return;
  std::set<std::string> keys;  // keys set by this extension
  GetExtensionControlledKeys(*(i->second), &keys);
  if (incognito)
    i->second->incognito_enabled = new_state;
  else
    i->second->enabled = new_state;
  NotifyPrefValueChanged(keys);
}

void ExtensionPrefValueMap::SetExtensionState(const std::string& ext_id,
                                              bool is_enabled) {
  SetExtensionStateHelper(ext_id, false, is_enabled);
}

void ExtensionPrefValueMap::SetExtensionIncognitoState(
    const std::string& ext_id,
    bool is_incognito_enabled) {
  SetExtensionStateHelper(ext_id, true, is_incognito_enabled);
}

My feeling is that neither is significantly better. WDYT?

[Yoyo Zhou, 2014-03-10 22:33:25]

LGTM

https://codereview.chromium.org/184043024/diff/120001/extensions/browser/exte...
File extensions/browser/extension_pref_value_map.cc (right):

https://codereview.chromium.org/184043024/diff/120001/extensions/browser/exte...
extensions/browser/extension_pref_value_map.cc:177:
NotifyPrefValueChanged(keys);
On 2014/03/10 13:50:11, battre wrote:
> On 2014/03/05 17:14:06, kalman wrote:
> > it would be kinda nice to factor this common logic sensibly with
> > SetExtensionState, but that may be more than it's worth
> 
> I see two different ways of achieving this:
> 
> 1)
> 
> void ExtensionPrefValueMap::SetExtensionStateHelper(ExtensionEntry* extension,
>                                                     bool* state,
>                                                     bool new_state) {
>   if (*state == new_state)
>     return;
>   std::set<std::string> keys;  // keys set by this extension
>   GetExtensionControlledKeys(*extension, &keys);
>   *state = new_state;
>   NotifyPrefValueChanged(keys);
> }
> 
> void ExtensionPrefValueMap::SetExtensionState(const std::string& ext_id,
>                                               bool is_enabled) {
>   ExtensionEntryMap::const_iterator i = entries_.find(ext_id);
>   // This may happen when sync sets the extension state for an
>   // extension that is not installed.
>   if (i == entries_.end())
>     return;
>   SetExtensionStateHelper(i->second, &i->second->enabled, is_enabled);
> }
> 
> void ExtensionPrefValueMap::SetExtensionIncognitoState(
>     const std::string& ext_id,
>     bool is_incognito_enabled) {
>   ExtensionEntryMap::const_iterator i = entries_.find(ext_id);
>   // This may happen when sync sets the extension state for an
>   // extension that is not installed.
>   if (i == entries_.end())
>     return;
>   SetExtensionStateHelper(
>       i->second, &i->second->incognito_enabled, is_incognito_enabled);
> }
> 
> 
> 2)
> 
> void ExtensionPrefValueMap::SetExtensionStateHelper(const std::string& ext_id,
>                                                     bool incognito,
>                                                     bool new_state) {
>   ExtensionEntryMap::const_iterator i = entries_.find(ext_id);
>   // This may happen when sync sets the extension state for an
>   // extension that is not installed.
>   if (i == entries_.end())
>     return;
>   bool old_state =
>       incognito ? i->second->incognito_enabled : i->second->enabled;
>   if (old_state == new_state)
>     return;
>   std::set<std::string> keys;  // keys set by this extension
>   GetExtensionControlledKeys(*(i->second), &keys);
>   if (incognito)
>     i->second->incognito_enabled = new_state;
>   else
>     i->second->enabled = new_state;
>   NotifyPrefValueChanged(keys);
> }
> 
> void ExtensionPrefValueMap::SetExtensionState(const std::string& ext_id,
>                                               bool is_enabled) {
>   SetExtensionStateHelper(ext_id, false, is_enabled);
> }
> 
> void ExtensionPrefValueMap::SetExtensionIncognitoState(
>     const std::string& ext_id,
>     bool is_incognito_enabled) {
>   SetExtensionStateHelper(ext_id, true, is_incognito_enabled);
> }
> 
> My feeling is that neither is significantly better. WDYT?

Yes, I don't see either of these as an improvement.

At core, the logic is
1. find extension
2. apply change and see if it's different, return if not
3. notify changes for keys

2. would be easiest as a lambda, but C++ isn't the right language for it. I
think it's okay as is.

[battre, 2014-03-11 16:19:47]

The CQ bit was checked by battre@chromium.org

[commit-bot: I haz the power, 2014-03-11 16:29:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/184043024/120001

[commit-bot: I haz the power, 2014-03-12 00:29:18]

Change committed as 256362