Add a Restore() method to ValueStore and make StorageAPI use it

Add a Restore() method to ValueStore and make StorageAPI use it
Take the first steps in preventing the Storage from getting and staying
corrupted. This lets us Restore() a ValueStore when it's corrputed, and tries to
do so in the StorageAPI calls.
This also refactors so that there's a Local and Sync SettingsBackend - this has
the added advantage of meaning we don't decorate the Local stores with Sync
behavior.
BUG=261623
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252617

[Devlin, 2014-02-13 23:52:17]

On 2014/02/13 23:51:13, D Cronin wrote:

Sorry for the review size. :/  On the upside, I think it's shorter than the bug!

[not at google - send to devlin, 2014-02-14 19:35:56]

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/leveldb_settings_storage_factory.h
(right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/leveldb_settings_storage_factory.h:20:
const std::string& extension_id) OVERRIDE;
nit: blank line after this

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/local_storage_backend.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/local_storage_backend.cc:47:
GetStorage(extension_id);
Creating a storage area here just to delete it straight away seems like
overkill. Can you just delete it on the file system directly?

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/policy_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/policy_value_store.cc:165: return
delegate_->Restore();
We should be able to restore from policy?

+joaodasilva

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/settings_backend.h (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_backend.h:46: const
SettingsStorageQuotaEnforcer::Limits& quota() const { return quota_; }
It looks like these accessors can be protected, but, protected things imply
complex inheritance structure which is rarely a good thing.

That said, one of the purpose of these "storage factory" classes is to be able
to pull the implementation details of creating storage areas out of the
backends.

see comment in sync_storage_backend.cc about this.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc
(right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc:230: //
If we failed, we can't calculate the usage.
nit: comment seems like it's in the wrong place, but if you reverse the
condition it would look right. It might also be nice to say like "That's ok
because next Restore(), if it succeeds, will need to recalculate the storage
anyway".

Which might imply that we should free the usage entirely (i.e. reset the usage
counter) just to free up resources and put ourselves in a predictable state.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc:238: if
(delegate_->RestoreKey(key)) {
nit: this would also look neater if you reversed the condition

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/storage_api.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_api.cc:79: if
(result->error().code == ValueStore::CORRUPTION &&
result->IsCorruption()

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_api.cc:87: base::DictionaryValue*
dict = new base::DictionaryValue();
what was wrong with read_result vs result?

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_api.cc:96: if
(result->error().code == ValueStore::CORRUPTION &&
ditto

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_api.cc:119:
tried_restoring_storage_ = true;
I'd rather this state be entirely handled within this method; rename this
"OnError", "HandleError", something, then instead of these DCHECKs have like

bool SettingsFunction::HandleError(const ValueStore::Error& error, ValueStore*
storage) {
  if (is_handling_error_)
    return false;
  is_handling_error_ = true;

  switch (error.code) {
    case ValueStore::CORRUPTION:
      // all the stuff you've written here.
      return ...;

    case ValueStore::...:
    case ValueStore::...:
     ...
     return ...;
  }

  NOTREACHED();
}

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/sync_or_local_value_store_cache.cc
(right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/sync_or_local_value_store_cache.cc:56:
DCHECK(app_backend_.get());
all this DCHECK'ing of backends looks weird. Let's just have an |initialized_|
boolean.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/sync_or_local_value_store_cache.cc:109:
const bool local = settings_namespace_ == settings_namespace::LOCAL;
use switch

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/sync_storage_backend.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/sync_storage_backend.cc:76:
ValueStore::ReadResult result = storage->Get();
corruption seems pretty rare, and there isn't much harm in creating a quota
enforcer over a corrupt database. so, more of this logic is common with the
local storage backend than just this (I suppose that's why it was combined
before).

What I would imagine: the factory still does the work (but curries in the base
path).

the local storage backend would just create the storage.

this class would create the storage, try to read, restore on failure, then
re-create.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/syncable_settings_storage.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/syncable_settings_storage.cc:127:
StopSyncing();
For this method and below:

I would have thought that
(1) if restore is successful and
(2) we're syncing

then this class can actually properly restore its state from the sync processor.

did you plan to do this in a follow-up? Because maybe that would be best (though
add a TODO) since I already have this change in my head :)

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
File chrome/browser/value_store/leveldb_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store.cc:303: if (result->HasError() &&
result->error().code == CORRUPTION) {
all my talk of making corruption a switch... turns out this is a pretty frequent
pattern. how about adding a result->IsCorruption() method?

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store.cc:314: return
!(result->HasError() && result->error().code == CORRUPTION);
return !result->HasError() || result->error().code != CORRUPTION;

though see above.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store.cc:318:
scoped_ptr<ValueStore::Error> error = WriteToDb(batch);
inline this

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
File chrome/browser/value_store/leveldb_value_store_unittest.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store_unittest.cc:30:
testing::Values(&Param));
is there a test we can add to settings frontend too, to make sure this work
end-to-end?

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store_unittest.cc:42:
testing::Test::SetUp();
I don't think you're expected to do this.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store_unittest.cc:56: void OpenStore()
{ store_.reset(new LeveldbValueStore(database_path())); }
these aren't accessors so make them proper functions (i.e. no inlining)

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store_unittest.cc:65: // Need these so
that the DCHECKs for running on FILE or UI threads pass.
comment not necessary

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store_unittest.cc:68:
content::TestBrowserThread file_thread_;
can you just use a single content::TestBrowserThreadBundle?

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
File chrome/browser/value_store/testing_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/testing_value_store.cc:130: while (result->HasError()
&& result->error().code == CORRUPTION) {
How does this class get corrupted storage?

I'm surprised how complex these methods are. I would have thought they're
basically no-ops.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
File chrome/browser/value_store/value_store.h (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/value_store.h:201: // Note: This method needs to read
(and possibly delete) the whole database,
why does it need to read the whole database?

[Devlin, 2014-02-18 23:55:22]

Sorry for the delay - a few of those took a bit more than I expected.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/leveldb_settings_storage_factory.h
(right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/leveldb_settings_storage_factory.h:20:
const std::string& extension_id) OVERRIDE;
On 2014/02/14 19:35:56, kalman wrote:
> nit: blank line after this

Done.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/local_storage_backend.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/local_storage_backend.cc:47:
GetStorage(extension_id);
On 2014/02/14 19:35:56, kalman wrote:
> Creating a storage area here just to delete it straight away seems like
> overkill. Can you just delete it on the file system directly?

Initially, I thought there was more cleanup involved when we deleted a db. 
Apparently not.  Makes it much simpler.  Thanks :)

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/policy_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/policy_value_store.cc:165: return
delegate_->Restore();
On 2014/02/14 19:35:56, kalman wrote:
> We should be able to restore from policy?
> 
> +joaodasilva

I think so, yes.  The Policy stores, even though they're read-only, are backed
by a normal store.  If they get corrupted (which will hopefully be less likely,
but could still happen in weird cases, like the windows restore bug), I think we
still need the ability to wipe them.

But I'll defer to Joao's knowledge, here.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/settings_backend.h (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_backend.h:46: const
SettingsStorageQuotaEnforcer::Limits& quota() const { return quota_; }
On 2014/02/14 19:35:56, kalman wrote:
> It looks like these accessors can be protected, but, protected things imply
> complex inheritance structure which is rarely a good thing.
> 
> That said, one of the purpose of these "storage factory" classes is to be able
> to pull the implementation details of creating storage areas out of the
> backends.
> 
> see comment in sync_storage_backend.cc about this.

Done.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc
(right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc:230: //
If we failed, we can't calculate the usage.
On 2014/02/14 19:35:56, kalman wrote:
> nit: comment seems like it's in the wrong place, but if you reverse the
> condition it would look right. It might also be nice to say like "That's ok
> because next Restore(), if it succeeds, will need to recalculate the storage
> anyway".
> 
> Which might imply that we should free the usage entirely (i.e. reset the usage
> counter) just to free up resources and put ourselves in a predictable state.

Done.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc:238: if
(delegate_->RestoreKey(key)) {
On 2014/02/14 19:35:56, kalman wrote:
> nit: this would also look neater if you reversed the condition

Done.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/storage_api.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_api.cc:79: if
(result->error().code == ValueStore::CORRUPTION &&
On 2014/02/14 19:35:56, kalman wrote:
> result->IsCorruption()

Unneeded now.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_api.cc:87: base::DictionaryValue*
dict = new base::DictionaryValue();
On 2014/02/14 19:35:56, kalman wrote:
> what was wrong with read_result vs result?

It was declared as |result| in the .h file
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...),
so, to make the definition match the declaration, I picked |result|, since it
happened to match UseWriteResult.  I can instead switch the declaration to be
read_result, if you'd prefer. :)

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_api.cc:96: if
(result->error().code == ValueStore::CORRUPTION &&
On 2014/02/14 19:35:56, kalman wrote:
> ditto

Unneeded now.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_api.cc:119:
tried_restoring_storage_ = true;
On 2014/02/14 19:35:56, kalman wrote:
> I'd rather this state be entirely handled within this method; rename this
> "OnError", "HandleError", something, then instead of these DCHECKs have like
> 
> bool SettingsFunction::HandleError(const ValueStore::Error& error, ValueStore*
> storage) {
>   if (is_handling_error_)
>     return false;
>   is_handling_error_ = true;
> 
>   switch (error.code) {
>     case ValueStore::CORRUPTION:
>       // all the stuff you've written here.
>       return ...;
> 
>     case ValueStore::...:
>     case ValueStore::...:
>      ...
>      return ...;
>   }
> 
>   NOTREACHED();
> }

Done (mostly - there's not really a need for a switch() since we only handle
CORRUPTION).

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/sync_or_local_value_store_cache.cc
(right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/sync_or_local_value_store_cache.cc:56:
DCHECK(app_backend_.get());
On 2014/02/14 19:35:56, kalman wrote:
> all this DCHECK'ing of backends looks weird. Let's just have an |initialized_|
> boolean.

Done.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/sync_or_local_value_store_cache.cc:109:
const bool local = settings_namespace_ == settings_namespace::LOCAL;
On 2014/02/14 19:35:56, kalman wrote:
> use switch

Done.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/sync_storage_backend.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/sync_storage_backend.cc:76:
ValueStore::ReadResult result = storage->Get();
On 2014/02/14 19:35:56, kalman wrote:
> corruption seems pretty rare, and there isn't much harm in creating a quota
> enforcer over a corrupt database. so, more of this logic is common with the
> local storage backend than just this (I suppose that's why it was combined
> before).
> 
> What I would imagine: the factory still does the work (but curries in the base
> path).
> 
> the local storage backend would just create the storage.
> 
> this class would create the storage, try to read, restore on failure, then
> re-create.

Oooh, good point.  We can actually do more, because I forgot that QuotaEnforcer
has to read the full ValueStore at construction anyway. So we can actually have
QuotaEnforcer restore the DB, and then put almost all creation in
SettingsBackend. Thanks :)

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/syncable_settings_storage.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/syncable_settings_storage.cc:127:
StopSyncing();
On 2014/02/14 19:35:56, kalman wrote:
> For this method and below:
> 
> I would have thought that
> (1) if restore is successful and
> (2) we're syncing
> 
> then this class can actually properly restore its state from the sync
processor.
> 
> did you plan to do this in a follow-up? Because maybe that would be best
(though
> add a TODO) since I already have this change in my head :)

Right now, my understanding of what will happen is:
- We Restore() (hopefully successfully), and lose some (or all) data.
- At next startup, we'll sync, and get back any data we can because we take the
union of everything in sync and on disk, and we didn't break anything on sync
(hopefully).

It would be better if we can trigger this sync automatically, but I'm not sure
there's a way to do that.  Similarly, it might work that we could StopSyncing(),
Restore(), and StartSyncing().  I'll investigate more and find out.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
File chrome/browser/value_store/leveldb_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store.cc:303: if (result->HasError() &&
result->error().code == CORRUPTION) {
On 2014/02/14 19:35:56, kalman wrote:
> all my talk of making corruption a switch... turns out this is a pretty
frequent
> pattern. how about adding a result->IsCorruption() method?

Done.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store.cc:314: return
!(result->HasError() && result->error().code == CORRUPTION);
On 2014/02/14 19:35:56, kalman wrote:
> return !result->HasError() || result->error().code != CORRUPTION;
> 
> though see above.

Done.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store.cc:318:
scoped_ptr<ValueStore::Error> error = WriteToDb(batch);
On 2014/02/14 19:35:56, kalman wrote:
> inline this

Done.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
File chrome/browser/value_store/leveldb_value_store_unittest.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store_unittest.cc:30:
testing::Values(&Param));
On 2014/02/14 19:35:56, kalman wrote:
> is there a test we can add to settings frontend too, to make sure this work
> end-to-end?

Wow.  That was more work than I expected.  But I agree an end-to-end test is
useful.

Unfortunately, to test end-to-end, we need to go higher than SettingsFrontend,
since that really wouldn't do much more than test the GetStorage() function
(since all the real logic is in SettingsFunction).  So, made a
StorageApiUnittest, which hopefully does the trick.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store_unittest.cc:42:
testing::Test::SetUp();
On 2014/02/14 19:35:56, kalman wrote:
> I don't think you're expected to do this.

We definitely need to for tests which inherit from anything other than
testing::Test (e.g. ExtensionUnittest), and and I think that we encourage it
just because if we ever, at some point, put something into
testing::Test::SetUp(), we don't have to then adjust several hundred tests.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store_unittest.cc:56: void OpenStore()
{ store_.reset(new LeveldbValueStore(database_path())); }
On 2014/02/14 19:35:56, kalman wrote:
> these aren't accessors so make them proper functions (i.e. no inlining)

Don't we usually do this on tests?  Most I see (at least, those without a header
file) have the functions defined in-class... Should we not be doing this?

Unless you're referring to the formatting.  In which case I invite you to join
me in cursing the new "git cl format".

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store_unittest.cc:65: // Need these so
that the DCHECKs for running on FILE or UI threads pass.
On 2014/02/14 19:35:56, kalman wrote:
> comment not necessary

Done.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store_unittest.cc:68:
content::TestBrowserThread file_thread_;
On 2014/02/14 19:35:56, kalman wrote:
> can you just use a single content::TestBrowserThreadBundle?

Done.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
File chrome/browser/value_store/testing_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/testing_value_store.cc:130: while (result->HasError()
&& result->error().code == CORRUPTION) {
On 2014/02/14 19:35:56, kalman wrote:
> How does this class get corrupted storage?
> 
> I'm surprised how complex these methods are. I would have thought they're
> basically no-ops.

Yeah...this might be overkill.  My basic thought was that I wanted the
TestingValueStore to behave as much as possible like a real value store, so that
if we wanted to instigate corruption for testing reasons, it would still behave
like the other stores.  Not worth it?

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
File chrome/browser/value_store/value_store.h (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/value_store.h:201: // Note: This method needs to read
(and possibly delete) the whole database,
On 2014/02/14 19:35:56, kalman wrote:
> why does it need to read the whole database?

Because it relies on Get() to ensure that there's no corruption, which reads
everything in the database.  I put in RestoreKey() as a more fine-grained
approach, but sometimes there's no specific key at fault, and we have to blow it
all away.  The only way to make sure the restore succeeded, though, is to Get()
it. :/

[not at google - send to devlin, 2014-02-18 23:58:13]

(Before I forget: +joao for real)

[Matt Perry, 2014-02-19 01:34:22]

I only looked at the value_store changes. Mostly LGTM, with 1 thing that needs
fixing.

https://codereview.chromium.org/165223003/diff/170001/chrome/browser/value_st...
File chrome/browser/value_store/leveldb_value_store_unittest.cc (right):

https://codereview.chromium.org/165223003/diff/170001/chrome/browser/value_st...
chrome/browser/value_store/leveldb_value_store_unittest.cc:41:
CHECK(database_dir_.CreateUniqueTempDir());
Don't use CHECK in a unittest. It'll crash the whole suite and prevent other
tests from running. If ASSERT_TRUE doesn't work here, maybe set some flag like
successful_setup and assert that in each test?

https://codereview.chromium.org/165223003/diff/170001/chrome/browser/value_st...
File chrome/browser/value_store/value_store.h (right):

https://codereview.chromium.org/165223003/diff/170001/chrome/browser/value_st...
chrome/browser/value_store/value_store.h:80: bool IsCorruption() const {
opt: HasCorruption or IsCorrupted would sound better IMO

[Joao da Silva, 2014-02-19 08:32:03]

policy_value_store changes lgtm

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/policy_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/policy_value_store.cc:165: return
delegate_->Restore();
On 2014/02/18 23:55:22, D Cronin wrote:
> On 2014/02/14 19:35:56, kalman wrote:
> > We should be able to restore from policy?
> > 
> > +joaodasilva
> 
> I think so, yes.  The Policy stores, even though they're read-only, are backed
> by a normal store.  If they get corrupted (which will hopefully be less
likely,
> but could still happen in weird cases, like the windows restore bug), I think
we
> still need the ability to wipe them.
> 
> But I'll defer to Joao's knowledge, here.

This is fine. The PolicyValueStore is used to detect policy changes across
browser restarts, and contains a copy of the policies loaded by Chrome.

So SetCurrentPolicy() will overwrite the whole thing again with sane values as
soon as Chrome reloads the policies again, or at the next restart.

[Devlin, 2014-02-19 17:43:18]

https://codereview.chromium.org/165223003/diff/170001/chrome/browser/value_st...
File chrome/browser/value_store/leveldb_value_store_unittest.cc (right):

https://codereview.chromium.org/165223003/diff/170001/chrome/browser/value_st...
chrome/browser/value_store/leveldb_value_store_unittest.cc:41:
CHECK(database_dir_.CreateUniqueTempDir());
On 2014/02/19 01:34:22, Matt Perry wrote:
> Don't use CHECK in a unittest. It'll crash the whole suite and prevent other
> tests from running. If ASSERT_TRUE doesn't work here, maybe set some flag like
> successful_setup and assert that in each test?

Done.

https://codereview.chromium.org/165223003/diff/170001/chrome/browser/value_st...
File chrome/browser/value_store/value_store.h (right):

https://codereview.chromium.org/165223003/diff/170001/chrome/browser/value_st...
chrome/browser/value_store/value_store.h:80: bool IsCorruption() const {
On 2014/02/19 01:34:22, Matt Perry wrote:
> opt: HasCorruption or IsCorrupted would sound better IMO

I've no preference, but I think Ben originally suggested IsCorruption() so it
matches the very-similar leveldb call. [1]  But if he doesn't care either, I'm
happy to change it to IsCorrupted().

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/leveld...

[not at google - send to devlin, 2014-02-19 17:45:14]

On 2014/02/19 17:43:18, D Cronin wrote:
>
https://codereview.chromium.org/165223003/diff/170001/chrome/browser/value_st...
> File chrome/browser/value_store/leveldb_value_store_unittest.cc (right):
> 
>
https://codereview.chromium.org/165223003/diff/170001/chrome/browser/value_st...
> chrome/browser/value_store/leveldb_value_store_unittest.cc:41:
> CHECK(database_dir_.CreateUniqueTempDir());
> On 2014/02/19 01:34:22, Matt Perry wrote:
> > Don't use CHECK in a unittest. It'll crash the whole suite and prevent other
> > tests from running. If ASSERT_TRUE doesn't work here, maybe set some flag
like
> > successful_setup and assert that in each test?
> 
> Done.
> 
>
https://codereview.chromium.org/165223003/diff/170001/chrome/browser/value_st...
> File chrome/browser/value_store/value_store.h (right):
> 
>
https://codereview.chromium.org/165223003/diff/170001/chrome/browser/value_st...
> chrome/browser/value_store/value_store.h:80: bool IsCorruption() const {
> On 2014/02/19 01:34:22, Matt Perry wrote:
> > opt: HasCorruption or IsCorrupted would sound better IMO
> 
> I've no preference, but I think Ben originally suggested IsCorruption() so it
> matches the very-similar leveldb call. [1]  But if he doesn't care either, I'm
> happy to change it to IsCorrupted().
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/leveld...

I don't feel strongly.

[Devlin, 2014-02-19 17:48:51]

https://codereview.chromium.org/165223003/diff/170001/chrome/browser/value_st...
File chrome/browser/value_store/value_store.h (right):

https://codereview.chromium.org/165223003/diff/170001/chrome/browser/value_st...
chrome/browser/value_store/value_store.h:80: bool IsCorruption() const {
On 2014/02/19 17:43:19, D Cronin wrote:
> On 2014/02/19 01:34:22, Matt Perry wrote:
> > opt: HasCorruption or IsCorrupted would sound better IMO
> 
> I've no preference, but I think Ben originally suggested IsCorruption() so it
> matches the very-similar leveldb call. [1]  But if he doesn't care either, I'm
> happy to change it to IsCorrupted().
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/leveld...

Done.

[not at google - send to devlin, 2014-02-19 17:55:58]

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/policy_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/policy_value_store.cc:165: return
delegate_->Restore();
On 2014/02/19 08:32:03, Joao da Silva wrote:
> On 2014/02/18 23:55:22, D Cronin wrote:
> > On 2014/02/14 19:35:56, kalman wrote:
> > > We should be able to restore from policy?
> > > 
> > > +joaodasilva
> > 
> > I think so, yes.  The Policy stores, even though they're read-only, are
backed
> > by a normal store.  If they get corrupted (which will hopefully be less
> likely,
> > but could still happen in weird cases, like the windows restore bug), I
think
> we
> > still need the ability to wipe them.
> > 
> > But I'll defer to Joao's knowledge, here.
> 
> This is fine. The PolicyValueStore is used to detect policy changes across
> browser restarts, and contains a copy of the policies loaded by Chrome.
> 
> So SetCurrentPolicy() will overwrite the whole thing again with sane values as
> soon as Chrome reloads the policies again, or at the next restart.

Ok. So there's no reason to restore them now? Devlin could you add a comment
here to that effect?

[Joao da Silva, 2014-02-19 18:05:43]

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/policy_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/policy_value_store.cc:165: return
delegate_->Restore();
> > This is fine. The PolicyValueStore is used to detect policy changes across
> > browser restarts, and contains a copy of the policies loaded by Chrome.
> > 
> > So SetCurrentPolicy() will overwrite the whole thing again with sane values
as
> > soon as Chrome reloads the policies again, or at the next restart.
> 
> Ok. So there's no reason to restore them now? Devlin could you add a comment
> here to that effect?

I don't know how LevelDB handles corrupted databases. Will it recover if the
corrupted values are written again? Or are there circumstances where those
writes would fail? Could the database fail to open on a subsequent restart?

I'd keep the Restore() calls to the delegate to be on the safe side given my
ignorance of how LevelDB works. You know the database better, so I defer a
decision to you :-)

[not at google - send to devlin, 2014-02-19 18:13:53]

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/policy_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/policy_value_store.cc:165: return
delegate_->Restore();
On 2014/02/19 18:05:43, Joao da Silva wrote:
> > > This is fine. The PolicyValueStore is used to detect policy changes across
> > > browser restarts, and contains a copy of the policies loaded by Chrome.
> > > 
> > > So SetCurrentPolicy() will overwrite the whole thing again with sane
values
> as
> > > soon as Chrome reloads the policies again, or at the next restart.
> > 
> > Ok. So there's no reason to restore them now? Devlin could you add a comment
> > here to that effect?
> 
> I don't know how LevelDB handles corrupted databases. Will it recover if the
> corrupted values are written again? Or are there circumstances where those
> writes would fail? Could the database fail to open on a subsequent restart?
> 
> I'd keep the Restore() calls to the delegate to be on the safe side given my
> ignorance of how LevelDB works. You know the database better, so I defer a
> decision to you :-)

Restore on the leveldb instance is really just force-clear. To actually restore
data properly it will need to pull state from policy after restore.

So I think eventually you will need to pull from policy on restore, ideally.
Same for RestoreKey. Otherwise it will give spurious change events on startup.

However the same can be said for sync, and I commented there that we might want
to just do that in a follow up.

[Devlin, 2014-02-19 18:36:15]

On 2014/02/19 18:05:43, Joao da Silva wrote:
>
https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
> File chrome/browser/extensions/api/storage/policy_value_store.cc (right):
> 
>
https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
> chrome/browser/extensions/api/storage/policy_value_store.cc:165: return
> delegate_->Restore();
> > > This is fine. The PolicyValueStore is used to detect policy changes across
> > > browser restarts, and contains a copy of the policies loaded by Chrome.
> > > 
> > > So SetCurrentPolicy() will overwrite the whole thing again with sane
values
> as
> > > soon as Chrome reloads the policies again, or at the next restart.
> > 
> > Ok. So there's no reason to restore them now? Devlin could you add a comment
> > here to that effect?
> 
> I don't know how LevelDB handles corrupted databases. Will it recover if the
> corrupted values are written again? Or are there circumstances where those
> writes would fail? Could the database fail to open on a subsequent restart?
> 
> I'd keep the Restore() calls to the delegate to be on the safe side given my
> ignorance of how LevelDB works. You know the database better, so I defer a
> decision to you :-)

I think that we may still need/want these Restore() methods.  In particular,
even though we reset the policies at each restart of chrome, SetCurrentPolicy()
relies upon ValueStore::[Get|Remove|Set](), which can all fail if the DB is
corrupted.  Put in a comment saying that we hopefully won't need them as a RO
DB, but left them in, and added a call to Restore() iff we find corruption in
SetCurrentPolicy().

[not at google - send to devlin, 2014-02-19 18:40:36]

sorry to keep banging on about the policy stuff. I'll get to the rest of the
change in a minute :)

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/policy_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/policy_value_store.cc:64: read_result =
delegate_->Get();
I still don't think this is quite right though, because now it means that down
below where the changes are calculated the extension will get told that every
key was added.

However maybe it's better to have that behaviour vs not saying anything at all,
just in case.

Anyway it's more subtle than "if the database is corrupted, try to restore it".

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/policy_value_store.cc:171: bool
PolicyValueStore::Restore() { return delegate_->Restore(); }
GRR GIT CL FORMAT IS WRONG.

anyhow, it seems like this should be

bool PolicyVaueStore::Restore() {
  if delegate_->Restore()) {
    delegate_->Set(/* the current policy state */);
    return true;
  }
  return false;
}

? or am I missing something?

[not at google - send to devlin, 2014-02-19 18:48:26]

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/policy_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/policy_value_store.cc:171: bool
PolicyValueStore::Restore() { return delegate_->Restore(); }
On 2014/02/19 18:40:37, kalman wrote:
> GRR GIT CL FORMAT IS WRONG.
> 
> anyhow, it seems like this should be
> 
> bool PolicyVaueStore::Restore() {
>   if delegate_->Restore()) {
>     delegate_->Set(/* the current policy state */);
>     return true;
>   }
>   return false;
> }
> 
> ? or am I missing something?

I suppose if Restore actually does manage to restore it then you wouldn't need
to read in from store.

But if Restore ends up clearing then you'd need to do the equivalent of the code
I just wrote.

But if Restore fails at everything you don't want to touch it at all.

So maybe Restore needs to be an enum :)

[Devlin, 2014-02-19 19:13:55]

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/policy_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/policy_value_store.cc:64: read_result =
delegate_->Get();
On 2014/02/19 18:40:37, kalman wrote:
> I still don't think this is quite right though, because now it means that down
> below where the changes are calculated the extension will get told that every
> key was added.
> 
> However maybe it's better to have that behaviour vs not saying anything at
all,
> just in case.
> 
> Anyway it's more subtle than "if the database is corrupted, try to restore
it".

Yes, the extension will be notified of all keys being added, but I don't think
that it's actually new behavior.  Previously (and still, if it's an error other
than CORRUPTION), we just don't swap in the previous policy, so we'd still be
notified of every new key.  Isn't this behavior basically the same (we found an
error, we couldn't get the old stuff, so we say everything's added)?

Regardless, I'll add a comment to make it explicitly clear.

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/policy_value_store.cc:171: bool
PolicyValueStore::Restore() { return delegate_->Restore(); }
On 2014/02/19 18:48:26, kalman wrote:
> On 2014/02/19 18:40:37, kalman wrote:
> > GRR GIT CL FORMAT IS WRONG.
> > 
> > anyhow, it seems like this should be
> > 
> > bool PolicyVaueStore::Restore() {
> >   if delegate_->Restore()) {
> >     delegate_->Set(/* the current policy state */);
> >     return true;
> >   }
> >   return false;
> > }
> > 
> > ? or am I missing something?
> 
> I suppose if Restore actually does manage to restore it then you wouldn't need
> to read in from store.
> 
> But if Restore ends up clearing then you'd need to do the equivalent of the
code
> I just wrote.
> 
> But if Restore fails at everything you don't want to touch it at all.
> 
> So maybe Restore needs to be an enum :)

Ideally, yes, Restore() would give more information about what actually
happened.  But we wouldn't be able to use an enum (or at least, not just an
enum).  Restore(), while more intelligent than immediately blowing away the
database, still doesn't actually "restore" data - it just selectively removes
corrupted pieces.  But the smallest chunk ever removed is a full key.  So the
return value would need to indicate:
- Did we succeed?
- Did we remove everything?
  - If no, which keys did we remove?

Then, whatever called restore could make an effort to reconcile the damage.

Unfortunately, I think this might be too much to cram into this already-large
change (but if you think it's worth adding it now, I'm happy to).  But it
shouldn't be disastrous in either case where we could get storage back (here and
sync).  In both cases, we'll reconcile the storage once we restart Chrome.  Not
ideal, but hopefully better.

[not at google - send to devlin, 2014-02-19 19:21:27]

On 2014/02/19 19:13:55, D Cronin wrote:
>
https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
> File chrome/browser/extensions/api/storage/policy_value_store.cc (right):
> 
>
https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
> chrome/browser/extensions/api/storage/policy_value_store.cc:64: read_result =
> delegate_->Get();
> On 2014/02/19 18:40:37, kalman wrote:
> > I still don't think this is quite right though, because now it means that
down
> > below where the changes are calculated the extension will get told that
every
> > key was added.
> > 
> > However maybe it's better to have that behaviour vs not saying anything at
> all,
> > just in case.
> > 
> > Anyway it's more subtle than "if the database is corrupted, try to restore
> it".
> 
> Yes, the extension will be notified of all keys being added, but I don't think
> that it's actually new behavior.  Previously (and still, if it's an error
other
> than CORRUPTION), we just don't swap in the previous policy, so we'd still be
> notified of every new key.  Isn't this behavior basically the same (we found
an
> error, we couldn't get the old stuff, so we say everything's added)?
> 
> Regardless, I'll add a comment to make it explicitly clear.
> 
>
https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
> chrome/browser/extensions/api/storage/policy_value_store.cc:171: bool
> PolicyValueStore::Restore() { return delegate_->Restore(); }
> On 2014/02/19 18:48:26, kalman wrote:
> > On 2014/02/19 18:40:37, kalman wrote:
> > > GRR GIT CL FORMAT IS WRONG.
> > > 
> > > anyhow, it seems like this should be
> > > 
> > > bool PolicyVaueStore::Restore() {
> > >   if delegate_->Restore()) {
> > >     delegate_->Set(/* the current policy state */);
> > >     return true;
> > >   }
> > >   return false;
> > > }
> > > 
> > > ? or am I missing something?
> > 
> > I suppose if Restore actually does manage to restore it then you wouldn't
need
> > to read in from store.
> > 
> > But if Restore ends up clearing then you'd need to do the equivalent of the
> code
> > I just wrote.
> > 
> > But if Restore fails at everything you don't want to touch it at all.
> > 
> > So maybe Restore needs to be an enum :)
> 
> Ideally, yes, Restore() would give more information about what actually
> happened.  But we wouldn't be able to use an enum (or at least, not just an
> enum).  Restore(), while more intelligent than immediately blowing away the
> database, still doesn't actually "restore" data - it just selectively removes
> corrupted pieces.  But the smallest chunk ever removed is a full key.  So the
> return value would need to indicate:
> - Did we succeed?
> - Did we remove everything?
>   - If no, which keys did we remove?
> 
> Then, whatever called restore could make an effort to reconcile the damage.
> 
> Unfortunately, I think this might be too much to cram into this already-large
> change (but if you think it's worth adding it now, I'm happy to).  But it
> shouldn't be disastrous in either case where we could get storage back (here
and
> sync).  In both cases, we'll reconcile the storage once we restart Chrome. 
Not
> ideal, but hopefully better.

