gdata: Fix a bug in GDataLevelDB where errors are not handled right

gdata: Fix a bug in GDataLevelDB where errors are not handled right

GDataEntry::FromProtoString() fails if incompatible proto is detected
We should return an error rather than DCHECK it.
See crbug.com/142912 for how incompatible proto is rejected.

BUG=127856
TEST=out/Release/unit_tests --gtest_filter=GDataDBTest.*

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

[satorux1, 2012-07-10 17:10:17]

https://chromiumcodereview.appspot.com/10701118/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/gdata/gdata_db_unittest.cc (right):

https://chromiumcodereview.appspot.com/10701118/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:205:
ASSERT_EQ(GDataDB::DB_OK, status);
change it to ASSERT. We shouldn't go further if DB operation fails.

[achuithb, 2012-07-10 17:21:18]

http://codereview.chromium.org/10701118/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_db.h (right):

http://codereview.chromium.org/10701118/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_db.h:60: const
google::protobuf::MessageLite& protobuf) = 0;
Any reason why we don't just pass in a string as the value to store in the db?
The protobuf is invalid anyway. It'll make the signature nicer and you don't
have to forward declare google::protobuf::MessageLite.

http://codereview.chromium.org/10701118/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_leveldb.cc (right):

http://codereview.chromium.org/10701118/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:143: if (!entry->get())
nit: can we make this a ternary return? It saves 2 lines:
return entry.get() ? DB_OK : DB_CORRUPTION;

[satorux1, 2012-07-10 17:28:32]

http://codereview.chromium.org/10701118/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_db.h (right):

http://codereview.chromium.org/10701118/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_db.h:60: const
google::protobuf::MessageLite& protobuf) = 0;
On 2012/07/10 17:21:19, achuith.bhandarkar wrote:
> Any reason why we don't just pass in a string as the value to store in the db?
> The protobuf is invalid anyway. It'll make the signature nicer and you don't
> have to forward declare google::protobuf::MessageLite.

Good idea. Done.

http://codereview.chromium.org/10701118/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_leveldb.cc (right):

http://codereview.chromium.org/10701118/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:143: if (!entry->get())
On 2012/07/10 17:21:19, achuith.bhandarkar wrote:
> nit: can we make this a ternary return? It saves 2 lines:
> return entry.get() ? DB_OK : DB_CORRUPTION;

Done.

[achuithb, 2012-07-10 18:42:55]

lgtm. Thanks!

http://codereview.chromium.org/10701118/diff/2002/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_db.h (right):

http://codereview.chromium.org/10701118/diff/2002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_db.h:51: // Used for testing (ex. injecting
incompatibe proto).
nit: spelling incompatible

[satorux1, 2012-07-10 20:11:24]

will submit shortly

http://codereview.chromium.org/10701118/diff/2002/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_db.h (right):

http://codereview.chromium.org/10701118/diff/2002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_db.h:51: // Used for testing (ex. injecting
incompatibe proto).
On 2012/07/10 18:42:55, achuith.bhandarkar wrote:
> nit: spelling incompatible

Done.