Enable/disable extensions upon changes in blacklist.

Enable/disable extensions upon changes in blacklist.

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

[Oleg Eterevsky, 2014-01-21 17:43:54]

I'm still working on additional unit tests, but all the main things are in
place. Please have a look.

[not at google - send to devlin, 2014-01-22 00:33:14]

mostly lg, some structural/naming comments, will look at unit tests when ready.

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
File chrome/browser/extensions/extension_prefs.h (right):

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
chrome/browser/extensions/extension_prefs.h:250: // Gets the set of extensions
that have been blacklisted in prefs.
you might want to add a comment here and below explaining what "blacklisted"
actually means now that we have a greylist too.

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
chrome/browser/extensions/extension_service.cc:2783: std::set<std::string>
blocked;
(following on from comment in header file) personally I prefer the term
"malware" here since it's more specific than just "blocked"... term is a bit
abstract to me... but not a big deal.

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
chrome/browser/extensions/extension_service.cc:2785: std::set<std::string>
unknown;
nit: could you call this "ignore" or "skip" everywhere rather than "unknown"?
I'm finding the code hard to parse as "unknown", it's too overloaded a term.

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
chrome/browser/extensions/extension_service.cc:2868: void
ExtensionService::UpdateGreylistedExtensions(
seems like this method is basically the same as UpdateBlockedExtensions just
dealing with different sets, and with UMA logging above (should you be doing UMA
logging here too?).

it's probably too contrived to actually make them share all of the code, but you
should at least be able to pull out the set division (lines 2871 - 2882) into a
sensible common place.

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
File chrome/browser/extensions/extension_service.h (right):

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
chrome/browser/extensions/extension_service.h:709: // Add extensions in
|malware| to blacklisted_extensions, remove extensions
s/malware/blocked? or s/blocked/malware?

https://chromiumcodereview.appspot.com/98463005/diff/110001/extensions/common...
File extensions/common/extension.h (right):

https://chromiumcodereview.appspot.com/98463005/diff/110001/extensions/common...
extensions/common/extension.h:96: 
nit: no blank line here, remove blank line at 93.

[Oleg Eterevsky, 2014-01-22 21:25:04]

I added a couple of unit tests, slightly changed the logic for
enabling/disabling, store blacklist state in prefs, show the disable reason on
chrome://extensions.

Please have a look!

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
File chrome/browser/extensions/extension_prefs.h (right):

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
chrome/browser/extensions/extension_prefs.h:250: // Gets the set of extensions
that have been blacklisted in prefs.
On 2014/01/22 00:33:14, kalman wrote:
> you might want to add a comment here and below explaining what "blacklisted"
> actually means now that we have a greylist too.

Actually, no blacklisted here will returned only malware. But I'll add comment.

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
chrome/browser/extensions/extension_service.cc:2783: std::set<std::string>
blocked;
On 2014/01/22 00:33:14, kalman wrote:
> (following on from comment in header file) personally I prefer the term
> "malware" here since it's more specific than just "blocked"... term is a bit
> abstract to me... but not a big deal.

I think we should fix the terminology everywhere, but it will touch so many
files, that I'd prefer to do it in a separate CL. How about:

blacklist -- refer to Blacklist with all the possible states,

malware, potentially unwanted, ... -- refer to the reason for the action we
take, but not to the actual categories of extensions in service and
ExtensionRegistry

blocked -- what was formerly blacklisted. If you can come up with a better term,
I'm willing to change

greylist -- disabled once

This way, if we decide to change how we treat some type of extensions (like, we
decide to permanently block extensions violating web store policy), we'll just
change a could of switches and ifs.

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
chrome/browser/extensions/extension_service.cc:2785: std::set<std::string>
unknown;
On 2014/01/22 00:33:14, kalman wrote:
> nit: could you call this "ignore" or "skip" everywhere rather than "unknown"?
> I'm finding the code hard to parse as "unknown", it's too overloaded a term.

Changed to 'unchanged'.

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
chrome/browser/extensions/extension_service.cc:2868: void
ExtensionService::UpdateGreylistedExtensions(
On 2014/01/22 00:33:14, kalman wrote:
> seems like this method is basically the same as UpdateBlockedExtensions just
> dealing with different sets, and with UMA logging above (should you be doing
UMA
> logging here too?).

I'm not sure what exactly to log. Is it ok if I add a todo, and do it in a
separate CL?

> it's probably too contrived to actually make them share all of the code, but
you
> should at least be able to pull out the set division (lines 2871 - 2882) into
a
> sensible common place.

Well, if there were just one resulting set, it would simplify the code, but I
feel that in the current state an extra function will not be much help. If you
wish, I can add STLSetUnion to stl_unil add use it.

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
File chrome/browser/extensions/extension_service.h (right):

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
chrome/browser/extensions/extension_service.h:709: // Add extensions in
|malware| to blacklisted_extensions, remove extensions
On 2014/01/22 00:33:14, kalman wrote:
> s/malware/blocked? or s/blocked/malware?

Done.

[not at google - send to devlin, 2014-01-22 22:24:38]

lgtm with some comments most notably the Partition one. I think it would be a
decent improvement (whatever it should be called).

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
chrome/browser/extensions/extension_service.cc:2783: std::set<std::string>
blocked;
On 2014/01/22 21:25:04, Oleg wrote:
> On 2014/01/22 00:33:14, kalman wrote:
> > (following on from comment in header file) personally I prefer the term
> > "malware" here since it's more specific than just "blocked"... term is a bit
> > abstract to me... but not a big deal.
> 
> I think we should fix the terminology everywhere, but it will touch so many
> files, that I'd prefer to do it in a separate CL. How about:
> 
> blacklist -- refer to Blacklist with all the possible states,

I think this is actually the thing that needs to change. And I think based on
your comment you agree? Blacklist is no longer the right term for that class; to
me a blacklist really does mean a single list of things that are never allowed;
pulling a greylist out of that is odd.

As you say we can worry about that / discuss naming later.

> 
> malware, potentially unwanted, ... -- refer to the reason for the action we
> take, but not to the actual categories of extensions in service and
> ExtensionRegistry
> 
> blocked -- what was formerly blacklisted. If you can come up with a better
term,
> I'm willing to change

I'm ok leaving this as blocked for now, but I'd ideally like to see this back as
blacklisted (see above comment).

> 
> greylist -- disabled once
> 
> This way, if we decide to change how we treat some type of extensions (like,
we
> decide to permanently block extensions violating web store policy), we'll just
> change a could of switches and ifs.

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
chrome/browser/extensions/extension_service.cc:2868: void
ExtensionService::UpdateGreylistedExtensions(
On 2014/01/22 21:25:04, Oleg wrote:
> On 2014/01/22 00:33:14, kalman wrote:
> > seems like this method is basically the same as UpdateBlockedExtensions just
> > dealing with different sets, and with UMA logging above (should you be doing
> UMA
> > logging here too?).
> 
> I'm not sure what exactly to log. Is it ok if I add a todo, and do it in a
> separate CL?

I suppose you might as well do it in a follow up. I just mean to say that
tracking this stuff in UMA would be useful, and it occurs to me that greylisting
is even more subtle than blacklisting. How often are extensions disabled from
the greylist? How often are they re-enabled by the user? Etc. Based on that a
follow-up SGTM.

> 
> > it's probably too contrived to actually make them share all of the code, but
> you
> > should at least be able to pull out the set division (lines 2871 - 2882)
into
> a
> > sensible common place.
> 
> Well, if there were just one resulting set, it would simplify the code, but I
> feel that in the current state an extra function will not be much help. If you
> wish, I can add STLSetUnion to stl_unil add use it.

I'm imagining something like:

void Partition(const ExtensionIdSet& before,
               const ExtensionIdSet& after,
               const ExtensionIdSet& unchanged,
               ExtensionIdSet* no_longer,
               ExtensionIdSet* not_yet) {
  ExtensionIdSet before_and_unchanged;
  std::set_union(...);
  *no_longer = base::STLSetDifference(...);
  *not_yet = base::STLSetDifference(...);
}

void UpdateGreylistedExtensions(const ExtensionIdSet& greylist,
                                const ExtensionIdSet& unchanged) {
  ExtensionIdSet not_yet, no_longer;
  Partition(greylist_.GetIDs(), greylist, unchanged,
            &not_yet, &no_longer);
}

https://chromiumcodereview.appspot.com/98463005/diff/190001/chrome/browser/ex...
File chrome/browser/extensions/extension_prefs.cc (right):

https://chromiumcodereview.appspot.com/98463005/diff/190001/chrome/browser/ex...
chrome/browser/extensions/extension_prefs.cc:1166: if (ext_prefs &&
ext_prefs->GetInteger(kPrefGreylist, &int_value))
"greylist" seems like a misnomer now [that this is just generic "blacklist
state"]?

https://chromiumcodereview.appspot.com/98463005/diff/190001/chrome/browser/ex...
chrome/browser/extensions/extension_prefs.cc:1168: else
no else after return.

https://chromiumcodereview.appspot.com/98463005/diff/190001/chrome/browser/ex...
chrome/browser/extensions/extension_prefs.cc:1169: return NOT_BLACKLISTED;
could you test this logic in extension_prefs_unittest.cc? just the way that the
IsExtensionBlacklisted takes priority and all that. should be simple enough.

https://chromiumcodereview.appspot.com/98463005/diff/190001/chrome/browser/ex...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/98463005/diff/190001/chrome/browser/ex...
chrome/browser/extensions/extension_service_unittest.cc:3557: // after it is
installed. It is then successfuly re-enabled by the user.
successfully

https://chromiumcodereview.appspot.com/98463005/diff/190001/chrome/browser/ex...
chrome/browser/extensions/extension_service_unittest.cc:3585:
disabled_extensions.Contains(good0));
Just put all of these as single assertions, i.e. rather than the !foo && bar
have

EXPECT_FALSE(enabled_extensions.Contains(good0));
EXPECT_TRUE(disabled_extensions.Contains(good0));

etc, and for all occurrences of this. It will give slightly more meaningful
failure messages (i.e. if the !foo && bar form changes.. it is !foo or bar which
was not fulfilled?)

[Oleg Eterevsky, 2014-01-23 14:48:53]

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
chrome/browser/extensions/extension_service.cc:2783: std::set<std::string>
blocked;
On 2014/01/22 22:24:38, kalman wrote:
> On 2014/01/22 21:25:04, Oleg wrote:
> > On 2014/01/22 00:33:14, kalman wrote:
> > > (following on from comment in header file) personally I prefer the term
> > > "malware" here since it's more specific than just "blocked"... term is a
bit
> > > abstract to me... but not a big deal.
> > 
> > I think we should fix the terminology everywhere, but it will touch so many
> > files, that I'd prefer to do it in a separate CL. How about:
> > 
> > blacklist -- refer to Blacklist with all the possible states,
> 
> I think this is actually the thing that needs to change. And I think based on
> your comment you agree? Blacklist is no longer the right term for that class;
to
> me a blacklist really does mean a single list of things that are never
allowed;
> pulling a greylist out of that is odd.
> 
> As you say we can worry about that / discuss naming later.
> 
> > 
> > malware, potentially unwanted, ... -- refer to the reason for the action we
> > take, but not to the actual categories of extensions in service and
> > ExtensionRegistry
> > 
> > blocked -- what was formerly blacklisted. If you can come up with a better
> term,
> > I'm willing to change
> 
> I'm ok leaving this as blocked for now, but I'd ideally like to see this back
as
> blacklisted (see above comment).
> 
> > 
> > greylist -- disabled once
> > 
> > This way, if we decide to change how we treat some type of extensions (like,
> we
> > decide to permanently block extensions violating web store policy), we'll
just
> > change a could of switches and ifs.
> 

Ok, let's commit it like this and then discuss the best terminology on
chrome-apps-team@ or apps-dev@.

https://chromiumcodereview.appspot.com/98463005/diff/110001/chrome/browser/ex...
chrome/browser/extensions/extension_service.cc:2868: void
ExtensionService::UpdateGreylistedExtensions(
On 2014/01/22 22:24:38, kalman wrote:
> On 2014/01/22 21:25:04, Oleg wrote:
> > On 2014/01/22 00:33:14, kalman wrote:
> > > seems like this method is basically the same as UpdateBlockedExtensions
just
> > > dealing with different sets, and with UMA logging above (should you be
doing
> > UMA
> > > logging here too?).
> > 
> > I'm not sure what exactly to log. Is it ok if I add a todo, and do it in a
> > separate CL?
> 
> I suppose you might as well do it in a follow up. I just mean to say that
> tracking this stuff in UMA would be useful, and it occurs to me that
greylisting
> is even more subtle than blacklisting. How often are extensions disabled from
> the greylist? How often are they re-enabled by the user? Etc. Based on that a
> follow-up SGTM.
> 
> > 
> > > it's probably too contrived to actually make them share all of the code,
but
> > you
> > > should at least be able to pull out the set division (lines 2871 - 2882)
> into
> > a
> > > sensible common place.
> > 
> > Well, if there were just one resulting set, it would simplify the code, but
I
> > feel that in the current state an extra function will not be much help. If
you
> > wish, I can add STLSetUnion to stl_unil add use it.
> 
> I'm imagining something like:
> 
> void Partition(const ExtensionIdSet& before,
>                const ExtensionIdSet& after,
>                const ExtensionIdSet& unchanged,
>                ExtensionIdSet* no_longer,
>                ExtensionIdSet* not_yet) {
>   ExtensionIdSet before_and_unchanged;
>   std::set_union(...);
>   *no_longer = base::STLSetDifference(...);
>   *not_yet = base::STLSetDifference(...);
> }
> 
> void UpdateGreylistedExtensions(const ExtensionIdSet& greylist,
>                                 const ExtensionIdSet& unchanged) {
>   ExtensionIdSet not_yet, no_longer;
>   Partition(greylist_.GetIDs(), greylist, unchanged,
>             &not_yet, &no_longer);
> }

Done. It actually did make the code simpler.

https://chromiumcodereview.appspot.com/98463005/diff/190001/chrome/browser/ex...
File chrome/browser/extensions/extension_prefs.cc (right):

https://chromiumcodereview.appspot.com/98463005/diff/190001/chrome/browser/ex...
chrome/browser/extensions/extension_prefs.cc:1166: if (ext_prefs &&
ext_prefs->GetInteger(kPrefGreylist, &int_value))
On 2014/01/22 22:24:39, kalman wrote:
> "greylist" seems like a misnomer now [that this is just generic "blacklist
> state"]?

Done.

https://chromiumcodereview.appspot.com/98463005/diff/190001/chrome/browser/ex...
chrome/browser/extensions/extension_prefs.cc:1168: else
On 2014/01/22 22:24:39, kalman wrote:
> no else after return.

Done.

https://chromiumcodereview.appspot.com/98463005/diff/190001/chrome/browser/ex...
chrome/browser/extensions/extension_prefs.cc:1169: return NOT_BLACKLISTED;
On 2014/01/22 22:24:39, kalman wrote:
> could you test this logic in extension_prefs_unittest.cc? just the way that
the
> IsExtensionBlacklisted takes priority and all that. should be simple enough.

Done.

https://chromiumcodereview.appspot.com/98463005/diff/190001/chrome/browser/ex...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/98463005/diff/190001/chrome/browser/ex...
chrome/browser/extensions/extension_service_unittest.cc:3557: // after it is
installed. It is then successfuly re-enabled by the user.
On 2014/01/22 22:24:39, kalman wrote:
> successfully

Done.

https://chromiumcodereview.appspot.com/98463005/diff/190001/chrome/browser/ex...
chrome/browser/extensions/extension_service_unittest.cc:3585:
disabled_extensions.Contains(good0));
On 2014/01/22 22:24:39, kalman wrote:
> Just put all of these as single assertions, i.e. rather than the !foo && bar
> have
> 
> EXPECT_FALSE(enabled_extensions.Contains(good0));
> EXPECT_TRUE(disabled_extensions.Contains(good0));
> 
> etc, and for all occurrences of this. It will give slightly more meaningful
> failure messages (i.e. if the !foo && bar form changes.. it is !foo or bar
which
> was not fulfilled?)

Done.

[Oleg Eterevsky, 2014-01-23 14:52:19]

Hi Finnur!

Could you please review this CL? Ben Kalman already LGTMd everything but the
change to chrome/browser/ui/webui/extensions/extension_settings_handler.cc

[Finnur, 2014-01-23 15:21:37]

You have a pretty common first name, would you mind going to
https://chromiumcodereview.appspot.com/settings
... and add your last name in the nickname section, so people can differentiate
easier? :)

https://chromiumcodereview.appspot.com/98463005/diff/280001/chrome/browser/ui...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://chromiumcodereview.appspot.com/98463005/diff/280001/chrome/browser/ui...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:260: } else if
(blacklist_state != NOT_BLACKLISTED) {
It is a bit weird that we'd categorize this label lower in priority than the one
for policy-extensions, the one for internal-but-not-from-webstore-extensions and
for registry-extensions. 

But lets take a step back. With the blacklist label this is really not an
either-or kind of situation anymore, like |location| is. It is not a stretch to
imagine us blacklisting one of the types of extensions that the if clauses (on
line 250-259 above) cover. We'd never see the blacklist label in that case,
which doesn't seem right.

This label should, in my mind be separate and added right after the location
based one.

https://chromiumcodereview.appspot.com/98463005/diff/280001/chrome/browser/ui...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:275: break;
Why is BLACKLISTED_MALWARE not handled?

[Oleg Eterevsky, 2014-01-23 16:33:05]

https://chromiumcodereview.appspot.com/98463005/diff/280001/chrome/browser/ui...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://chromiumcodereview.appspot.com/98463005/diff/280001/chrome/browser/ui...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:260: } else if
(blacklist_state != NOT_BLACKLISTED) {
On 2014/01/23 15:21:38, Finnur wrote:
> It is a bit weird that we'd categorize this label lower in priority than the
one
> for policy-extensions, the one for internal-but-not-from-webstore-extensions
and
> for registry-extensions. 
> 
> But lets take a step back. With the blacklist label this is really not an
> either-or kind of situation anymore, like |location| is. It is not a stretch
to
> imagine us blacklisting one of the types of extensions that the if clauses (on
> line 250-259 above) cover. We'd never see the blacklist label in that case,
> which doesn't seem right.
> 
> This label should, in my mind be separate and added right after the location
> based one.

I'll ask a PM working on this feature to find the right people to decide what
should happen if these two states coincide. In my opinion, this shouldn't really
happen, and we should just show the more important message.

https://chromiumcodereview.appspot.com/98463005/diff/280001/chrome/browser/ui...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:275: break;
On 2014/01/23 15:21:38, Finnur wrote:
> Why is BLACKLISTED_MALWARE not handled?

Because malware extensions are not shown in the list at all. We do not even have
a string for them.

[Oleg Eterevsky, 2014-01-24 15:14:00]

Ok, I'm now showing the blacklist state under location text in case both are
presented.

[Finnur, 2014-01-24 17:03:43]

Thank you. Can you post a screenshot somewhere (on the bug perhaps)?

[Oleg Eterevsky, 2014-01-27 12:05:19]

On 2014/01/24 17:03:43, Finnur wrote:
> Thank you. Can you post a screenshot somewhere (on the bug perhaps)?

Done. See https://code.google.com/p/chromium/issues/detail?id=267514#c15

[Finnur, 2014-01-27 13:15:03]

https://chromiumcodereview.appspot.com/98463005/diff/340001/chrome/browser/re...
File chrome/browser/resources/extensions/extensions.html (right):

https://chromiumcodereview.appspot.com/98463005/diff/340001/chrome/browser/re...
chrome/browser/resources/extensions/extensions.html:130: <div
class="permanent-warnings">
Looking at the screenshot (thank you for that), I realize we should not be
adding the text under |location|. That space is too narrow for this much text
and the error you are showing, which is pretty significant, does not get enough
attention.

I think you should instead add your message here, and do it in the same way as
we do below, with the two child divs that start out hidden. This will look much
better on screen and draw the user's attention to it immediately.

https://chromiumcodereview.appspot.com/98463005/diff/340001/chrome/browser/re...
chrome/browser/resources/extensions/extensions.html:135: <div
class="suspicious-install-message" hidden>
Please note that this div is very similar in nature to your div showing very
similar text. It is shown when an extension has been wiped (disabled) for being
suspicious (to simulate it you can disable an extension and set the
DisableReason for it in the prefs to 256 and then you'll see this div).

This does make me wonder if we'll have any unfortunate overlap with what Antony
has been doing (disabling non-cws extensions). CC-ing Antony, to keep him in the
loop.

[Oleg Eterevsky, 2014-01-27 13:59:58]

https://chromiumcodereview.appspot.com/98463005/diff/340001/chrome/browser/re...
File chrome/browser/resources/extensions/extensions.html (right):

https://chromiumcodereview.appspot.com/98463005/diff/340001/chrome/browser/re...
chrome/browser/resources/extensions/extensions.html:130: <div
class="permanent-warnings">
On 2014/01/27 13:15:04, Finnur wrote:
> Looking at the screenshot (thank you for that), I realize we should not be
> adding the text under |location|. That space is too narrow for this much text
> and the error you are showing, which is pretty significant, does not get
enough
> attention.
> 
> I think you should instead add your message here, and do it in the same way as
> we do below, with the two child divs that start out hidden. This will look
much
> better on screen and draw the user's attention to it immediately.

I believe the UX for disable list message was confirmed by UX team. 

This message is not really a warning. It is more of a notice to explain to user
why we have disabled this extension.

Do you think it would be ok to commit this feature as is, so that it could start
being tested, and then ask around about which form of this message is better?

https://chromiumcodereview.appspot.com/98463005/diff/340001/chrome/browser/re...
chrome/browser/resources/extensions/extensions.html:135: <div
class="suspicious-install-message" hidden>
On 2014/01/27 13:15:04, Finnur wrote:
> Please note that this div is very similar in nature to your div showing very
> similar text. It is shown when an extension has been wiped (disabled) for
being
> suspicious (to simulate it you can disable an extension and set the
> DisableReason for it in the prefs to 256 and then you'll see this div).
> 
> This does make me wonder if we'll have any unfortunate overlap with what
Antony
> has been doing (disabling non-cws extensions). CC-ing Antony, to keep him in
the
> loop.

Thanks. We met Antony a while ago and he should be aware of this feature.

[Finnur, 2014-01-27 14:12:20]

> I believe the UX for disable list message was confirmed by UX team. 

> This message is not really a warning. It is more of a notice to explain to
user
> why we have disabled this extension.

We can probably do that. Who approved it?

[Oleg Eterevsky, 2014-01-27 14:15:29]

On 2014/01/27 14:12:20, Finnur wrote:
> > I believe the UX for disable list message was confirmed by UX team. 
> 
> > This message is not really a warning. It is more of a notice to explain to
> user
> > why we have disabled this extension.
> 
> We can probably do that. Who approved it?

Judging by https://code.google.com/p/chromium/issues/detail?id=267514#c3, it was
roma@chromium.org

[Finnur, 2014-01-27 14:23:23]

On 2014/01/27 14:15:29, Oleg Eterevsky wrote:
> On 2014/01/27 14:12:20, Finnur wrote:
> > > I believe the UX for disable list message was confirmed by UX team. 
> > 
> > > This message is not really a warning. It is more of a notice to explain to
> > user
> > > why we have disabled this extension.
> > 
> > We can probably do that. Who approved it?
> 
> Judging by https://code.google.com/p/chromium/issues/detail?id=267514#c3, it
was
> mailto:roma@chromium.org

Thanks. I'll start a thread with Roma and you about this. In the meantime I'll
flip the review bit now (LGTM) but with the caveat that you might have to move
the string (as per the outcome of that thread).

[Oleg Eterevsky, 2014-01-27 14:24:36]

On 2014/01/27 14:23:23, Finnur wrote:
> On 2014/01/27 14:15:29, Oleg Eterevsky wrote:
> > On 2014/01/27 14:12:20, Finnur wrote:
> > > > I believe the UX for disable list message was confirmed by UX team. 
> > > 
> > > > This message is not really a warning. It is more of a notice to explain
to
> > > user
> > > > why we have disabled this extension.
> > > 
> > > We can probably do that. Who approved it?
> > 
> > Judging by https://code.google.com/p/chromium/issues/detail?id=267514#c3, it
> was
> > mailto:roma@chromium.org
> 
> Thanks. I'll start a thread with Roma and you about this. In the meantime I'll
> flip the review bit now (LGTM) but with the caveat that you might have to move
> the string (as per the outcome of that thread).

Sure. Thanks for the review!

[commit-bot: I haz the power, 2014-01-27 14:24:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oleg@chromium.org/98463005/430001

[commit-bot: I haz the power, 2014-01-27 15:20:47]

Retried try job too often on mac_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-27 15:52:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oleg@chromium.org/98463005/510003

[commit-bot: I haz the power, 2014-01-27 17:30:03]

Change committed as 247263