Maybe "Recover" would have been a better term for this function come to think of
it.

But yes, this change is big enough for now already.

[not at google - send to devlin, 2014-02-19 21:25:28]

lgtm with a bunch more comments but mostly minor.

also: you were going to add comments in some places :)

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/syncable_settings_storage.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/syncable_settings_storage.cc:127:
StopSyncing();
On 2014/02/18 23:55:22, D Cronin wrote:
> On 2014/02/14 19:35:56, kalman wrote:
> > For this method and below:
> > 
> > I would have thought that
> > (1) if restore is successful and
> > (2) we're syncing
> > 
> > then this class can actually properly restore its state from the sync
> processor.
> > 
> > did you plan to do this in a follow-up? Because maybe that would be best
> (though
> > add a TODO) since I already have this change in my head :)
> 
> Right now, my understanding of what will happen is:
> - We Restore() (hopefully successfully), and lose some (or all) data.
> - At next startup, we'll sync, and get back any data we can because we take
the
> union of everything in sync and on disk, and we didn't break anything on sync
> (hopefully).
> 
> It would be better if we can trigger this sync automatically, but I'm not sure
> there's a way to do that.  Similarly, it might work that we could
StopSyncing(),
> Restore(), and StartSyncing().  I'll investigate more and find out.

Cool. This is basically the same discussion we had with the policy store then.
What you have is fine, just, I think the behaviour is subtle enough to warrant a
slightly more helpful comment.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
File chrome/browser/value_store/leveldb_value_store_unittest.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store_unittest.cc:42:
testing::Test::SetUp();
On 2014/02/18 23:55:22, D Cronin wrote:
> On 2014/02/14 19:35:56, kalman wrote:
> > I don't think you're expected to do this.
> 
> We definitely need to for tests which inherit from anything other than
> testing::Test (e.g. ExtensionUnittest), and and I think that we encourage it
> just because if we ever, at some point, put something into
> testing::Test::SetUp(), we don't have to then adjust several hundred tests.

