Migrate the IndexedDB backend from Blink to Chromium

Migrate the IndexedDB backend from Blink to Chromium

To get the IDB backend off the (deprecated) WebKit thread, remove
intermediate proxying, and let us take advantage of base utilities,
we're moving the code from Blink to Chromium.

This patch is basically a glorified copy/paste of the Blink IDB
backend code, with Chromium coding style applied, WTF dependencies
replaced with STL and base/, redundant classes removed, etc. It
introduces some new temporary proxy classes
(content/browser/webidb*_impl.*) to allow us build both the old and
new backends.

The new backend is currently disabled by default. It can be enabled
using a new (and temporary) command line switch: --new-indexeddb Once
we've done some further cleanup and are confident that the new backend
is stable, and the bots have moved from DumpRenderTree to
content_shell, we'll switch to the new backend by default. Once that
has survived through a dev channel release, we'll delete the Blink
code and eliminate unnecessary proxy classes.

BUG=234278
R=alecflett@chromium.org, dgrogan@chromium.org, piman@chromium.org

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

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

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

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

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

[jsbell, 2013-05-21 19:18:32]

alecflett@ - sanity check the uploaded patch?

jam@ - can you review as content owner?

Please read the CL description to understand the status of the patch. There's a
lot of additional cleanup planned.

Random additional notes:

* The content/browser/indexed_db/leveldb/avltree.h and fixed_array.h retain the
copyright headers from Blink, but may need updates/additions.

* If we decide to keep content/browser/indexed_db/list_set.h it should probably
be moved to base/ at some point.

[alecflett, 2013-05-21 19:59:24]

On 2013/05/21 19:18:32, jsbell wrote:
> alecflett@ - sanity check the uploaded patch?

lgtm of course !

[jsbell, 2013-05-21 20:53:40]

Need to wait for a blink roll before try jobs will work.

[jsbell, 2013-05-21 21:10:54]

Oops, jam is unavailable.

darin@ - can you take a look? (see previous comments for more context)

[jsbell, 2013-05-21 23:55:35]

And someone let darin away from the keyboard as well.

piman@ - can you take a look? See previous comments for context.

