[Extensions] Fixed removing from blacklist.

[Extensions] Fixed removing from blacklist.

1. Moved SetExtensionBlacklisted to private section and replace all
occurrences to SetExtensionBlacklistState.
2. Added ExtensionServiceTest.RemoveExtensionFromBlacklist test.

BUG=

Committed: https://crrev.com/6dc7c442ec4091655accee856d239f2cfdfc0e41
Cr-Commit-Position: refs/heads/master@{#406509}

[atuchin, 2016-07-13 12:17:35]

Description was changed from

==========
[Extensions] Fixed removing from blacklist.

1. Moved SetExtensionBlacklisted to private section and replace all
occurrences to SetExtensionBlacklistState.
2. Added ExtensionServiceTest.RemoveExtensionFromBlacklist test.

BUG=
==========

to

==========
[Extensions] Fixed removing from blacklist.

1. Moved SetExtensionBlacklisted to private section and replace all
occurrences to SetExtensionBlacklistState.
2. Added ExtensionServiceTest.RemoveExtensionFromBlacklist test.

BUG=
==========

[atuchin, 2016-07-13 12:17:35]

atuchin@yandex-team.ru changed reviewers:
+ asargent@chromium.org, finnur@chromium.org

[atuchin, 2016-07-13 12:18:21]

On 2016/07/13 12:17:35, atuchin wrote:
> mailto:atuchin@yandex-team.ru changed reviewers:
> + mailto:asargent@chromium.org, mailto:finnur@chromium.org

Hi!
I found a bug in an ExtensionService.
Let’s assume that a user had some blacklisted extension, marked as
BLACKLISTED_MALWARE. Then extensions blacklist was updated, and the new version
didn’t contain the extension. Therefore the browser should enable the extension
and cleans blacklisted prefs.

But something was wrong: ExtensionService::UpdateBlacklistedExtensions calls old
SetExtensionBlacklisted method instead of new SetExtensionBlacklistState. The
result is that only “blacked” prefs was cleaned. And “blacklist_state” was left.

It looks like a minor problem but it isn’t if a user enable sync.
CreateSyncData looks to GetExtensionBlacklistState(..) == NOT_BLACKLISTED and
set enabled=false in ExtensionSyncData. The result is that the extension is
disabled on each browser start. 

I have prepared a patch to fix the problem:
1) Changed SetExtensionBlacklisted to SetExtensionBlacklistState in
extension_service.cc and unit_tests.
2) Moved ExtensionPrefs::SetExtensionBlacklisted to private section.
3) Added a unit test to verify that both of “blacklist” and “blacklist_state”
are clear after an extension is removed from blacklist. 

I also hope that old "blacklist" preference will be removed in future releases. 

Best regards, Mikhail.

[atuchin, 2016-07-14 06:58:16]

atuchin@yandex-team.ru changed reviewers:
+ benwells@chromium.org, rdevlin.cronin@chromium.org
- finnur@chromium.org

[atuchin, 2016-07-18 10:33:19]

On 2016/07/13 12:18:21, atuchin wrote:
> On 2016/07/13 12:17:35, atuchin wrote:
> > mailto:atuchin@yandex-team.ru changed reviewers:
> > + mailto:asargent@chromium.org, mailto:finnur@chromium.org
> 
> Hi!
> I found a bug in an ExtensionService.
> Let’s assume that a user had some blacklisted extension, marked as
> BLACKLISTED_MALWARE. Then extensions blacklist was updated, and the new
version
> didn’t contain the extension. Therefore the browser should enable the
extension
> and cleans blacklisted prefs.
> 
> But something was wrong: ExtensionService::UpdateBlacklistedExtensions calls
old
> SetExtensionBlacklisted method instead of new SetExtensionBlacklistState. The
> result is that only “blacked” prefs was cleaned. And “blacklist_state” was
left.
> 
> It looks like a minor problem but it isn’t if a user enable sync.
> CreateSyncData looks to GetExtensionBlacklistState(..) == NOT_BLACKLISTED and
> set enabled=false in ExtensionSyncData. The result is that the extension is
> disabled on each browser start. 
> 
> I have prepared a patch to fix the problem:
> 1) Changed SetExtensionBlacklisted to SetExtensionBlacklistState in
> extension_service.cc and unit_tests.
> 2) Moved ExtensionPrefs::SetExtensionBlacklisted to private section.
> 3) Added a unit test to verify that both of “blacklist” and “blacklist_state”
> are clear after an extension is removed from blacklist. 
> 
> I also hope that old "blacklist" preference will be removed in future
releases. 
> 
> Best regards, Mikhail.

Ping) Any feedback?

[Devlin, 2016-07-18 16:22:17]

This looks pretty good, thanks for the patch!  Mostly just a couple of nits, and
I'm running it through a few trybots to check that it doesn't break anything
unexpected.

https://codereview.chromium.org/2147923002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/2147923002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_unittest.cc:3125: // from blacklist.
Also checks that all blacklisted preferences will be cleaned
nit: from *the* blacklist

https://codereview.chromium.org/2147923002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_unittest.cc:3138: // Add the
extension from blacklist.
nit: s/from/to the

https://codereview.chromium.org/2147923002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_unittest.cc:3156: // should be
cleaned.
s/cleaned/cleared

https://codereview.chromium.org/2147923002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_prefs.h (right):

https://codereview.chromium.org/2147923002/diff/1/extensions/browser/extensio...
extensions/browser/extension_prefs.h:587: // Sets whether the extension with
|id| is blacklisted.
Can you add here:
// DEPRECATED. Use GetExtensionBlacklistState().
// TODO(atuchin): Remove this once all clients are updated.