Yes for all cases other than testing::Test you would need to do this. But if
gtest decided to add important logic into SetUp then (a) it would break lots of
things and (b) it wouldn't make a whole lot of sense, since it's the entity
which calls SetUp.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store_unittest.cc:56: void OpenStore()
{ store_.reset(new LeveldbValueStore(database_path())); }
On 2014/02/18 23:55:22, D Cronin wrote:
> On 2014/02/14 19:35:56, kalman wrote:
> > these aren't accessors so make them proper functions (i.e. no inlining)
> 
> Don't we usually do this on tests?  Most I see (at least, those without a
header
> file) have the functions defined in-class... Should we not be doing this?
> 
> Unless you're referring to the formatting.  In which case I invite you to join
> me in cursing the new "git cl format".

Yes I was referring to the formatting :)

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
File chrome/browser/value_store/testing_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/testing_value_store.cc:130: while (result->HasError()
&& result->error().code == CORRUPTION) {
On 2014/02/18 23:55:22, D Cronin wrote:
> On 2014/02/14 19:35:56, kalman wrote:
> > How does this class get corrupted storage?
> > 
> > I'm surprised how complex these methods are. I would have thought they're
> > basically no-ops.
> 
> Yeah...this might be overkill.  My basic thought was that I wanted the
> TestingValueStore to behave as much as possible like a real value store, so
that
> if we wanted to instigate corruption for testing reasons, it would still
behave
> like the other stores.  Not worth it?

Not worth it.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
File chrome/browser/value_store/value_store.h (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/value_store.h:201: // Note: This method needs to read
(and possibly delete) the whole database,
On 2014/02/18 23:55:22, D Cronin wrote:
> On 2014/02/14 19:35:56, kalman wrote:
> > why does it need to read the whole database?
> 
> Because it relies on Get() to ensure that there's no corruption, which reads
> everything in the database.  I put in RestoreKey() as a more fine-grained
> approach, but sometimes there's no specific key at fault, and we have to blow
it
> all away.  The only way to make sure the restore succeeded, though, is to
Get()
> it. :/

Ok. That's an implementation detail though. It may need to Get(), or leveldb may
add some cool method down the line which sanity checks state.

I think a comment like "Note: this method may be expensive, for example, some
implementations may need to read the entire database in order to restore. Use
sparing."

along those lines.

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/leveldb_settings_storage_factory.h
(right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/leveldb_settings_storage_factory.h:20:
const std::string& extension_id) OVERRIDE;
as discussed: don't need this on the factory interface anymore, it can be in an
anon namepace in the cc file

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/settings_quota_unittest.cc (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/settings_quota_unittest.cc:594:
EXPECT_EQ(4u, storage_->GetBytesInUse(ab));
Another thing to do in a follow-up is write tests here.

To do that, add the ability to TestingValueStore to mimic corruption then make
sure it behaves properly.

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc
(right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc:252:
void SettingsStorageQuotaEnforcer::CalculateUsage() {
yeah, this is all complex enough to warrant testing

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/storage_api_unittest.cc (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/storage_api_unittest.cc:64: bool
RunGetFunction(const std::string& key, std::string* value) {
return a testing::AssertionResult here. It would look something like

testing::AssertionResult RunGetFunction(...) {
  scoped_ptr<base::Value> result = ...;
  base::DictionaryValue* dict = NULL;
  if (!result->GetAsDictionary(&dict))
    return testing::AssertionFailure() << result << " was not a dictionary";
  if (!dict->GetString(key, value))
    return testing::AssertionFailure() << dict << " did not contain key \"" <<
key << "\"";
  return testing::AssertionSuccess();

testing::AssertionResults are super cool.

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/storage_api_unittest.cc:98:
static_cast<LeveldbValueStore*>(quota_store->get_delegate_for_test());
this does make me very uneasy given there is no safety whatsoever here and there
are 2 assumptions about structure.

as we talked about in person, a safer way would be to directly corrupt the
on-disk database but that would require making sure it's closed after setting
those values, and the test infrastructure may not be factored in a way that
supports it.

also, it would be nice to test the full Restore end-to-end but I think that
would block on the database being closed (since you'd want to do what you do
with the leveldb storage tests and delete random files).

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/sync_storage_backend.h (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/sync_storage_backend.h:1: // Copyright
2014 The Chromium Authors. All rights reserved.
does some tool do this or did you make this change yourself?

because IIRC we're not supposed to update copyright years.

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/syncable_settings_storage.cc (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/syncable_settings_storage.cc:130: //
MergeDataAndStartSyncing() to immediately get back any data we can.
(and it would be very nice to continue to sync as well)

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/test_extension_system.cc (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/test_extension_system.cc:155: void
TestExtensionSystem::SetEventRouter(scoped_ptr<EventRouter> event_router) {
event routers are free to construct. I think you should just always create one
and save callers the bother of needing to call SetEventRouter.

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/value_st...
File chrome/browser/value_store/leveldb_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/value_st...
chrome/browser/value_store/leveldb_value_store.cc:296: return
!result->IsCorrupted();
I guess another complication with all this is if invalid keys make it into sync
or policy. I'm not sure how all of this would behave, particularly if we're
going to go and turn around and restore the data.

i'll add it to the list of things to look into later.

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/value_st...
File chrome/browser/value_store/testing_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/value_st...
chrome/browser/value_store/testing_value_store.cc:148: }
yeah I think this isn't really called for yet.

[Devlin, 2014-02-19 23:12:27]

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/syncable_settings_storage.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/storage/syncable_settings_storage.cc:127:
StopSyncing();
On 2014/02/19 21:25:29, kalman wrote:
> On 2014/02/18 23:55:22, D Cronin wrote:
> > On 2014/02/14 19:35:56, kalman wrote:
> > > For this method and below:
> > > 
> > > I would have thought that
> > > (1) if restore is successful and
> > > (2) we're syncing
> > > 
> > > then this class can actually properly restore its state from the sync
> > processor.
> > > 
> > > did you plan to do this in a follow-up? Because maybe that would be best
> > (though
> > > add a TODO) since I already have this change in my head :)
> > 
> > Right now, my understanding of what will happen is:
> > - We Restore() (hopefully successfully), and lose some (or all) data.
> > - At next startup, we'll sync, and get back any data we can because we take
> the
> > union of everything in sync and on disk, and we didn't break anything on
sync
> > (hopefully).
> > 
> > It would be better if we can trigger this sync automatically, but I'm not
sure
> > there's a way to do that.  Similarly, it might work that we could
> StopSyncing(),
> > Restore(), and StartSyncing().  I'll investigate more and find out.
> 
> Cool. This is basically the same discussion we had with the policy store then.
> What you have is fine, just, I think the behaviour is subtle enough to warrant
a
> slightly more helpful comment.

Already done on patch set 2.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
File chrome/browser/value_store/leveldb_value_store_unittest.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/leveldb_value_store_unittest.cc:42:
testing::Test::SetUp();
On 2014/02/19 21:25:29, kalman wrote:
> On 2014/02/18 23:55:22, D Cronin wrote:
> > On 2014/02/14 19:35:56, kalman wrote:
> > > I don't think you're expected to do this.
> > 
> > We definitely need to for tests which inherit from anything other than
> > testing::Test (e.g. ExtensionUnittest), and and I think that we encourage it
> > just because if we ever, at some point, put something into
> > testing::Test::SetUp(), we don't have to then adjust several hundred tests.
> 
> Yes for all cases other than testing::Test you would need to do this. But if
> gtest decided to add important logic into SetUp then (a) it would break lots
of
> things and (b) it wouldn't make a whole lot of sense, since it's the entity
> which calls SetUp.

Okay, done.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
File chrome/browser/value_store/testing_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/testing_value_store.cc:130: while (result->HasError()
&& result->error().code == CORRUPTION) {
On 2014/02/19 21:25:29, kalman wrote:
> On 2014/02/18 23:55:22, D Cronin wrote:
> > On 2014/02/14 19:35:56, kalman wrote:
> > > How does this class get corrupted storage?
> > > 
> > > I'm surprised how complex these methods are. I would have thought they're
> > > basically no-ops.
> > 
> > Yeah...this might be overkill.  My basic thought was that I wanted the
> > TestingValueStore to behave as much as possible like a real value store, so
> that
> > if we wanted to instigate corruption for testing reasons, it would still
> behave
> > like the other stores.  Not worth it?
> 
> Not worth it.

Done.

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
File chrome/browser/value_store/value_store.h (right):

https://codereview.chromium.org/165223003/diff/30001/chrome/browser/value_sto...
chrome/browser/value_store/value_store.h:201: // Note: This method needs to read
(and possibly delete) the whole database,
On 2014/02/19 21:25:29, kalman wrote:
> On 2014/02/18 23:55:22, D Cronin wrote:
> > On 2014/02/14 19:35:56, kalman wrote:
> > > why does it need to read the whole database?
> > 
> > Because it relies on Get() to ensure that there's no corruption, which reads
> > everything in the database.  I put in RestoreKey() as a more fine-grained
> > approach, but sometimes there's no specific key at fault, and we have to
blow
> it
> > all away.  The only way to make sure the restore succeeded, though, is to
> Get()
> > it. :/
> 
> Ok. That's an implementation detail though. It may need to Get(), or leveldb
may
> add some cool method down the line which sanity checks state.
> 
> I think a comment like "Note: this method may be expensive, for example, some
> implementations may need to read the entire database in order to restore. Use
> sparing."
> 
> along those lines.

Done.

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/leveldb_settings_storage_factory.h
(right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/leveldb_settings_storage_factory.h:20:
const std::string& extension_id) OVERRIDE;
On 2014/02/19 21:25:29, kalman wrote:
> as discussed: don't need this on the factory interface anymore, it can be in
an
> anon namepace in the cc file

Done.

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/settings_quota_unittest.cc (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/settings_quota_unittest.cc:594:
EXPECT_EQ(4u, storage_->GetBytesInUse(ab));
On 2014/02/19 21:25:29, kalman wrote:
> Another thing to do in a follow-up is write tests here.
> 
> To do that, add the ability to TestingValueStore to mimic corruption then make
> sure it behaves properly.

Adding to the wishlist...oh, you already did. :)

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc
(right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc:252:
void SettingsStorageQuotaEnforcer::CalculateUsage() {
On 2014/02/19 21:25:29, kalman wrote:
> yeah, this is all complex enough to warrant testing

Will do in follow-up, if that's okay.

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/storage_api_unittest.cc (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/storage_api_unittest.cc:64: bool
RunGetFunction(const std::string& key, std::string* value) {
On 2014/02/19 21:25:29, kalman wrote:
> return a testing::AssertionResult here. It would look something like
> 
> testing::AssertionResult RunGetFunction(...) {
>   scoped_ptr<base::Value> result = ...;
>   base::DictionaryValue* dict = NULL;
>   if (!result->GetAsDictionary(&dict))
>     return testing::AssertionFailure() << result << " was not a dictionary";
>   if (!dict->GetString(key, value))
>     return testing::AssertionFailure() << dict << " did not contain key \"" <<
> key << "\"";
>   return testing::AssertionSuccess();
> 
> testing::AssertionResults are super cool.

Done.

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/storage_api_unittest.cc:98:
static_cast<LeveldbValueStore*>(quota_store->get_delegate_for_test());
On 2014/02/19 21:25:29, kalman wrote:
> this does make me very uneasy given there is no safety whatsoever here and
there
> are 2 assumptions about structure.
> 
> as we talked about in person, a safer way would be to directly corrupt the
> on-disk database but that would require making sure it's closed after setting
> those values, and the test infrastructure may not be factored in a way that
> supports it.
> 
> also, it would be nice to test the full Restore end-to-end but I think that
> would block on the database being closed (since you'd want to do what you do
> with the leveldb storage tests and delete random files).

Per long in-person conversation, there's no pretty way of doing this.  This will
do until we clean more of this up.

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/sync_storage_backend.h (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/sync_storage_backend.h:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2014/02/19 21:25:29, kalman wrote:
> does some tool do this or did you make this change yourself?
> 
> because IIRC we're not supposed to update copyright years.

It's a new file (and thus gets the current year and style). It's just so close
to the previous SettingsBackend that Rietveld doesn't know it.

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/syncable_settings_storage.cc (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/syncable_settings_storage.cc:130: //
MergeDataAndStartSyncing() to immediately get back any data we can.
On 2014/02/19 21:25:29, kalman wrote:
> (and it would be very nice to continue to sync as well)

Comment updated.

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/test_extension_system.cc (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/test_extension_system.cc:155: void
TestExtensionSystem::SetEventRouter(scoped_ptr<EventRouter> event_router) {
On 2014/02/19 21:25:29, kalman wrote:
> event routers are free to construct. I think you should just always create one
> and save callers the bother of needing to call SetEventRouter.

EventRouter is free, but needs an ExtensionPrefs, which isn't. :/

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/value_st...
File chrome/browser/value_store/leveldb_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/value_st...
chrome/browser/value_store/leveldb_value_store.cc:296: return
!result->IsCorrupted();
On 2014/02/19 21:25:29, kalman wrote:
> I guess another complication with all this is if invalid keys make it into
sync
> or policy. I'm not sure how all of this would behave, particularly if we're
> going to go and turn around and restore the data.
> 
> i'll add it to the list of things to look into later.

Yeah...

I don't think that we can get a corrupted key _from_ a local copy of sync
storage to the cloud's copy of sync storage, because it relies on Get() to do so
(which would fail if it's corrupt).  But if the cloud version gets corrupted,
we're going to have a bad time.

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/value_st...
File chrome/browser/value_store/testing_value_store.cc (right):

https://codereview.chromium.org/165223003/diff/320001/chrome/browser/value_st...
chrome/browser/value_store/testing_value_store.cc:148: }
On 2014/02/19 21:25:29, kalman wrote:
> yeah I think this isn't really called for yet.

Done.

[Devlin, 2014-02-21 15:56:17]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-02-21 15:56:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/165223003/...

[commit-bot: I haz the power, 2014-02-21 17:12:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-21 17:12:19]

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

[Devlin, 2014-02-21 17:16:10]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-02-21 17:16:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/165223003/...

[commit-bot: I haz the power, 2014-02-21 18:23:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-21 18:23:29]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[commit-bot: I haz the power, 2014-02-21 18:23:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-21 18:23:37]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Devlin, 2014-02-21 18:27:16]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-02-21 18:27:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/165223003/...

[commit-bot: I haz the power, 2014-02-21 19:10:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/165223003/...

[commit-bot: I haz the power, 2014-02-21 19:39:59]

Change committed as 252617

[Devlin, 2014-02-21 21:38:45]

A revert of this CL has been created in
https://codereview.chromium.org/175853002/ by rdevlin.cronin@chromium.org.

The reason for reverting is: Broke win unit_tests with
StorageApiUnittest.RestoreStorage.