IndexedDB: Convert decoding functions to pass StringPieces vs. pointers

IndexedDB: Convert decoding functions to pass StringPieces vs. pointers

The ugly decoding functions that do pointer arithmetic were nasty in
the Blink codebase. Conversion to Chromium and replacing WTF::Vector
with std::vector made things worse due to iterator differences across
platforms in C++03. Start replacing all that goo with StringPiece
which encapsulates the pointers and is a better match for the "slice"
model of leveldb.

Also avoids a std::string to std::vector<char> data copy when getting
values - hooray! Standardizes the various DecodeXXX function signatures
to take a mutable slice and always return a success code.

Key path encode/decode tests were updated to have the "expected" encoded
data in the test rather than manually computing it.

Left to do: the various Key-type classes still pass around raw pointers,
and LevelDBSlice should be removed entirely in favor of StringPiece.

BUG=234278
R=alecflett@chromium.org

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

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

[jsbell, 2013-06-03 16:38:09]

alecflett@ - please take a look?

[alecflett, 2013-06-03 21:22:40]

On 2013/06/03 16:38:09, jsbell wrote:
> alecflett@ - please take a look?

lgtm as is - but minor things that we can maybe deal with later

[alecflett, 2013-06-03 21:22:53]

https://codereview.chromium.org/16256014/diff/8016/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/16256014/diff/8016/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:1817: return
DecodeIDBKey(&slice, found_primary_key) && slice.empty();
This pattern makes me think we might also want something that takes a 'const
StringPiece&' so we don't have to make so many explicit temporaries that we'll
never use.

https://codereview.chromium.org/16256014/diff/8016/content/browser/indexed_db...
File content/browser/indexed_db/leveldb/leveldb_slice.h (right):

https://codereview.chromium.org/16256014/diff/8016/content/browser/indexed_db...
content/browser/indexed_db/leveldb/leveldb_slice.h:37: operator
base::StringPiece() const {
I wish there was an easy way to make this explicit.. but I guess not? is it
worth adding a AsStringPiece() method or something instead? (I don't know how
often this is invoked)

[jsbell, 2013-06-03 21:37:09]

https://codereview.chromium.org/16256014/diff/8016/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/16256014/diff/8016/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:1817: return
DecodeIDBKey(&slice, found_primary_key) && slice.empty();
On 2013/06/03 21:22:53, alecflett wrote:
> This pattern makes me think we might also want something that takes a 'const
> StringPiece&' so we don't have to make so many explicit temporaries that we'll
> never use.

Here there is the && slice.empty() at the end that is confirming that the decode
consumed the entire slice, so it's explicitly non-const.

The initialization using &* is gross. If we can convert this from
std::vector<char> to std::string then that initialization is cleaner.

(Fundamentally, the StringPiece usage here is like a stream. We may actually
want to make it a proper stream at some point, but getting consistent is an
incremental step.)

https://codereview.chromium.org/16256014/diff/8016/content/browser/indexed_db...
File content/browser/indexed_db/leveldb/leveldb_slice.h (right):

https://codereview.chromium.org/16256014/diff/8016/content/browser/indexed_db...
content/browser/indexed_db/leveldb/leveldb_slice.h:37: operator
base::StringPiece() const {
On 2013/06/03 21:22:53, alecflett wrote:
> I wish there was an easy way to make this explicit.. but I guess not? is it
> worth adding a AsStringPiece() method or something instead? (I don't know how
> often this is invoked)

This is just used when initializing a StringPiece from a LevelDBSlice, e.g.

StringPiece slice(leveldb_slice);

I'll make it a method for now so it's obvious.

The usage will go away when LevelDBSlice does.

[commit-bot: I haz the power, 2013-06-03 21:56:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/16256014/7014

[commit-bot: I haz the power, 2013-06-04 01:33:39]

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

[commit-bot: I haz the power, 2013-06-04 12:33:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/16256014/7014

[jsbell, 2013-06-04 16:32:44]

On 2013/06/04 12:33:48, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/jsbell%40chromium.org/16256014/7014

Huh... the android_dbg_triggered_tests shows:

[ RUN      ] IndexedDBBackingStoreTest.PutGetConsistency
[ CRASHED      ]

[ RUN      ] IndexedDBBackingStoreTest.HighIds
[ CRASHED      ]

Gonna clear the commit flag and dig in to that. Plausible a subtle bug was
introduced that only android is turning up.

[jsbell, 2013-06-04 18:36:55]

On 2013/06/04 16:32:44, jsbell wrote:
> On 2013/06/04 12:33:48, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> > https://chromium-status.appspot.com/cq/jsbell%2540chromium.org/16256014/7014
> 
> Huh... the android_dbg_triggered_tests shows:
> 
> [ RUN      ] IndexedDBBackingStoreTest.PutGetConsistency
> [ CRASHED      ]
> 
> [ RUN      ] IndexedDBBackingStoreTest.HighIds
> [ CRASHED      ]
> 
> Gonna clear the commit flag and dig in to that. Plausible a subtle bug was
> introduced that only android is turning up.

It's consistent between runs on android. No luck with asan or valgrind on linux.
:(

This definitely goes through the code that changed, so it's probably legitimate,
but I'm not seeing the bug.

[commit-bot: I haz the power, 2013-06-05 18:48:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/16256014/28001

[commit-bot: I haz the power, 2013-06-05 22:48:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/16256014/28001

[commit-bot: I haz the power, 2013-06-06 02:34:15]

Change committed as 204391

[commit-bot: I haz the power, 2013-06-06 05:03:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/16256014/43001

[commit-bot: I haz the power, 2013-06-06 16:02:25]

Change committed as 204512