IndexedDB: Switch key/value handling from vector<char> to std::string

IndexedDB: Switch key/value handling from vector<char> to std::string

LevelDB uses std::string in its APIs. To eliminate a conversion and
simplify the code, switch the IDB backend from vector<char> to string
as well.

Also eliminate the LevelDBSlice type as well, using StringPiece
and leveldb::Slice as appropriate.

R=alecflett@chromium.org,tsepez@chromium.org
TBR=jam@chromium.org

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

[jsbell, 2013-07-09 00:41:43]

alecflett@ - please take a look?

[alecflett, 2013-07-09 18:26:30]

on the whole this is pretty awesome.

A few minor nits, and some ideas for the future.

in particular I'd like to see the verbose foo.insert(foo.end(), ...) stuff
removed in this patch.

Looking back over my commetns, I may also be mixing up CompareIndexKeys() and
Compare() - I can't remember if they both decode on every call or just
Compare()?

lgtm with the insert()'s replaced.

https://chromiumcodereview.appspot.com/18075008/diff/1/content/browser/indexe...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://chromiumcodereview.appspot.com/18075008/diff/1/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:1675: while
(CompareIndexKeys(it->Key(), target) > 0) {
Not a code review comment but:

This is interesting - I hadn't noticed this before but since 'target' is an
invariant here, I wonder if CompareIndexKeys could 'target' pre-decoded to avoid
decoding it each time through this loop.

https://chromiumcodereview.appspot.com/18075008/diff/1/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:1682:
found_key->assign(it->Key().begin(), it->Key().end());
Can this now be *found_key = id->Key() ?

https://chromiumcodereview.appspot.com/18075008/diff/1/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:1686: } while
(it->IsValid() && !CompareIndexKeys(it->Key(), target));
ditto here about the pre-decoding on 'target'

https://chromiumcodereview.appspot.com/18075008/diff/1/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:1741: if
(CompareIndexKeys(it->Key(), leveldb_key) > 0)
oh wow, lots of opportunity for pre-decoding?

https://chromiumcodereview.appspot.com/18075008/diff/1/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:1988: int compare =
CompareIndexKeys(iterator_->Key(), cursor_options_.low_key);
I guess we should just look at every call in our own code to CompareIndexKeys!

https://chromiumcodereview.appspot.com/18075008/diff/1/content/browser/indexe...
File content/browser/indexed_db/indexed_db_leveldb_coding.cc (right):

https://chromiumcodereview.appspot.com/18075008/diff/1/content/browser/indexe...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:1147:
ret.insert(ret.end(), database_id_string.begin(), database_id_string.end());
these inserts can become appends now, no?

https://chromiumcodereview.appspot.com/18075008/diff/1/content/browser/indexe...
File content/browser/indexed_db/indexed_db_leveldb_coding_unittest.cc (left):

https://chromiumcodereview.appspot.com/18075008/diff/1/content/browser/indexe...
content/browser/indexed_db/indexed_db_leveldb_coding_unittest.cc:8: #include
<string>
I think you still want <string> here, no?

https://chromiumcodereview.appspot.com/18075008/diff/1/content/browser/indexe...
File content/browser/indexed_db/leveldb/leveldb_comparator.h (right):

https://chromiumcodereview.appspot.com/18075008/diff/1/content/browser/indexe...
content/browser/indexed_db/leveldb/leveldb_comparator.h:12: class
LevelDBComparator {
Seems like we could get rid of this adapter class entirely with a little more
work (for another patch...)

https://chromiumcodereview.appspot.com/18075008/diff/1/content/child/indexed_...
File content/child/indexed_db/indexed_db_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/18075008/diff/1/content/child/indexed_...
content/child/indexed_db/indexed_db_dispatcher.cc:500: if (value.size())
I think you can skip the if (value.size()) now? Or does that
deref-empty-iterators issue affect strings too?

[jsbell, 2013-07-09 19:41:46]

re: Compare/CompareIndexKeys - they all end up being the same thing - a Compare
call w/ a flag on whether or not to compare the primary key or not. 

re: str.insert(str.end(), s.begin(), s.end()) 

Replaced with str.append(s) or str = s.as_string() as appropriate. Also cleaned
up:

StringPiece(str.begin(), str.end()) => StringPiece(str)
StringPiece(str.begin(), str.size()) => StringPiece(str)
s.end() - s.begin() => s.size()
explicit std::string->StringPiece conversions removed

https://codereview.chromium.org/18075008/diff/1/content/browser/indexed_db/in...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/18075008/diff/1/content/browser/indexed_db/in...
content/browser/indexed_db/indexed_db_backing_store.cc:1675: while
(CompareIndexKeys(it->Key(), target) > 0) {
On 2013/07/09 18:26:30, alecflett wrote:
> Not a code review comment but:
> 
> This is interesting - I hadn't noticed this before but since 'target' is an
> invariant here, I wonder if CompareIndexKeys could 'target' pre-decoded to
avoid
> decoding it each time through this loop.

I filed crbug.com/258538

Note that comparisons don't require a full decode of both keys - there are
dedicated CompareEncodedKeys functions - so this may actually be more efficient
than hoisting a target decode out of the loop then doing a *full* decode of each
key during the iteration.

https://codereview.chromium.org/18075008/diff/1/content/browser/indexed_db/in...
content/browser/indexed_db/indexed_db_backing_store.cc:1682:
found_key->assign(it->Key().begin(), it->Key().end());
On 2013/07/09 18:26:30, alecflett wrote:
> Can this now be *found_key = id->Key() ?

Sadly, no. But it can be *found_key = id->Key().as_string();

https://codereview.chromium.org/18075008/diff/1/content/browser/indexed_db/in...
File content/browser/indexed_db/indexed_db_leveldb_coding.cc (right):

https://codereview.chromium.org/18075008/diff/1/content/browser/indexed_db/in...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:1147:
ret.insert(ret.end(), database_id_string.begin(), database_id_string.end());
On 2013/07/09 18:26:30, alecflett wrote:
> these inserts can become appends now, no?

Done.

https://codereview.chromium.org/18075008/diff/1/content/browser/indexed_db/in...
File content/browser/indexed_db/indexed_db_leveldb_coding_unittest.cc (left):

https://codereview.chromium.org/18075008/diff/1/content/browser/indexed_db/in...
content/browser/indexed_db/indexed_db_leveldb_coding_unittest.cc:8: #include
<string>
On 2013/07/09 18:26:30, alecflett wrote:
> I think you still want <string> here, no?

Per pkasting on IRC: "Per the style guide, foo_unittest.cc should #include foo.h
first, just like foo.cc does.  (Then it doesn't need to repeat anything else.)"

(So technically, more of the includes below could be removed too.)

https://codereview.chromium.org/18075008/diff/1/content/browser/indexed_db/le...
File content/browser/indexed_db/leveldb/leveldb_comparator.h (right):

https://codereview.chromium.org/18075008/diff/1/content/browser/indexed_db/le...
content/browser/indexed_db/leveldb/leveldb_comparator.h:12: class
LevelDBComparator {
On 2013/07/09 18:26:30, alecflett wrote:
> Seems like we could get rid of this adapter class entirely with a little more
> work (for another patch...)

Agreed.

https://codereview.chromium.org/18075008/diff/1/content/child/indexed_db/inde...
File content/child/indexed_db/indexed_db_dispatcher.cc (right):

https://codereview.chromium.org/18075008/diff/1/content/child/indexed_db/inde...
content/child/indexed_db/indexed_db_dispatcher.cc:500: if (value.size())
On 2013/07/09 18:26:30, alecflett wrote:
> I think you can skip the if (value.size()) now? Or does that
> deref-empty-iterators issue affect strings too?

Affects strings too.

[commit-bot: I haz the power, 2013-07-09 19:43:49]

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

[commit-bot: I haz the power, 2013-07-09 19:54:56]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[jsbell, 2013-07-09 19:58:01]

tsepez@ - can you review the IPC message changes? std::vector<char> =>
std::string in a few places.

jam@ - OWNERS review of gypi change?
(just a file removal, I'll TBR if you're busy)

[Tom Sepez, 2013-07-09 20:15:08]

lgtm

[commit-bot: I haz the power, 2013-07-09 20:16:13]

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

[commit-bot: I haz the power, 2013-07-09 20:37:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
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.

[commit-bot: I haz the power, 2013-07-09 20:58:03]

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

[commit-bot: I haz the power, 2013-07-09 22:05:01]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
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.

[jsbell, 2013-07-09 22:24:10]

On 2013/07/09 22:05:01, I haz the power (commit-bot) wrote:
> Sorry for I got bad news for ya.
> Compile failed with a clobber build on win_x64_rel.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
> 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.

Windows build break that was reverted - yay for LKGR?

[commit-bot: I haz the power, 2013-07-09 23:16:11]

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

[jam, 2013-07-10 01:14:55]

gypi lgtm

[commit-bot: I haz the power, 2013-07-10 01:31:58]

Change committed as 210698