(Feel free to put devlin instead of atuchin if you want.)

[Devlin, 2016-07-18 16:23:05]

rdevlin.cronin@chromium.org changed reviewers:
- asargent@chromium.org, benwells@chromium.org

[Devlin, 2016-07-18 16:23:06]

(Since I got to this first, moving benwells@ and asargent@ to cc. Note that
usually one extension reviewer is enough. :))

[atuchin, 2016-07-19 06:00:48]

On 2016/07/18 16:23:06, Devlin wrote:
> (Since I got to this first, moving benwells@ and asargent@ to cc. Note that
> usually one extension reviewer is enough. :))
Hi, Devlin.
Thanks for answer.
I fixed the couple of nits, thanks, see Patch Set #2.

About the number of extension reviewers - I've got it, the time I will add only
one=)
Best regards, Mikhail.

[atuchin, 2016-07-19 06:03:29]

On 2016/07/19 06:00:48, atuchin wrote:
> On 2016/07/18 16:23:06, Devlin wrote:
> > (Since I got to this first, moving benwells@ and asargent@ to cc. Note that
> > usually one extension reviewer is enough. :))
> Hi, Devlin.
> Thanks for answer.
> I fixed the couple of nits, thanks, see Patch Set #2.
> 
> About the number of extension reviewers - I've got it, the time I will add
only
> one=)
> Best regards, Mikhail.
the time -> the next time

[Devlin, 2016-07-19 16:02:45]

lgtm, thanks!

There were a few last nits that I missed on the first round, but this looks
great.

https://codereview.chromium.org/2147923002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/blacklist_unittest.cc (right):

https://codereview.chromium.org/2147923002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/blacklist_unittest.cc:127:
prefs()->SetExtensionBlacklistState(a, extensions::BLACKLISTED_MALWARE);
This file is in the extensions namespace, so the extensions:: prefix here is
unnecessary.

https://codereview.chromium.org/2147923002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_error_controller_unittest.cc (right):

https://codereview.chromium.org/2147923002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_error_controller_unittest.cc:134:
extensions::BLACKLISTED_MALWARE);
ditto re extensions:: prefix

https://codereview.chromium.org/2147923002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

https://codereview.chromium.org/2147923002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_prefs_unittest.cc:855:
extensions::BLACKLISTED_MALWARE);
here, too

[atuchin, 2016-07-20 06:02:46]

Thanks!
I removed unnecessary extensions:: namespace.

https://codereview.chromium.org/2147923002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/blacklist_unittest.cc (right):

https://codereview.chromium.org/2147923002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/blacklist_unittest.cc:127:
prefs()->SetExtensionBlacklistState(a, extensions::BLACKLISTED_MALWARE);
On 2016/07/19 16:02:44, Devlin wrote:
> This file is in the extensions namespace, so the extensions:: prefix here is
> unnecessary.

Done.

https://codereview.chromium.org/2147923002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_error_controller_unittest.cc (right):

https://codereview.chromium.org/2147923002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_error_controller_unittest.cc:134:
extensions::BLACKLISTED_MALWARE);
On 2016/07/19 16:02:44, Devlin wrote:
> ditto re extensions:: prefix

Done.

https://codereview.chromium.org/2147923002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

https://codereview.chromium.org/2147923002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_prefs_unittest.cc:855:
extensions::BLACKLISTED_MALWARE);
On 2016/07/19 16:02:44, Devlin wrote:
> here, too

Done.

[atuchin, 2016-07-20 06:24:05]

The CQ bit was checked by atuchin@yandex-team.ru

[atuchin, 2016-07-20 06:24:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2147923002/#ps40001
(title: "Unnecessary extensions:: namespace is removed.")

[commit-bot: I haz the power, 2016-07-20 06:24:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 07:04:49]

Description was changed from

==========
[Extensions] Fixed removing from blacklist.

1. Moved SetExtensionBlacklisted to private section and replace all
occurrences to SetExtensionBlacklistState.
2. Added ExtensionServiceTest.RemoveExtensionFromBlacklist test.

BUG=
==========

to

==========
[Extensions] Fixed removing from blacklist.

1. Moved SetExtensionBlacklisted to private section and replace all
occurrences to SetExtensionBlacklistState.
2. Added ExtensionServiceTest.RemoveExtensionFromBlacklist test.

BUG=
==========

[commit-bot: I haz the power, 2016-07-20 07:04:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-20 07:04:59]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 07:06:24]

Description was changed from

==========
[Extensions] Fixed removing from blacklist.

1. Moved SetExtensionBlacklisted to private section and replace all
occurrences to SetExtensionBlacklistState.
2. Added ExtensionServiceTest.RemoveExtensionFromBlacklist test.

BUG=
==========

to

==========
[Extensions] Fixed removing from blacklist.

1. Moved SetExtensionBlacklisted to private section and replace all
occurrences to SetExtensionBlacklistState.
2. Added ExtensionServiceTest.RemoveExtensionFromBlacklist test.

BUG=

Committed: https://crrev.com/6dc7c442ec4091655accee856d239f2cfdfc0e41
Cr-Commit-Position: refs/heads/master@{#406509}
==========

[commit-bot: I haz the power, 2016-07-20 07:06:25]

Patchset 3 (id:??) landed as
https://crrev.com/6dc7c442ec4091655accee856d239f2cfdfc0e41
Cr-Commit-Position: refs/heads/master@{#406509}