Make Blacklist::IsBlacklist asynchronous (it will need to be for safe

Make Blacklist::IsBlacklist asynchronous (it will need to be for safe
browsing), and unravel the knots that result from it:
 - Decouple it from the admin policy.
 - Take the other half of blacklist logic out of ExtensionPrefs and into
   Blacklist.
 - Fix bug where blacklisted extensions couldn't be re-installed (let alone
   re-enabled) if they get taken off the blacklist.


TBR=sky@chromium.org
BUG=154149, 156750
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171079

[not at google - send to devlin, 2012-11-30 00:05:04]

yoz: doing refactoring and reviewed the original changes to Blacklist so adding
you as a reviewer.
asargent: familiar with the current way of blacklisting things.

[not at google - send to devlin, 2012-11-30 18:57:20]

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
File chrome/browser/extensions/blacklist.cc (right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
chrome/browser/extensions/blacklist.cc:83: }
I'm thinking about moving the prefs modification stuff into the similar-looking
code in ExtensionService, and making Blacklist just cache the result of
SetFromUpdater (reading in the initial blacklist from prefs). WDYT?

[asargent_no_longer_on_chrome, 2012-11-30 21:44:22]

LGTM

That's cool that we're going to use the safebrowsing system for the blacklist
(this is the first I've heard of it). Does this mean we're going to rip out the
stuff in the autoupdate system?

(+cc mek since he might be interested in this)

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
File chrome/browser/extensions/blacklist.cc (right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
chrome/browser/extensions/blacklist.cc:74: not_yet_blacklisted.begin()));
It would be sort of cool if we had a SetDifference helper method to cut down on
all the STL boilerplate here. I wonder if enough other places in the code do
this that it would be worth having in base/stl_util.h.

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_blacklist_browsertest.cc (right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_blacklist_browsertest.cc:186: Profile*
profile() {
optional suggestion: It might be worth hoisting this up into
ExtensionBrowserTest - I feel like a lot of tests could take advantage of it.

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_blacklist_browsertest.cc:194:
ExtensionService* extension_service() {
same for this one

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_prefs.cc:775: new
base::FundamentalValue(true));
optional suggestion: I personally prefer "base::CreateBooleanValue(true)"

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.h:860: // Blacklist for the owning
profile (weak reference).
optional suggestion: Whenever I see "weak reference" I think of WeakPtr<T>,
which is a little different from "a pointer I don't own". I kind of prefer to
use the term "unowned" instead of "weak" when talking about these.

aside: Sometimes I've wondered if our codebase would benefit from a
UnownedPtr<T> class that just serves to document non-ownership, so that you
don't have to wonder about a given naked pointer - it's always wrapped with one
of scoped_ptr/scoped_refptr/linked_ptr, WeakPtr, UnownedPtr, etc.

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/sta...
File chrome/browser/extensions/standard_management_policy_provider_unittest.cc
(right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/sta...
chrome/browser/extensions/standard_management_policy_provider_unittest.cc:54: /*
Is there a reason you just commented this out instead of removing it?

[asargent_no_longer_on_chrome, 2012-11-30 21:49:59]

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
File chrome/browser/extensions/blacklist.cc (right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
chrome/browser/extensions/blacklist.cc:83: }
On 2012/11/30 18:57:20, kalman wrote:
> I'm thinking about moving the prefs modification stuff into the
similar-looking
> code in ExtensionService, and making Blacklist just cache the result of
> SetFromUpdater (reading in the initial blacklist from prefs). WDYT?

Sorry, I missed this comment when going through the CL. What's the long-term
vision for the storage of this set of id's? You mention using safebrowsing -
does that mean we'll have a separate data store for them outside of the
profile-specific prefs? (I assume we'd just store in something like leveldb, as
opposed to getting fancy and having a bloom filter with fallback to network
requests to avoid false positives, since the data set is so much smaller.)

[not at google - send to devlin, 2012-11-30 22:06:39]

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
File chrome/browser/extensions/blacklist.cc (right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
chrome/browser/extensions/blacklist.cc:83: }
On 2012/11/30 21:49:59, Antony Sargent wrote:
> On 2012/11/30 18:57:20, kalman wrote:
> > I'm thinking about moving the prefs modification stuff into the
> similar-looking
> > code in ExtensionService, and making Blacklist just cache the result of
> > SetFromUpdater (reading in the initial blacklist from prefs). WDYT?
> 
> Sorry, I missed this comment when going through the CL. What's the long-term
> vision for the storage of this set of id's? You mention using safebrowsing -
> does that mean we'll have a separate data store for them outside of the
> profile-specific prefs? (I assume we'd just store in something like leveldb,
as
> opposed to getting fancy and having a bloom filter with fallback to network
> requests to avoid false positives, since the data set is so much smaller.)

The nice thing about the safebrowsing databases is we won't get false positives,
since the hashes they use are 32 bytes, the same length as extension IDs. So we
can just store the IDs directly and not have to worry about round trips etc. I'm
writing the code to add the new database to the safebrowsing's set and it seems
pretty uncontroversial (cargo cult so far).

I believe we actually are going to start getting a lot more extensions
blacklisted, the process of which is blocked on getting this code out. During
migration perhaps we'll still have the old set but keep it small (only the very
bad extensions) then put the full set in safebrowsing.

(and yeah, obviously the blacklist property for extensions will remain in prefs,
just only for the extensions that are actually installed)

[Yoyo Zhou, 2012-11-30 22:24:35]

On 2012/11/30 22:06:39, kalman wrote:
>
https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
> File chrome/browser/extensions/blacklist.cc (right):
> 
>
https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
> chrome/browser/extensions/blacklist.cc:83: }
> On 2012/11/30 21:49:59, Antony Sargent wrote:
> > On 2012/11/30 18:57:20, kalman wrote:
> > > I'm thinking about moving the prefs modification stuff into the
> > similar-looking
> > > code in ExtensionService, and making Blacklist just cache the result of
> > > SetFromUpdater (reading in the initial blacklist from prefs). WDYT?
> > 
> > Sorry, I missed this comment when going through the CL. What's the long-term
> > vision for the storage of this set of id's? You mention using safebrowsing -
> > does that mean we'll have a separate data store for them outside of the
> > profile-specific prefs? (I assume we'd just store in something like leveldb,
> as
> > opposed to getting fancy and having a bloom filter with fallback to network
> > requests to avoid false positives, since the data set is so much smaller.)
> 
> The nice thing about the safebrowsing databases is we won't get false
positives,
> since the hashes they use are 32 bytes, the same length as extension IDs. So
we
> can just store the IDs directly and not have to worry about round trips etc.
I'm
> writing the code to add the new database to the safebrowsing's set and it
seems
> pretty uncontroversial (cargo cult so far).
> 
> I believe we actually are going to start getting a lot more extensions
> blacklisted, the process of which is blocked on getting this code out. During
> migration perhaps we'll still have the old set but keep it small (only the
very
> bad extensions) then put the full set in safebrowsing.
> 
> (and yeah, obviously the blacklist property for extensions will remain in
prefs,
> just only for the extensions that are actually installed)

In other words, the main motivation is that current solution of storing every
blacklisted id in the prefs doesn't scale.

[asargent_no_longer_on_chrome, 2012-11-30 22:27:13]

On 2012/11/30 22:06:39, kalman wrote:
>
https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
> File chrome/browser/extensions/blacklist.cc (right):
> 
>
https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
> chrome/browser/extensions/blacklist.cc:83: }
> On 2012/11/30 21:49:59, Antony Sargent wrote:
> > On 2012/11/30 18:57:20, kalman wrote:
> > > I'm thinking about moving the prefs modification stuff into the
> > similar-looking
> > > code in ExtensionService, and making Blacklist just cache the result of
> > > SetFromUpdater (reading in the initial blacklist from prefs). WDYT?
> > 
> > Sorry, I missed this comment when going through the CL. What's the long-term
> > vision for the storage of this set of id's? You mention using safebrowsing -
> > does that mean we'll have a separate data store for them outside of the
> > profile-specific prefs? (I assume we'd just store in something like leveldb,
> as
> > opposed to getting fancy and having a bloom filter with fallback to network
> > requests to avoid false positives, since the data set is so much smaller.)
> 
> The nice thing about the safebrowsing databases is we won't get false
positives,
> since the hashes they use are 32 bytes, the same length as extension IDs. So
we
> can just store the IDs directly and not have to worry about round trips etc.
I'm
> writing the code to add the new database to the safebrowsing's set and it
seems
> pretty uncontroversial (cargo cult so far).
> 
> I believe we actually are going to start getting a lot more extensions
> blacklisted, the process of which is blocked on getting this code out. During
> migration perhaps we'll still have the old set but keep it small (only the
very
> bad extensions) then put the full set in safebrowsing.
> 
> (and yeah, obviously the blacklist property for extensions will remain in
prefs,
> just only for the extensions that are actually installed)

Ok, that makes sense. I think you're right that in the end state we want just
the ExtensionService using the prefs (for installed but blacklisted extensions),
and the Blacklist will just use the safe browsing data store. If you want to do
part of that work in this CL, that's fine by me.

[not at google - send to devlin, 2012-11-30 23:09:54]

Yoyo lmk when it lgty.

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
File chrome/browser/extensions/blacklist.cc (right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
chrome/browser/extensions/blacklist.cc:74: not_yet_blacklisted.begin()));
On 2012/11/30 21:44:22, Antony Sargent wrote:
> It would be sort of cool if we had a SetDifference helper method to cut down
on
> all the STL boilerplate here. I wonder if enough other places in the code do
> this that it would be worth having in base/stl_util.h.

Sounds good to me.

Here we go: https://codereview.chromium.org/11415239/

If that change goes in before this one, I'll convert this to using it. Otherwise
I'll do it in a follow-up.

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
chrome/browser/extensions/blacklist.cc:83: }
On 2012/11/30 22:06:39, kalman wrote:
> On 2012/11/30 21:49:59, Antony Sargent wrote:
> > On 2012/11/30 18:57:20, kalman wrote:
> > > I'm thinking about moving the prefs modification stuff into the
> > similar-looking
> > > code in ExtensionService, and making Blacklist just cache the result of
> > > SetFromUpdater (reading in the initial blacklist from prefs). WDYT?
> > 
> > Sorry, I missed this comment when going through the CL. What's the long-term
> > vision for the storage of this set of id's? You mention using safebrowsing -
> > does that mean we'll have a separate data store for them outside of the
> > profile-specific prefs? (I assume we'd just store in something like leveldb,
> as
> > opposed to getting fancy and having a bloom filter with fallback to network
> > requests to avoid false positives, since the data set is so much smaller.)
> 
> The nice thing about the safebrowsing databases is we won't get false
positives,
> since the hashes they use are 32 bytes, the same length as extension IDs. So
we
> can just store the IDs directly and not have to worry about round trips etc.
I'm
> writing the code to add the new database to the safebrowsing's set and it
seems
> pretty uncontroversial (cargo cult so far).
> 
> I believe we actually are going to start getting a lot more extensions
> blacklisted, the process of which is blocked on getting this code out. During
> migration perhaps we'll still have the old set but keep it small (only the
very
> bad extensions) then put the full set in safebrowsing.
> 
> (and yeah, obviously the blacklist property for extensions will remain in
prefs,
> just only for the extensions that are actually installed)

As you say, I think this change would make more sense to do as a follow-up when
the role of ExtensionPrefs as the blacklist has been fully replaced by
safebrowsing.

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_blacklist_browsertest.cc (right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_blacklist_browsertest.cc:186: Profile*
profile() {
On 2012/11/30 21:44:22, Antony Sargent wrote:
> optional suggestion: It might be worth hoisting this up into
> ExtensionBrowserTest - I feel like a lot of tests could take advantage of it. 

Done.

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_blacklist_browsertest.cc:194:
ExtensionService* extension_service() {
On 2012/11/30 21:44:22, Antony Sargent wrote:
> same for this one

Done.

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_prefs.cc:775: new
base::FundamentalValue(true));
On 2012/11/30 21:44:22, Antony Sargent wrote:
> optional suggestion: I personally prefer "base::CreateBooleanValue(true)"
> 

I don't really have a preference though I'm used to using CreateBooleanValue -
however, it seems somebody has written "DEPRECATED" on them (git blame says 20th
november), which is why I did it this way.

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.h:860: // Blacklist for the owning
profile (weak reference).
On 2012/11/30 21:44:22, Antony Sargent wrote:
> optional suggestion: Whenever I see "weak reference" I think of WeakPtr<T>,
> which is a little different from "a pointer I don't own". I kind of prefer to
> use the term "unowned" instead of "weak" when talking about these.
> 
> aside: Sometimes I've wondered if our codebase would benefit from a
> UnownedPtr<T> class that just serves to document non-ownership, so that you
> don't have to wonder about a given naked pointer - it's always wrapped with
one
> of scoped_ptr/scoped_refptr/linked_ptr, WeakPtr, UnownedPtr, etc.
> 

Done.

TBH I thought the "weak reference" thing was always redundant anyway, since
usually that's conveyed by the type - raw pointer means not-owned.

And aside: I have also occasionally wanted the UnownedPtr thing, though mostly
so that it could NULL itself out (like WeakPtr does - it's helped with tracking
down a bug before), but presumably this is (a) basically WeakPtr anyway, and (b)
introduces threading issues (like WeakPtr).

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/sta...
File chrome/browser/extensions/standard_management_policy_provider_unittest.cc
(right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/sta...
chrome/browser/extensions/standard_management_policy_provider_unittest.cc:54: /*
On 2012/11/30 21:44:22, Antony Sargent wrote:
> Is there a reason you just commented this out instead of removing it? 

Oh, I didn't notice. Oops. Yeah I incorporated the non-redundant parts of this
test into the new tests.

[asargent_no_longer_on_chrome, 2012-11-30 23:30:24]

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_prefs.cc:775: new
base::FundamentalValue(true));
On 2012/11/30 23:09:54, kalman wrote:
> On 2012/11/30 21:44:22, Antony Sargent wrote:
> > optional suggestion: I personally prefer "base::CreateBooleanValue(true)"
> > 
> 
> I don't really have a preference though I'm used to using CreateBooleanValue -
> however, it seems somebody has written "DEPRECATED" on them (git blame says
20th
> november), which is why I did it this way.

Ah, I missed that. Ok, probably better to go with "new FundamentalValue" then.

(I looked at https://chromiumcodereview.appspot.com/11418048 and don't really
agree with it, but only for bikeshedding reasons. Consistency is far more
important).

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.h:860: // Blacklist for the owning
profile (weak reference).
> TBH I thought the "weak reference" thing was always redundant anyway, since
> usually that's conveyed by the type - raw pointer means not-owned.

Good point.

[Yoyo Zhou, 2012-11-30 23:44:06]

LGTM

https://chromiumcodereview.appspot.com/11415216/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/blacklist.cc (right):

https://chromiumcodereview.appspot.com/11415216/diff/1/chrome/browser/extensi...
chrome/browser/extensions/blacklist.cc:83: }
On 2012/11/30 18:57:20, kalman wrote:
> I'm thinking about moving the prefs modification stuff into the
similar-looking
> code in ExtensionService, and making Blacklist just cache the result of
> SetFromUpdater (reading in the initial blacklist from prefs). WDYT?

Just as a general comment, there should be less stuff in ExtensionService, not
more, so I'd prefer to have blacklist-related prefs interaction here in
Blacklist rather than in ExtensionService.

https://chromiumcodereview.appspot.com/11415216/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/blacklist.h (right):

https://chromiumcodereview.appspot.com/11415216/diff/1/chrome/browser/extensi...
chrome/browser/extensions/blacklist.h:48: void IsBlacklisted(const
std::set<std::string>& ids,
This name reads strangely; I expect an Is* function to return a bool. Would
GetBlacklistedIDs be better?

https://chromiumcodereview.appspot.com/11415216/diff/1/chrome/browser/extensi...
chrome/browser/extensions/blacklist.h:53: const IsBlacklistedCallback&
callback);
The callback parameter doesn't seem that convenient for a single ID. (I would
expect a bool argument instead of a set<string>!) If that's too much trouble
(and there are no users of this) then I would just omit this method for now.

https://chromiumcodereview.appspot.com/11415216/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/11415216/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_service.cc:1866: // Extensions that really
are blacklisted.
I was confused reading the "disabled by policy" block before I got here, so
maybe this one should come first.

https://chromiumcodereview.appspot.com/11415216/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_service.cc:2119: // Don't check the
Blacklist here because it's asynchronous (we do it at
nit: s/here/yet/

[not at google - send to devlin, 2012-12-01 02:51:30]

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
File chrome/browser/extensions/blacklist.cc (right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
chrome/browser/extensions/blacklist.cc:83: }
On 2012/11/30 23:44:07, Yoyo Zhou wrote:
> On 2012/11/30 18:57:20, kalman wrote:
> > I'm thinking about moving the prefs modification stuff into the
> similar-looking
> > code in ExtensionService, and making Blacklist just cache the result of
> > SetFromUpdater (reading in the initial blacklist from prefs). WDYT?
> 
> Just as a general comment, there should be less stuff in ExtensionService, not
> more, so I'd prefer to have blacklist-related prefs interaction here in
> Blacklist rather than in ExtensionService.

My plan was to have Blacklist be essentially an interface for querying a
blacklist, and notifying things when it changes, not actually doing the
management itself. For once I think that the logic I've added to
ExtensionService actually fits its designed purpose - managing installed
extensions.

The thing that bugs me a bit here is just that ExtensionService needs to read
the blacklist state of extensions from prefs (see synchronous check in
AddExtension) and yet doesn't actually set it.

Or to put it another way - when ExtensionService disables an extension it sets
the disabled bit in prefs. Blacklisting should work the same.

However, until the full-blacklist and extension-is-blacklisted concepts are
unconflated, I think it's best that it stays this way.

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
File chrome/browser/extensions/blacklist.h (right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
chrome/browser/extensions/blacklist.h:48: void IsBlacklisted(const
std::set<std::string>& ids,
On 2012/11/30 23:44:07, Yoyo Zhou wrote:
> This name reads strangely; I expect an Is* function to return a bool. Would
> GetBlacklistedIDs be better?

Done.

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/bla...
chrome/browser/extensions/blacklist.h:53: const IsBlacklistedCallback&
callback);
On 2012/11/30 23:44:07, Yoyo Zhou wrote:
> The callback parameter doesn't seem that convenient for a single ID. (I would
> expect a bool argument instead of a set<string>!) If that's too much trouble
> (and there are no users of this) then I would just omit this method for now.

Done.

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:1866: // Extensions that really
are blacklisted.
On 2012/11/30 23:44:07, Yoyo Zhou wrote:
> I was confused reading the "disabled by policy" block before I got here, so
> maybe this one should come first.

Done.

https://codereview.chromium.org/11415216/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:2119: // Don't check the
Blacklist here because it's asynchronous (we do it at
On 2012/11/30 23:44:07, Yoyo Zhou wrote:
> nit: s/here/yet/

Done.

[commit-bot: I haz the power, 2012-12-01 02:52:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/11415216/7003

[commit-bot: I haz the power, 2012-12-01 02:52:20]

Presubmit check for 11415216-7003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/background
    chrome/browser/protector

Presubmit checks took 3.4s to calculate.

[commit-bot: I haz the power, 2012-12-01 02:53:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/11415216/7003

[commit-bot: I haz the power, 2012-12-01 03:12:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/11415216/14001

[commit-bot: I haz the power, 2012-12-01 03:12:33]

Failed to apply patch for chrome/browser/extensions/extension_service.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_service.cc
  Hunk #2 succeeded at 366 (offset 1 line).
  Hunk #3 succeeded at 472 (offset 1 line).
  Hunk #4 succeeded at 900 (offset 3 lines).
  Hunk #5 succeeded at 1275 (offset -1 lines).
  Hunk #6 succeeded at 1839 (offset -1 lines).
  Hunk #7 succeeded at 1871 (offset -1 lines).
  Hunk #8 succeeded at 2111 (offset -1 lines).
  Hunk #9 FAILED at 2134.
  Hunk #10 succeeded at 3074 (offset 6 lines).
  1 out of 10 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_service.cc.rej

Patch:       chrome/browser/extensions/extension_service.cc
Index: chrome/browser/extensions/extension_service.cc
diff --git a/chrome/browser/extensions/extension_service.cc
b/chrome/browser/extensions/extension_service.cc
index
f49aaa819136327285f04351bcec21f20c32ad90..ed36a77efd77dc6684613c26d7848292f337ebf8
100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/extensions/extension_service.h"
 
 #include <algorithm>
+#include <iterator>
 #include <set>
 
 #include "base/basictypes.h"
@@ -364,6 +365,7 @@ ExtensionService::ExtensionService(Profile* profile,
       profile_(profile),
       system_(extensions::ExtensionSystem::Get(profile)),
       extension_prefs_(extension_prefs),
+      blacklist_(blacklist),
       settings_frontend_(extensions::SettingsFrontend::Create(profile)),
       pending_extension_manager_(*ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       install_directory_(install_directory),
@@ -469,23 +471,28 @@ const ExtensionSet*
ExtensionService::terminated_extensions() const {
   return &terminated_extensions_;
 }
 
-const ExtensionSet* ExtensionService::GenerateInstalledExtensionsSet() const {
-  ExtensionSet* installed_extensions = new ExtensionSet();
+const ExtensionSet* ExtensionService::blacklisted_extensions() const {
+  return &blacklisted_extensions_;
+}
+
+scoped_ptr<const ExtensionSet>
+    ExtensionService::GenerateInstalledExtensionsSet() const {
+  scoped_ptr<ExtensionSet> installed_extensions(new ExtensionSet());
   installed_extensions->InsertAll(extensions_);
   installed_extensions->InsertAll(disabled_extensions_);
   installed_extensions->InsertAll(terminated_extensions_);
-  return installed_extensions;
+  return installed_extensions.PassAs<const ExtensionSet>();
 }
 
-const ExtensionSet* ExtensionService::GetWipedOutExtensions() const {
-  ExtensionSet* extension_set = new ExtensionSet();
+scoped_ptr<const ExtensionSet> ExtensionService::GetWipedOutExtensions() const
{
+  scoped_ptr<ExtensionSet> extension_set(new ExtensionSet());
   for (ExtensionSet::const_iterator iter = disabled_extensions_.begin();
        iter != disabled_extensions_.end(); ++iter) {
     int disabled_reason = extension_prefs_->GetDisableReasons((*iter)->id());
     if ((disabled_reason & Extension::DISABLE_SIDELOAD_WIPEOUT) != 0)
       extension_set->Insert(*iter);
   }
-  return extension_set;
+  return extension_set.PassAs<const ExtensionSet>();
 }
 
 extensions::PendingExtensionManager*
@@ -890,8 +897,10 @@ bool ExtensionService::IsExtensionEnabled(
     return true;
   }
 
-  if (disabled_extensions_.Contains(extension_id))
+  if (disabled_extensions_.Contains(extension_id) ||
+      blacklisted_extensions_.Contains(extension_id)) {
     return false;
+  }
 
   // If the extension hasn't been loaded yet, check the prefs for it. Assume
   // enabled unless otherwise noted.
@@ -1267,6 +1276,7 @@ extensions::ExtensionUpdater* ExtensionService::updater()
{
 
 void ExtensionService::CheckManagementPolicy() {
   std::vector<std::string> to_be_removed;
+
   // Loop through extensions list, unload installed extensions.
   for (ExtensionSet::const_iterator iter = extensions_.begin();
        iter != extensions_.end(); ++iter) {
@@ -1830,15 +1840,31 @@ void ExtensionService::IdentifyAlertableExtensions() {
 bool ExtensionService::PopulateExtensionErrorUI(
     ExtensionErrorUI* extension_error_ui) {
   bool needs_alert = false;
+
+  // Extensions that are blacklisted.
+  for (ExtensionSet::const_iterator it = blacklisted_extensions_.begin();
+       it != blacklisted_extensions_.end(); ++it) {
+    std::string id = (*it)->id();
+    if (!extension_prefs_->IsBlacklistedExtensionAcknowledged(id)) {
+      extension_error_ui->AddBlacklistedExtension(id);
+      needs_alert = true;
+    }
+  }
+
   for (ExtensionSet::const_iterator iter = extensions_.begin();
        iter != extensions_.end(); ++iter) {
     const Extension* e = *iter;
+
+    // Extensions disabled by policy. Note: this no longer includes blacklisted
+    // extensions, though we still show the same UI.
     if (!system_->management_policy()->UserMayLoad(e, NULL)) {
       if (!extension_prefs_->IsBlacklistedExtensionAcknowledged(e->id())) {
         extension_error_ui->AddBlacklistedExtension(e->id());
         needs_alert = true;
       }
     }
+
+    // Orphaned extensions.
     if (extension_prefs_->IsExtensionOrphaned(e->id())) {
       if (!extension_prefs_->IsOrphanedExtensionAcknowledged(e->id())) {
         extension_error_ui->AddOrphanedExtension(e->id());
@@ -1846,6 +1872,7 @@ bool ExtensionService::PopulateExtensionErrorUI(
       }
     }
   }
+
   return needs_alert;
 }
 
@@ -2085,7 +2112,17 @@ void ExtensionService::AddExtension(const Extension*
extension) {
   // wipeout before, we might disable this extension here.
   MaybeWipeout(extension);
 
-  if (extension_prefs_->IsExtensionDisabled(extension->id())) {
+  // Communicated to the Blacklist.
+  std::set<std::string> already_in_blacklist;
+
+  if (extension_prefs_->IsExtensionBlacklisted(extension->id())) {
+    // Don't check the Blacklist yet because it's asynchronous (we do it at
+    // the end). This pre-emptive check is because we will always store the
+    // blacklisted state of *installed* extensions in prefs, and it's important
+    // not to re-enable blacklisted extensions.
+    blacklisted_extensions_.Insert(extension);
+    already_in_blacklist.insert(extension->id());
+  } else if (extension_prefs_->IsExtensionDisabled(extension->id())) {
     disabled_extensions_.Insert(extension);
     SyncExtensionChangeIfNeeded(*extension);
     content::NotificationService::current()->Notify(
@@ -2097,20 +2134,29 @@ void ExtensionService::AddExtension(const Extension*
extension) {
         Extension::DISABLE_PERMISSIONS_INCREASE) {
       extensions::AddExtensionDisabledError(this, extension);
     }
-    return;
-  }
+  } else {
+    // All apps that are displayed in the launcher are ordered by their
ordinals
+    // so we must ensure they have valid ordinals.
+    if (extension->RequiresSortOrdinal()) {
+      extension_prefs_->extension_sorting()->EnsureValidOrdinals(
+          extension->id(), syncer::StringOrdinal());
+    }
 
-  // All apps that are displayed in the launcher are ordered by their ordinals
-  // so we must ensure they have valid ordinals.
-  if (extension->RequiresSortOrdinal()) {
-    extension_prefs_->extension_sorting()->EnsureValidOrdinals(
-        extension->id(), syncer::StringOrdinal());
+    extensions_.Insert(extension);
+    SyncExtensionChangeIfNeeded(*extension);
+    NotifyExtensionLoaded(extension);
+    DoPostLoadTasks(extension);
   }
 
-  extensions_.Insert(extension);
-  SyncExtensionChangeIfNeeded(*extension);
-  NotifyExtensionLoaded(extension);
-  DoPostLoadTasks(extension);
+  // Lastly, begin the process for checking the blacklist status of extensions.
+  // This may need to go to other threads so is asynchronous.
+  std::set<std::string> id_set;
+  id_set.insert(extension->id());
+  blacklist_->GetBlacklistedIDs(
+      id_set,
+      base::Bind(&ExtensionService::ManageBlacklist,
+                 AsWeakPtr(),
+                 already_in_blacklist));
 }
 
 void ExtensionService::AddComponentExtension(const Extension* extension) {
@@ -3031,5 +3077,47 @@ bool ExtensionService::ShouldDelayExtensionUpdate(
 }
 
 void ExtensionService::OnBlacklistUpdated() {
-  CheckManagementPolicy();
+  blacklist_->GetBlacklistedIDs(
+      GenerateInstalledExtensionsSet()->GetIDs(),
+      base::Bind(&ExtensionService::ManageBlacklist,
+                 AsWeakPtr(),
+                 blacklisted_extensions_.GetIDs()));
+}
+
+void ExtensionService::ManageBlacklist(
+    const std::set<std::string>& old_blacklisted_ids,
+    const std::set<std::string>& new_blacklisted_ids) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  std::set<std::string> no_longer_blacklisted;
+  std::set_difference(old_blacklisted_ids.begin(), old_blacklisted_ids.end(),
+                      new_blacklisted_ids.begin(), new_blacklisted_ids.end(),
+                      std::inserter(no_longer_blacklisted,
+                                    no_longer_blacklisted.begin()));
+  std::set<std::string> not_yet_blacklisted;
+  std::set_difference(new_blacklisted_ids.begin(), new_blacklisted_ids.end(),
+                      old_blacklisted_ids.begin(), old_blacklisted_ids.end(),
+                      std::inserter(not_yet_blacklisted,
+                                    not_yet_blacklisted.begin()));
+
+  for (std::set<std::string>::iterator it = no_longer_blacklisted.begin();
+       it != no_longer_blacklisted.end(); ++it) {
+    scoped_refptr<const Extension> extension =
+        blacklisted_extensions_.GetByID(*it);
+    if (!extension)
+      continue;
+    blacklisted_extensions_.Remove(*it);
+    AddExtension(extension);
+  }
+
+  for (std::set<std::string>::iterator it = not_yet_blacklisted.begin();
+       it != not_yet_blacklisted.end(); ++it) {
+    scoped_refptr<const Extension> extension = GetInstalledExtension(*it);
+    if (!extension)
+      continue;
+    blacklisted_extensions_.Insert(extension);
+    UnloadExtension(*it, exten…
(message too large)

[commit-bot: I haz the power, 2012-12-01 03:23:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/11415216/2005

[commit-bot: I haz the power, 2012-12-01 03:45:36]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[not at google - send to devlin, 2012-12-03 21:27:23]

+ivankr

https://chromiumcodereview.appspot.com/11415216/diff/2005/chrome/browser/prot...
File chrome/browser/protector/protected_prefs_watcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/11415216/diff/2005/chrome/browser/prot...
chrome/browser/protector/protected_prefs_watcher_unittest.cc:137:
EXPECT_TRUE(IsSignatureValid());
@ivankr this check is failing. Do you know what it's testing, and perhaps,
what's making it fail?

[commit-bot: I haz the power, 2012-12-04 02:00:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/11415216/14036

[commit-bot: I haz the power, 2012-12-04 02:00:20]

Failed to apply patch for chrome/browser/extensions/extension_service.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_service.cc
  Hunk #2 succeeded at 363 (offset -3 lines).
  Hunk #3 succeeded at 469 (offset -3 lines).
  Hunk #4 succeeded at 587 (offset -3 lines).
  Hunk #5 succeeded at 675 (offset -3 lines).
  Hunk #6 succeeded at 926 (offset -3 lines).
  Hunk #7 succeeded at 952 (offset -3 lines).
  Hunk #8 succeeded at 1003 (offset -3 lines).
  Hunk #9 succeeded at 1300 (offset -3 lines).
  Hunk #10 succeeded at 1864 (offset -3 lines).
  Hunk #11 succeeded at 1896 (offset -3 lines).
  Hunk #12 succeeded at 1976 (offset -3 lines).
  Hunk #13 succeeded at 2136 (offset -3 lines).
  Hunk #14 FAILED at 2161.
  Hunk #15 succeeded at 2244 (offset -10 lines).
  Hunk #16 succeeded at 2425 (offset -10 lines).
  Hunk #17 succeeded at 2537 (offset -10 lines).
  Hunk #18 succeeded at 2551 (offset -10 lines).
  Hunk #19 succeeded at 3052 (offset -10 lines).
  Hunk #20 succeeded at 3069 (offset -10 lines).
  1 out of 20 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_service.cc.rej

Patch:       chrome/browser/extensions/extension_service.cc
Index: chrome/browser/extensions/extension_service.cc
diff --git a/chrome/browser/extensions/extension_service.cc
b/chrome/browser/extensions/extension_service.cc
index
970873359b1de7408e5fc92521563b1eb7e3f9ac..390b3858e695940281db92dd8aeeb8b8ffa45fbd
100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/extensions/extension_service.h"
 
 #include <algorithm>
+#include <iterator>
 #include <set>
 
 #include "base/basictypes.h"
@@ -365,6 +366,7 @@ ExtensionService::ExtensionService(Profile* profile,
       profile_(profile),
       system_(extensions::ExtensionSystem::Get(profile)),
       extension_prefs_(extension_prefs),
+      blacklist_(blacklist),
       settings_frontend_(extensions::SettingsFrontend::Create(profile)),
       pending_extension_manager_(*ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       install_directory_(install_directory),
@@ -470,23 +472,29 @@ const ExtensionSet*
ExtensionService::terminated_extensions() const {
   return &terminated_extensions_;
 }
 
-const ExtensionSet* ExtensionService::GenerateInstalledExtensionsSet() const {
-  ExtensionSet* installed_extensions = new ExtensionSet();
+const ExtensionSet* ExtensionService::blacklisted_extensions() const {
+  return &blacklisted_extensions_;
+}
+
+scoped_ptr<const ExtensionSet>
+    ExtensionService::GenerateInstalledExtensionsSet() const {
+  scoped_ptr<ExtensionSet> installed_extensions(new ExtensionSet());
   installed_extensions->InsertAll(extensions_);
   installed_extensions->InsertAll(disabled_extensions_);
   installed_extensions->InsertAll(terminated_extensions_);
-  return installed_extensions;
+  installed_extensions->InsertAll(blacklisted_extensions_);
+  return installed_extensions.PassAs<const ExtensionSet>();
 }
 
-const ExtensionSet* ExtensionService::GetWipedOutExtensions() const {
-  ExtensionSet* extension_set = new ExtensionSet();
+scoped_ptr<const ExtensionSet> ExtensionService::GetWipedOutExtensions() const
{
+  scoped_ptr<ExtensionSet> extension_set(new ExtensionSet());
   for (ExtensionSet::const_iterator iter = disabled_extensions_.begin();
        iter != disabled_extensions_.end(); ++iter) {
     int disabled_reason = extension_prefs_->GetDisableReasons((*iter)->id());
     if ((disabled_reason & Extension::DISABLE_SIDELOAD_WIPEOUT) != 0)
       extension_set->Insert(*iter);
   }
-  return extension_set;
+  return extension_set.PassAs<const ExtensionSet>();
 }
 
 extensions::PendingExtensionManager*
@@ -582,9 +590,39 @@ void ExtensionService::Shutdown() {
 const Extension* ExtensionService::GetExtensionById(
     const std::string& id, bool include_disabled) const {
   int include_mask = INCLUDE_ENABLED;
-  if (include_disabled)
-    include_mask |= INCLUDE_DISABLED;
-  return GetExtensionByIdInternal(id, include_mask);
+  if (include_disabled) {
+    // Include blacklisted extensions here because there are hundreds of
+    // callers of this function, and many might assume that this includes those
+    // that have been disabled due to blacklisting.
+    include_mask |= INCLUDE_DISABLED | INCLUDE_BLACKLISTED;
+  }
+  return GetExtensionById(id, include_mask);
+}
+
+const Extension* ExtensionService::GetExtensionById(
+    const std::string& id, int include_mask) const {
+  std::string lowercase_id = StringToLowerASCII(id);
+  if (include_mask & INCLUDE_ENABLED) {
+    const Extension* extension = extensions_.GetByID(lowercase_id);
+    if (extension)
+      return extension;
+  }
+  if (include_mask & INCLUDE_DISABLED) {
+    const Extension* extension = disabled_extensions_.GetByID(lowercase_id);
+    if (extension)
+      return extension;
+  }
+  if (include_mask & INCLUDE_TERMINATED) {
+    const Extension* extension = terminated_extensions_.GetByID(lowercase_id);
+    if (extension)
+      return extension;
+  }
+  if (include_mask & INCLUDE_BLACKLISTED) {
+    const Extension* extension = blacklisted_extensions_.GetByID(lowercase_id);
+    if (extension)
+      return extension;
+  }
+  return NULL;
 }
 
 void ExtensionService::Init() {
@@ -640,9 +678,7 @@ bool ExtensionService::UpdateExtension(const std::string&
id,
   const extensions::PendingExtensionInfo* pending_extension_info =
       pending_extension_manager()->GetById(id);
 
-  int include_mask = INCLUDE_ENABLED | INCLUDE_DISABLED;
-  const Extension* extension =
-      GetExtensionByIdInternal(id, include_mask);
+  const Extension* extension = GetInstalledExtension(id);
   if (!pending_extension_info && !extension) {
     LOG(WARNING) << "Will not update extension " << id
                  << " because it is not installed or pending";
@@ -893,13 +929,15 @@ bool ExtensionService::IsExtensionEnabled(
     return true;
   }
 
-  if (disabled_extensions_.Contains(extension_id))
+  if (disabled_extensions_.Contains(extension_id) ||
+      blacklisted_extensions_.Contains(extension_id)) {
     return false;
+  }
 
   // If the extension hasn't been loaded yet, check the prefs for it. Assume
   // enabled unless otherwise noted.
   return !extension_prefs_->IsExtensionDisabled(extension_id) &&
-      !extension_prefs_->IsExternalExtensionUninstalled(extension_id);
+         !extension_prefs_->IsExternalExtensionUninstalled(extension_id);
 }
 
 bool ExtensionService::IsExternalExtensionUninstalled(
@@ -917,8 +955,7 @@ void ExtensionService::EnableExtension(const std::string&
extension_id) {
   extension_prefs_->SetExtensionState(extension_id, Extension::ENABLED);
   extension_prefs_->ClearDisableReasons(extension_id);
 
-  const Extension* extension = GetExtensionByIdInternal(extension_id,
-      INCLUDE_DISABLED);
+  const Extension* extension = disabled_extensions_.GetByID(extension_id);
   // This can happen if sync enables an extension that is not
   // installed yet.
   if (!extension)
@@ -969,8 +1006,8 @@ void ExtensionService::DisableExtension(
   extension_prefs_->SetExtensionState(extension_id, Extension::DISABLED);
   extension_prefs_->AddDisableReason(extension_id, disable_reason);
 
-  int include_mask = INCLUDE_ENABLED | INCLUDE_TERMINATED;
-  extension = GetExtensionByIdInternal(extension_id, include_mask);
+  int include_mask = INCLUDE_EVERYTHING & ~INCLUDE_DISABLED;
+  extension = GetExtensionById(extension_id, include_mask);
   if (!extension)
     return;
 
@@ -1266,6 +1303,7 @@ extensions::ExtensionUpdater* ExtensionService::updater()
{
 
 void ExtensionService::CheckManagementPolicy() {
   std::vector<std::string> to_be_removed;
+
   // Loop through extensions list, unload installed extensions.
   for (ExtensionSet::const_iterator iter = extensions_.begin();
        iter != extensions_.end(); ++iter) {
@@ -1829,15 +1867,31 @@ void ExtensionService::IdentifyAlertableExtensions() {
 bool ExtensionService::PopulateExtensionErrorUI(
     ExtensionErrorUI* extension_error_ui) {
   bool needs_alert = false;
+
+  // Extensions that are blacklisted.
+  for (ExtensionSet::const_iterator it = blacklisted_extensions_.begin();
+       it != blacklisted_extensions_.end(); ++it) {
+    std::string id = (*it)->id();
+    if (!extension_prefs_->IsBlacklistedExtensionAcknowledged(id)) {
+      extension_error_ui->AddBlacklistedExtension(id);
+      needs_alert = true;
+    }
+  }
+
   for (ExtensionSet::const_iterator iter = extensions_.begin();
        iter != extensions_.end(); ++iter) {
     const Extension* e = *iter;
+
+    // Extensions disabled by policy. Note: this no longer includes blacklisted
+    // extensions, though we still show the same UI.
     if (!system_->management_policy()->UserMayLoad(e, NULL)) {
       if (!extension_prefs_->IsBlacklistedExtensionAcknowledged(e->id())) {
         extension_error_ui->AddBlacklistedExtension(e->id());
         needs_alert = true;
       }
     }
+
+    // Orphaned extensions.
     if (extension_prefs_->IsExtensionOrphaned(e->id())) {
       if (!extension_prefs_->IsOrphanedExtensionAcknowledged(e->id())) {
         extension_error_ui->AddOrphanedExtension(e->id());
@@ -1845,6 +1899,7 @@ bool ExtensionService::PopulateExtensionErrorUI(
       }
     }
   }
+
   return needs_alert;
 }
 
@@ -1924,9 +1979,9 @@ void ExtensionService::UnloadExtension(
     const std::string& extension_id,
     extension_misc::UnloadedExtensionReason reason) {
   // Make sure the extension gets deleted after we return from this function.
-  int include_mask = INCLUDE_ENABLED | INCLUDE_DISABLED;
+  int include_mask = INCLUDE_EVERYTHING & ~INCLUDE_TERMINATED;
   scoped_refptr<const Extension> extension(
-      GetExtensionByIdInternal(extension_id, include_mask));
+      GetExtensionById(extension_id, include_mask));
 
   // This method can be called via PostTask, so t…
(message too large)

[commit-bot: I haz the power, 2012-12-04 02:48:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/11415216/11063

[commit-bot: I haz the power, 2012-12-04 03:54:25]

Retried try job too often on linux_rel for step(s) unit_tests

[commit-bot: I haz the power, 2012-12-04 06:46:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/11415216/11067

[commit-bot: I haz the power, 2012-12-04 08:44:55]

Retried try job too often on mac_rel for step(s) unit_tests

[Ivan Korotkov, 2012-12-04 12:16:20]

https://chromiumcodereview.appspot.com/11415216/diff/2005/chrome/browser/prot...
File chrome/browser/protector/protected_prefs_watcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/11415216/diff/2005/chrome/browser/prot...
chrome/browser/protector/protected_prefs_watcher_unittest.cc:137:
EXPECT_TRUE(IsSignatureValid());
On 2012/12/03 21:27:23, kalman wrote:
> @ivankr this check is failing. Do you know what it's testing, and perhaps,
> what's making it fail?

ProtectedPrefsWatcher maintains a copy of installed (non-blacklisted) extension
IDs and watches for its updates. Looks like some expectations it relied upon
changed. Feel free to disable the test. It's not enabled by defaylt and we're
soon going to remove this code altogether anyway.

[Ivan Korotkov, 2012-12-04 12:16:48]

That said, lgtm

[commit-bot: I haz the power, 2012-12-04 18:19:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/11415216/4101

[commit-bot: I haz the power, 2012-12-04 23:20:33]

Change committed as 171079