[jamesr, 2013-05-21 23:56:06]

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db.h (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db.h:13: TransactionReadOnly = 0,
chromium style for enum values is SHOUTING_STYLE -
http://www.chromium.org/developers/coding-style#Naming

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

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:34: const int64_t
KeyGeneratorInitialNumber =
constants in chromium should be named kFooBarBaz

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names

do you really want this to have external linkage? this should be declared static
or in an anonymous namespace if not

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:39: kFindKeyInIndex = 3,
enum values should be SHOUTY

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:97:
GetInt(DBOrTransaction* db,
seems overwrapped. did clang-format put this on a separate line from the return
type?

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:192: const int64_t
latest_known_schema_version = 2;
should be kLatest...., and probably static

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:403:
IndexedDBLevelDBBackingStoreOpenMemorySuccess,
should be SHOUTY

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:1901: 0;
odd indentation here. if this is supposed to be a continuation of the return
statement, i'd expect it to be 4 spaces in from the start of 'return'

since this is multiple physical lines, should have {}s

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_backing_store.h (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.h:87:
DISALLOW_COPY_AND_ASSIGN(RecordIdentifier);
in chromium, this is nearly always the last thing in the private section not up
top

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.h:102: std::vector<char>
primary_key_;  // TODO: Make it more clear that this is
TODOs in chromium should have a name in parens, e.g. TODO(alecflett):
Blahblahblah

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#TODO_Comments

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.h:182: Ready = 0,
enums should SHOUT

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_database.h (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database.h:64: NormalTask = 0,
SHOUTY

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_index_writer.h (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_index_writer.h:23: // TODO: this namespace
is temporary until we move its contents out to their
TODO(who):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_leveldb_coding.cc (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:162: static const
unsigned char IndexedDBKeyNullTypeByte = 0;
all need to be kFooBar

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_leveldb_coding.h (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_leveldb_coding.h:23: const unsigned char
MinimumIndexId = 30;
kBlahBlah

should probably just declare the variable here but provide the definition in
some .cc that provides storage, so things don't get confused if someone does
something like take the address of this

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_leveldb_coding.h:28: static const size_t
DefaultInlineBufferSize = 32;
ditto

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_leveldb_coding.h:119: GlobalMetaData,
SHOUT

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/leveldb/leveldb_iterator.h (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/leveldb/leveldb_iterator.h:15: ;
hmm?

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/leveldb/leveldb_unittest.cc (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/leveldb/leveldb_unittest.cc:20: using namespace
content;
using declarations are banned in chromium style.  a fairly common idiom for unit
tests i've seen is to do:

namespace content {
namespace {
// test stuff follows

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/webidbdatabase_impl.h (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/webidbdatabase_impl.h:53: WebKit::WebIDBCallbacks*)
OVERRIDE;
please don't use OVERRIDE when implementing interfaces defined in Blink headers
- it makes rolling too hard. the clang checker ignores things in the WebKit
namespace for OVERRIDE checks

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/webidbfactory_impl.cc (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/webidbfactory_impl.cc:32: 0,
this looks like a pointer - should be NULL in chromium land

https://codereview.chromium.org/15564008/diff/11001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db.h (right):

https://codereview.chromium.org/15564008/diff/11001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db.h:10: namespace IndexedDB {
this is an unusual sort of namespace name for chromium code.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespace_Names
suggest namespace names should reflect directory structure and we don't have any
directories called IndexedDB in the tree

[piman, 2013-05-22 00:45:46]

From an OWNER standpoint, this is fine.

Please have someone familiar with the webkit code review the actual changes
carefully - I'm not that person.

James gave you a bunch of style nits.

Once that is all done, I'm happy to do a final OWNER review.

[jsbell, 2013-05-22 17:54:44]

Thank you *very* much for the extensive feedback, jamesr.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db.h (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db.h:13: TransactionReadOnly = 0,
On 2013/05/21 23:56:06, jamesr wrote:
> chromium style for enum values is SHOUTING_STYLE -
> http://www.chromium.org/developers/coding-style#Naming

Done.

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

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:34: const int64_t
KeyGeneratorInitialNumber =
On 2013/05/21 23:56:06, jamesr wrote:
> constants in chromium should be named kFooBarBaz
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names
> 
> do you really want this to have external linkage? this should be declared
static
> or in an anonymous namespace if not

Done.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:39: kFindKeyInIndex = 3,
On 2013/05/21 23:56:06, jamesr wrote:
> enum values should be SHOUTY

Done.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:97:
GetInt(DBOrTransaction* db,
On 2013/05/21 23:56:06, jamesr wrote:
> seems overwrapped. did clang-format put this on a separate line from the
return
> type?

Yeah, we're just letting clang-format do it's thing until we're done with the
massive updates. There are a couple of places where clang-format appears to do
crazy things - we'll file bugs against it.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:192: const int64_t
latest_known_schema_version = 2;
On 2013/05/21 23:56:06, jamesr wrote:
> should be kLatest...., and probably static

Done.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:403:
IndexedDBLevelDBBackingStoreOpenMemorySuccess,
On 2013/05/21 23:56:06, jamesr wrote:
> should be SHOUTY

Done.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:1901: 0;
On 2013/05/21 23:56:06, jamesr wrote:
> odd indentation here. if this is supposed to be a continuation of the return
> statement, i'd expect it to be 4 spaces in from the start of 'return'
> 
> since this is multiple physical lines, should have {}s

clang-format. Unfortunately, if you move the 0; to the previous line - which is
< 80chars - clang-format will again break the line if you run it again.

Again, leaving it in clang-format style (and will file bugs) until we're done
with all the cleanup.

I'll put in the {} though, yikes.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_backing_store.h (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.h:87:
DISALLOW_COPY_AND_ASSIGN(RecordIdentifier);
On 2013/05/21 23:56:06, jamesr wrote:
> in chromium, this is nearly always the last thing in the private section not
up
> top

Done.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.h:102: std::vector<char>
primary_key_;  // TODO: Make it more clear that this is
On 2013/05/21 23:56:06, jamesr wrote:
> TODOs in chromium should have a name in parens, e.g. TODO(alecflett):
> Blahblahblah
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#TODO_Comments

Done.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.h:182: Ready = 0,
On 2013/05/21 23:56:06, jamesr wrote:
> enums should SHOUT

Done.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_database.h (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database.h:64: NormalTask = 0,
On 2013/05/21 23:56:06, jamesr wrote:
> SHOUTY

Done.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_index_writer.h (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_index_writer.h:23: // TODO: this namespace
is temporary until we move its contents out to their
On 2013/05/21 23:56:06, jamesr wrote:
> TODO(who): 

Done.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_leveldb_coding.cc (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:162: static const
unsigned char IndexedDBKeyNullTypeByte = 0;
On 2013/05/21 23:56:06, jamesr wrote:
> all need to be kFooBar

Done.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_leveldb_coding.h (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_leveldb_coding.h:23: const unsigned char
MinimumIndexId = 30;
On 2013/05/21 23:56:06, jamesr wrote:
> kBlahBlah
> 
> should probably just declare the variable here but provide the definition in
> some .cc that provides storage, so things don't get confused if someone does
> something like take the address of this

Done.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_leveldb_coding.h:28: static const size_t
DefaultInlineBufferSize = 32;
On 2013/05/21 23:56:06, jamesr wrote:
> ditto

Done.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_leveldb_coding.h:119: GlobalMetaData,
On 2013/05/21 23:56:06, jamesr wrote:
> SHOUT

Done.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/leveldb/leveldb_iterator.h (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/leveldb/leveldb_iterator.h:15: ;
On 2013/05/21 23:56:06, jamesr wrote:
> hmm?

Done.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/leveldb/leveldb_unittest.cc (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/leveldb/leveldb_unittest.cc:20: using namespace
content;
On 2013/05/21 23:56:06, jamesr wrote:
> using declarations are banned in chromium style.  a fairly common idiom for
unit
> tests i've seen is to do:
> 
> namespace content {
> namespace {
> // test stuff follows

Done.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/webidbdatabase_impl.h (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/webidbdatabase_impl.h:53: WebKit::WebIDBCallbacks*)
OVERRIDE;
On 2013/05/21 23:56:06, jamesr wrote:
> please don't use OVERRIDE when implementing interfaces defined in Blink
headers
> - it makes rolling too hard. the clang checker ignores things in the WebKit
> namespace for OVERRIDE checks

Done.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
File content/browser/indexed_db/webidbfactory_impl.cc (right):

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/webidbfactory_impl.cc:32: 0,
On 2013/05/21 23:56:06, jamesr wrote:
> this looks like a pointer - should be NULL in chromium land

Done. Actually, killed that argument entirely which is unneeded in the new back
end.

https://codereview.chromium.org/15564008/diff/11001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db.h (right):

https://codereview.chromium.org/15564008/diff/11001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db.h:10: namespace IndexedDB {
On 2013/05/21 23:56:06, jamesr wrote:
> this is an unusual sort of namespace name for chromium code.
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespace_Names
> suggest namespace names should reflect directory structure and we don't have
any
> directories called IndexedDB in the tree

Renamed to indexed_db; we considered putting everything into an indexed_db
namespace but that will touch existing files (e.g. common/indexed_db,
common_child/indexed_db) so we wanted to wait for later.

[dgrogan, 2013-05-22 18:22:06]

lgtm

I spot checked some files to look for potential weirdness.

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_database_impl.cc (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_database_impl.cc:1567: // are
There are a few of these bad comment wraps.

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_database_impl.cc:1662: // frontend
comment wrapping

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_database_impl.cc:1716: // the
comment wrapping

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/leveldb/avltree.h (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/leveldb/avltree.h:2: * Copyright (C) 2008 Apple Inc.
All rights reserved.
This turned out to be ok?

[jamesr, 2013-05-22 18:59:44]

style mostly lg.

some of the files appear to have apple copyright headers. what's the story
there?

https://codereview.chromium.org/15564008/diff/22001/content/browser/in_proces...
File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/in_proces...
content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:320:
IndexedDBDispatcherHost::DestroyObject(IDMap<ObjectType, IDMapOwnPointer>* map,
ditto

https://codereview.chromium.org/15564008/diff/22001/content/browser/in_proces...
File content/browser/in_process_webkit/indexed_db_dispatcher_host.h (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/in_proces...
content/browser/in_process_webkit/indexed_db_dispatcher_host.h:95:
GetOrTerminateProcess(IDMap<ReturnType, IDMapOwnPointer>* map,
chromium style says: "For function declarations and definitions, put each
argument on a separate line unless the whole declaration fits on one line."

http://www.chromium.org/developers/coding-style#Code_Formatting

doing that would make the wrapping less crazy here too

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_database_error.h (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_database_error.h:28: unsigned short code()
const { return code_; }
this should probably be an explicitly sized type if you care about the size
(which I suspect you do). base/basictypes.h has uint16

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_factory.h (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_factory.h:17: typedef int ExceptionCode;
This seems like at least the third time I've seen this typedef in this patch. Is
it even used?

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_leveldb_coding.cc (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:156: #define INT64_MAX
0x7fffffffffffffffLL
any particular reason against using std::numeric_limits?

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:304: DCHECK(limit >=
start);
DCHECK_GE

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:622: DCHECK(&ptr_a !=
&ptr_b);
DCHECK_NE

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:623: DCHECK(ptr_a <
limit_a);
DCHECK_LT

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:624: DCHECK(ptr_b <
limit_b);
DCHECK_LT

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:739: DCHECK(p != limit);
DCHECK_NE

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:949: DCHECK(ptr_b !=
end_b);
_NE

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:1128: DCHECK(index_id_
!= kInvalidId);
_NE

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:1143:
DCHECK(index_id_string.size() <= kMaxIndexIdSizeBytes);
_LE

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:1162: DCHECK(ret.size()
<= kDefaultInlineBufferSize);
_LE

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:1169: DCHECK(index_id_
!= kInvalidId);
_NE

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:1183: DCHECK(index_id_
!= kInvalidId);
_NE

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:1579: // Can we remove
this?
comments wrapped oddly

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_metadata.cc (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_metadata.cc:39: }
  // namespace content

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/leveldb/avltree.h (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/leveldb/avltree.h:2: * Copyright (C) 2008 Apple Inc.
All rights reserved.
this doesn't look like the right copyright header...

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/leveldb/fixed_array.h (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/leveldb/fixed_array.h:2: * Copyright (C) 2010 Apple
Inc. All rights reserved.
huh?

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/leveldb/leveldb_database.cc (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/leveldb/leveldb_database.cc:114: long long
free_disk_space_in_k_bytes =
don't use a type like long long. if you want this to represent a number of
bytes, use size_t. if you want to match the base::SysInfo::AmountOfFreeDiskSpace
type, use int64 (like it does)

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/webidbcursor_impl.h (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/webidbcursor_impl.h:19: virtual void advance(unsigned
long, WebKit::WebIDBCallbacks*);
chromium style requires names for all parameters

[dgrogan, 2013-05-22 19:08:34]

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/leveldb/leveldb_database.cc (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/leveldb/leveldb_database.cc:115:
base::SysInfo::AmountOfFreeDiskSpace(file_name) / 1024;
This was long long because that's what the webkit api used. It should be changed
to int64.

[jsbell, 2013-05-22 19:13:03]

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_database_impl.cc (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_database_impl.cc:1662: // frontend
On 2013/05/22 18:22:06, dgrogan wrote:
> comment wrapping

Done.

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_database_impl.cc:1716: // the
On 2013/05/22 18:22:06, dgrogan wrote:
> comment wrapping

Done.

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/leveldb/avltree.h (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/leveldb/avltree.h:2: * Copyright (C) 2008 Apple Inc.
All rights reserved.
On 2013/05/22 18:22:06, dgrogan wrote:
> This turned out to be ok?

I asked the lawyerly types what to do here in advance; leaving the original
license header here this seems to match their advice, but seems wrong so I
pinged them again yesterday, waiting to hear back.

[michaeln, 2013-05-22 20:34:23]

drive-by-comment... sweet :)

[jsbell, 2013-05-22 22:21:13]

As noted, waiting on lawyerly types for the license headers. Will ping again.

There are a handful of DCHECK()s that introduce symbol resolution errors when
converted to DCHECK_EQ()/DCHECK_NE() when running the unit tests. Want to get to
the bottom of those.

https://codereview.chromium.org/15564008/diff/22001/content/browser/in_proces...
File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/in_proces...
content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:320:
IndexedDBDispatcherHost::DestroyObject(IDMap<ObjectType, IDMapOwnPointer>* map,
On 2013/05/22 18:59:44, jamesr wrote:
> ditto

Done.

https://codereview.chromium.org/15564008/diff/22001/content/browser/in_proces...
File content/browser/in_process_webkit/indexed_db_dispatcher_host.h (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/in_proces...
content/browser/in_process_webkit/indexed_db_dispatcher_host.h:95:
GetOrTerminateProcess(IDMap<ReturnType, IDMapOwnPointer>* map,
On 2013/05/22 18:59:44, jamesr wrote:
> chromium style says: "For function declarations and definitions, put each
> argument on a separate line unless the whole declaration fits on one line."
> 
> http://www.chromium.org/developers/coding-style#Code_Formatting
> 
> doing that would make the wrapping less crazy here too

clang-format bug :(

Fixed manually for now.

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_database_error.h (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_database_error.h:28: unsigned short code()
const { return code_; }
On 2013/05/22 18:59:44, jamesr wrote:
> this should probably be an explicitly sized type if you care about the size
> (which I suspect you do). base/basictypes.h has uint16

Done.

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_database_impl.cc (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_database_impl.cc:1567: // are
On 2013/05/22 18:22:06, dgrogan wrote:
> There are a few of these bad comment wraps.

Done.

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_factory.h (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_factory.h:17: typedef int ExceptionCode;
On 2013/05/22 18:59:44, jamesr wrote:
> This seems like at least the third time I've seen this typedef in this patch.
Is
> it even used?

Removed. Also removed some class forward declarations that weren't used.

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_leveldb_coding.cc (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:156: #define INT64_MAX
0x7fffffffffffffffLL
On 2013/05/22 18:59:44, jamesr wrote:
> any particular reason against using std::numeric_limits?

None at all, in Chromium code. Done!

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:1579: // Can we remove
this?
On 2013/05/22 18:59:44, jamesr wrote:
> comments wrapped oddly

Done.

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_metadata.cc (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_metadata.cc:39: }
On 2013/05/22 18:59:44, jamesr wrote:
>   // namespace content

Done.

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/leveldb/leveldb_database.cc (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/leveldb/leveldb_database.cc:114: long long
free_disk_space_in_k_bytes =
On 2013/05/22 18:59:44, jamesr wrote:
> don't use a type like long long. if you want this to represent a number of
> bytes, use size_t. if you want to match the
base::SysInfo::AmountOfFreeDiskSpace
> type, use int64 (like it does)

Done.

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
File content/browser/indexed_db/webidbcursor_impl.h (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/indexed_d...
content/browser/indexed_db/webidbcursor_impl.h:19: virtual void advance(unsigned
long, WebKit::WebIDBCallbacks*);
On 2013/05/22 18:59:44, jamesr wrote:
> chromium style requires names for all parameters

Done. (There may still be some lingering mismatches between declarations and
definitions.)

[jsbell, 2013-05-23 21:10:49]

FYI, clang-format appears to have been updated which addresses 2/3 formatting
nits.

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

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:97:
GetInt(DBOrTransaction* db,
On 2013/05/22 17:54:44, jsbell wrote:
> On 2013/05/21 23:56:06, jamesr wrote:
> > seems overwrapped. did clang-format put this on a separate line from the
> return
> > type?
> 
> Yeah, we're just letting clang-format do it's thing until we're done with the
> massive updates. There are a couple of places where clang-format appears to do
> crazy things - we'll file bugs against it.
> 

A clang-format update appears to have addressed this, it now wraps correctly.

https://codereview.chromium.org/15564008/diff/9001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:1901: 0;
On 2013/05/22 17:54:44, jsbell wrote:
> On 2013/05/21 23:56:06, jamesr wrote:
> > odd indentation here. if this is supposed to be a continuation of the return
> > statement, i'd expect it to be 4 spaces in from the start of 'return'
> > 
> > since this is multiple physical lines, should have {}s
> 
> clang-format. Unfortunately, if you move the 0; to the previous line - which
is
> < 80chars - clang-format will again break the line if you run it again.
> 
> Again, leaving it in clang-format style (and will file bugs) until we're done
> with all the cleanup.
> 
> I'll put in the {} though, yikes.

clang-format is still wrapping before the 0 which is legal but ugly. Filed a
(google internal) bug against it.

https://codereview.chromium.org/15564008/diff/22001/content/browser/in_proces...
File content/browser/in_process_webkit/indexed_db_dispatcher_host.h (right):

https://codereview.chromium.org/15564008/diff/22001/content/browser/in_proces...
content/browser/in_process_webkit/indexed_db_dispatcher_host.h:95:
GetOrTerminateProcess(IDMap<ReturnType, IDMapOwnPointer>* map,
On 2013/05/22 22:21:14, jsbell wrote:
> On 2013/05/22 18:59:44, jamesr wrote:
> > chromium style says: "For function declarations and definitions, put each
> > argument on a separate line unless the whole declaration fits on one line."
> > 
> > http://www.chromium.org/developers/coding-style#Code_Formatting
> > 
> > doing that would make the wrapping less crazy here too
> 
> clang-format bug :(
> 
> Fixed manually for now.

clang-format appears to have been updated and now wraps this more sensibly on
its own.

[jsbell, 2013-05-23 21:12:15]

On 2013/05/22 22:21:13, jsbell wrote:
> As noted, waiting on lawyerly types for the license headers. Will ping again.

And FYI, the resolution was to pre-pend the Chromium license to those files,
which as darin@ points out is done in a few other places in the code base.

At this point I'm just waiting for the win bots to show green before asking for
the final LGTM.

[jsbell, 2013-05-24 00:11:07]

Okay, I think we're down to:
* android webview license check - I sent mail to mnaganov@ to see if we can just
add the from-WTF files to android_webview/tools/third_party_files_whitelist.txt

* some issue with the iOS builds where it's trying to compile in
third_party/leveldatabase suddenly. I need to figure out why and/or who to ask -
I didn't think content would be pulled in at all.
http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...

[jamesr, 2013-05-24 00:15:55]

ios does all sorts of stupid crap you would not expect.  See the various
'conditions': [ [ 'OS != "ios"' ] blurbs stuck all over the gypis - you probably
need some of that too.

[mnaganov (inactive), 2013-05-24 08:24:00]

On 2013/05/24 00:11:07, jsbell wrote:
> Okay, I think we're down to:
> * android webview license check - I sent mail to mnaganov@ to see if we can
just
> add the from-WTF files to
android_webview/tools/third_party_files_whitelist.txt
> 

As dannyb@ has said that the current license statement on these files is
acceptable, then please add them into
android_webview/tools/third_party_files_whitelist.txt. You can TBR me, if you'll
be committing the CL at night time by GMT.

> * some issue with the iOS builds where it's trying to compile in
> third_party/leveldatabase suddenly. I need to figure out why and/or who to ask
-
> I didn't think content would be pulled in at all.
>
http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...

[jsbell, 2013-05-24 16:54:35]

On 2013/05/24 08:24:00, Mikhail Naganov (Chromium) wrote:
> 
> As dannyb@ has said that the current license statement on these files is
> acceptable, then please add them into
> android_webview/tools/third_party_files_whitelist.txt. You can TBR me, if
you'll
> be committing the CL at night time by GMT.

Thanks - added, can you please take a look?

I still need to sort out the ios issues, but time for another run past the
bots...

[mnaganov (inactive), 2013-05-24 17:03:45]

On 2013/05/24 16:54:35, jsbell wrote:
> On 2013/05/24 08:24:00, Mikhail Naganov (Chromium) wrote:
> > 
> > As dannyb@ has said that the current license statement on these files is
> > acceptable, then please add them into
> > android_webview/tools/third_party_files_whitelist.txt. You can TBR me, if
> you'll
> > be committing the CL at night time by GMT.
> 
> Thanks - added, can you please take a look?
> 
> I still need to sort out the ios issues, but time for another run past the
> bots...

LGTM for android_webview

[jsbell, 2013-05-24 20:26:40]

piman@, jamesr@ - any other comments or is this good to go once the bots are
green?

[jamesr, 2013-05-24 20:29:20]

looks good to me!

[piman, 2013-05-24 20:43:31]

LGTM if others are happy

[jsbell, 2013-05-24 23:04:38]

Committed patchset #15 manually as r202215 (presubmit successful).

[jsbell, 2013-05-28 16:33:18]

TL;DR: reverted, new patch sent to the bots

...

Reverted over in https://codereview.chromium.org/15870003

One of the sticky bits of the conversion was moving from WTF::Vector<char> to
std::vector<char>, given the decode routines which are a maze of twisty passages
and pointer arithmetic, to avoid copies. (It is a CPU hot spot when profiling.).
The bulk of the decode routines operate on (const char* start, const char*
limit). We'd like to make these take iterators instead - I have a side patch in
progress for that, but it's a slog. 

As a temporary measure we used (vec.data(), vec.data() + vec.size()) -
unfortunately, that's a C++11ism that isn't supported on Android. So we switched
to (&*vec.begin(), &*vec.end()) which is effectively the same thing although
strictly not supported on empty vectors (we guard against those anyway). The
revert is because a dereference of vec.end() is a debug assert on MSVC - didn't
show up on the try bot runs last week. :(

In the latest patch I switched that out &*vec.end() for the equivalent
&*vec.rbegin() + 1. 

(The goal is to get this landed so we can enable more tests and land the
followup encode/decode cleanup patches, before actually enabling the new
back-end. More pointer math fills me with dread, but it's progress...)

[jsbell, 2013-05-28 18:23:04]

Committed patchset #16 manually as r202604 (presubmit successful).

[jsbell, 2013-05-28 23:09:32]

On 2013/05/28 18:23:04, jsbell wrote:
> Committed patchset #16 manually as r202604 (presubmit successful).

Reverted again - missed a handful of iterator dereference cases. Built locally
on windows, fixed 'em for real.

Mac Memory FYI reported issues as well. I can't repro those on linux (*sad*) so
I'm not sure if they're due to the same root cause or not. Sent off a
mac_valgrind try job to see.

[jsbell, 2013-05-30 16:53:20]

Committed patchset #21 manually as r203164 (presubmit successful).

[alecflett, 2013-05-30 23:03:21]

Committed patchset #21 manually as r203257 (presubmit successful).

[alecflett, 2013-05-30 23:10:04]

On 2013/05/30 23:03:21, alecflett wrote:
> Committed patchset #21 manually as r203257 (presubmit successful).

Reverted.. again. This was supposed to be an attempt to land the patches without
building the tests, which seems to be where the mac memory stuff is failing.

Will try again tomorrow.

[alecflett, 2013-05-31 18:33:18]

Committed patchset #24 manually as r203443 (presubmit successful).