Extract protobuf database into a new 'leveldb_proto' component

Extract protobuf database into a new 'leveldb_proto' component

Code extracted from components/dom_distiller/core/dom_distiller_database.*

Slight API change: callers to UpdateEntries now have to pass a vector of (string, proto) as key and value, instead of just a vector of protos where key is derived.

Ran clang-format on the files I touched so you may see some diffs.

Note: Implementations are in proto_database_impl.h and fake_db.h for proper linking.

BUG=385747
TBR=jochen,dgrogan
TEST=DomDistiller*,ProtoDatabaseImplTest

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

[Mathieu, 2014-06-13 15:39:02]

Hi, 

This is ready for review.

cjhopman (Review):
- chrome/browser/dom_distiller*
- components/dom_distiller
- components/leveldb_proto

darin (owner):
- Approval chrome/browser/DEPS

blundell (owner):
- Approval components/leveldb_proto, components/*

[cjhopman, 2014-06-13 19:41:05]

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
File components/leveldb_proto/core/proto_database.h (right):

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
components/leveldb_proto/core/proto_database.h:32: // Asynchronously initializes
the object. |callback| will be invoked on the UI
s/UI/calling

and below, too

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
File components/leveldb_proto/core/proto_database_impl.h (right):

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
components/leveldb_proto/core/proto_database_impl.h:30: class
SequencedTaskRunner;
Don't need these forward declarations anymore.

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
components/leveldb_proto/core/proto_database_impl.h:35: class DB;
Don't need this one either.

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
components/leveldb_proto/core/proto_database_impl.h:41: // operations will be
completed and the corresponding callbacks will be called.
Maybe point out that construction/calls/destruction should all happen on the
same thread. And note, either here or around the constructor, that all blocking
calls/disk access will happen on the provided task_runner.

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
components/leveldb_proto/core/proto_database_impl.h:46: class Database {
I think that the SerializeAsString() and ParseFromArray() calls should be moved
up to ProtoDatabaseImpl so that the underlying blocking database wouldn't depend
on the template parameter.

You could even make a layer above ProtoDatabaseImpl that does those conversions,
but I don't think the benefit of doing that is as high and the cost (in code
complexity) is definitely higher.

...

As I think about this more, maybe all of this would be even more useful as just
a  string-based interface to leveldb (with this simple, blocking-thread-aware
api) and then also a proto-based wrapper of that that would do the conversions.

Anyway, don't let those thoughts block this change, we can wait until we
actually have a user/usecase for the non-proto version.

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
components/leveldb_proto/core/proto_database_impl.h:79: virtual void Init(const
base::FilePath& database_dir,
I don't think I like that this Init() is exposed to users and not just part of
the constructor (the underlying db needs to have the init() to do setup on the
blocking thread).

No need to fix it now, but it would be nice to do it before it becomes hard to
fix.

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
components/leveldb_proto/core/proto_database_impl.h:104: using
base::MessageLoop;
Remove these usings

[Mathieu, 2014-06-13 22:07:07]

Thanks for your reply!

Also added a couple of typedefs to make life more readable.

Have a look!

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
File components/leveldb_proto/core/proto_database.h (right):

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
components/leveldb_proto/core/proto_database.h:32: // Asynchronously initializes
the object. |callback| will be invoked on the UI
On 2014/06/13 19:41:05, cjhopman wrote:
> s/UI/calling
> 
> and below, too

Done.

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
File components/leveldb_proto/core/proto_database_impl.h (right):

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
components/leveldb_proto/core/proto_database_impl.h:30: class
SequencedTaskRunner;
On 2014/06/13 19:41:05, cjhopman wrote:
> Don't need these forward declarations anymore.

Done.

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
components/leveldb_proto/core/proto_database_impl.h:35: class DB;
On 2014/06/13 19:41:05, cjhopman wrote:
> Don't need this one either.

Done.

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
components/leveldb_proto/core/proto_database_impl.h:41: // operations will be
completed and the corresponding callbacks will be called.
On 2014/06/13 19:41:05, cjhopman wrote:
> Maybe point out that construction/calls/destruction should all happen on the
> same thread. And note, either here or around the constructor, that all
blocking
> calls/disk access will happen on the provided task_runner.

Done.

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
components/leveldb_proto/core/proto_database_impl.h:46: class Database {
On 2014/06/13 19:41:05, cjhopman wrote:
> I think that the SerializeAsString() and ParseFromArray() calls should be
moved
> up to ProtoDatabaseImpl so that the underlying blocking database wouldn't
depend
> on the template parameter.
> 
> You could even make a layer above ProtoDatabaseImpl that does those
conversions,
> but I don't think the benefit of doing that is as high and the cost (in code
> complexity) is definitely higher.
> 
> ...
> 
> As I think about this more, maybe all of this would be even more useful as
just
> a  string-based interface to leveldb (with this simple, blocking-thread-aware
> api) and then also a proto-based wrapper of that that would do the
conversions.
> 
> Anyway, don't let those thoughts block this change, we can wait until we
> actually have a user/usecase for the non-proto version.

Gotcha. The Database no longer depends on proto. The conversion is done within
the Impl. I had to modify some expectation functions in the test to convert
to/from string and proto.

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
components/leveldb_proto/core/proto_database_impl.h:79: virtual void Init(const
base::FilePath& database_dir,
On 2014/06/13 19:41:05, cjhopman wrote:
> I don't think I like that this Init() is exposed to users and not just part of
> the constructor (the underlying db needs to have the init() to do setup on the
> blocking thread).
> 
> No need to fix it now, but it would be nice to do it before it becomes hard to
> fix.

Ack. Added a TODO with you as point of contact.

https://codereview.chromium.org/330833002/diff/20001/components/leveldb_proto...
components/leveldb_proto/core/proto_database_impl.h:104: using
base::MessageLoop;
On 2014/06/13 19:41:05, cjhopman wrote:
> Remove these usings

Done.

[blundell, 2014-06-16 14:08:41]

I'll defer to Chris for the main review. Please ping me for components OWNERS
once you have his lgtm. Thanks!

[Mathieu, 2014-06-16 14:10:04]

On 2014/06/16 14:08:41, blundell wrote:
> I'll defer to Chris for the main review. Please ping me for components OWNERS
> once you have his lgtm. Thanks!

Thanks for the unintended lgtm Colin :) I'll wait for Chris and ping you back!

[cjhopman, 2014-06-16 22:44:44]

https://codereview.chromium.org/330833002/diff/40001/components/leveldb_proto...
File components/leveldb_proto/core/proto_database_impl.h (right):

https://codereview.chromium.org/330833002/diff/40001/components/leveldb_proto...
components/leveldb_proto/core/proto_database_impl.h:41: class Database {
Since this is an inner class of ProtoDatabaseImpl it still depends on the type T
(I would hope the compiler is smart enough to realize that the generated code is
independent of the type, but idk and it probably still needs to do a large part
of the code generation for each different type).

The implementation of this and LevelDB don't refer to the type (or to
ProtoDatabaseImpl) so they can be moved out of this class (though then you need
to come up with a good name for them).

[Mathieu, 2014-06-17 17:41:00]

Thanks, have a look!

https://codereview.chromium.org/330833002/diff/40001/components/leveldb_proto...
File components/leveldb_proto/core/proto_database_impl.h (right):

https://codereview.chromium.org/330833002/diff/40001/components/leveldb_proto...
components/leveldb_proto/core/proto_database_impl.h:41: class Database {
On 2014/06/16 22:44:44, cjhopman wrote:
> Since this is an inner class of ProtoDatabaseImpl it still depends on the type
T
> (I would hope the compiler is smart enough to realize that the generated code
is
> independent of the type, but idk and it probably still needs to do a large
part
> of the code generation for each different type).
> 
> The implementation of this and LevelDB don't refer to the type (or to
> ProtoDatabaseImpl) so they can be moved out of this class (though then you
need
> to come up with a good name for them).

Per discussion, I removed Database and kept LevelDB, now in a separate file.
Works well, and there are no circular dependencies.

[cjhopman, 2014-06-17 18:05:08]

lgtm

https://codereview.chromium.org/330833002/diff/80001/components/leveldb_proto...
File components/leveldb_proto/core/leveldb_database.h (right):

https://codereview.chromium.org/330833002/diff/80001/components/leveldb_proto...
components/leveldb_proto/core/leveldb_database.h:17: }  // end leveldb
nit: s/end/namespace

[Mathieu, 2014-06-17 18:08:56]

Thanks!

blundell: components/
darin: chrome/browser/DEPS

https://codereview.chromium.org/330833002/diff/80001/components/leveldb_proto...
File components/leveldb_proto/core/leveldb_database.h (right):

https://codereview.chromium.org/330833002/diff/80001/components/leveldb_proto...
components/leveldb_proto/core/leveldb_database.h:17: }  // end leveldb
On 2014/06/17 18:05:08, cjhopman wrote:
> nit: s/end/namespace

Done.

[blundell, 2014-06-17 18:12:51]

You need to add leveldb_proto.gypi.

https://codereview.chromium.org/330833002/diff/100001/components/leveldb_prot...
File components/leveldb_proto/core/leveldb_database.h (right):

https://codereview.chromium.org/330833002/diff/100001/components/leveldb_prot...
components/leveldb_proto/core/leveldb_database.h:5: #ifndef
COMPONENTS_LEVELDB_PROTO_CORE_LEVEL_DATABASE_H_
This component isn't a layered component, so you can eliminate the core/. Easy
way to do this is git mv components/leveldb_proto/core components/leveldb_proto
followed by tools/git/mass-rename.py.

[blundell, 2014-06-17 18:13:51]

https://codereview.chromium.org/330833002/diff/100001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/dom_distiller_service_factory.cc (right):

https://codereview.chromium.org/330833002/diff/100001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/dom_distiller_service_factory.cc:24: :
DomDistillerService(store.Pass(), distiller_factory.Pass(),
You can TBR jochen or thakis as //chrome OWNERS for this change.

[Mathieu, 2014-06-17 18:30:54]

Thanks! Hope you won't mind me still using the leveldb_proto_core target name. I
find it differentiates it from the _test_support target.

https://codereview.chromium.org/330833002/diff/100001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/dom_distiller_service_factory.cc (right):

https://codereview.chromium.org/330833002/diff/100001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/dom_distiller_service_factory.cc:24: :
DomDistillerService(store.Pass(), distiller_factory.Pass(),
On 2014/06/17 18:13:51, blundell wrote:
> You can TBR jochen or thakis as //chrome OWNERS for this change.

Done.

https://codereview.chromium.org/330833002/diff/100001/components/leveldb_prot...
File components/leveldb_proto/core/leveldb_database.h (right):

https://codereview.chromium.org/330833002/diff/100001/components/leveldb_prot...
components/leveldb_proto/core/leveldb_database.h:5: #ifndef
COMPONENTS_LEVELDB_PROTO_CORE_LEVEL_DATABASE_H_
On 2014/06/17 18:12:51, blundell wrote:
> This component isn't a layered component, so you can eliminate the core/. Easy
> way to do this is git mv components/leveldb_proto/core
components/leveldb_proto
> followed by tools/git/mass-rename.py.

Done. Nice tool!

[Mathieu, 2014-06-17 18:31:34]

+dgrogan third_party/leveldatabase OWNERS, because apparently I have to...

[blundell, 2014-06-17 18:37:05]

LGTM

Make sure to add jochen as a reviewer and specify what you're asking him to TBR.
(I think you could also TBR the third_party/leveldb DEPS addition fwiw).

https://codereview.chromium.org/330833002/diff/120001/components/leveldb_prot...
File components/leveldb_proto.gypi (right):

https://codereview.chromium.org/330833002/diff/120001/components/leveldb_prot...
components/leveldb_proto.gypi:8: 'target_name': 'leveldb_proto_core',
This actually doesn't really make sense, it should be just leveldb_proto. Sorry
:\.

[Mathieu, 2014-06-17 19:31:18]

TBR jochen for chrome/browser/DEPS
TBR dgrogan because a presubmit check warns me I should add
third_party/leveldatabase OWNERS

https://codereview.chromium.org/330833002/diff/120001/components/leveldb_prot...
File components/leveldb_proto.gypi (right):

https://codereview.chromium.org/330833002/diff/120001/components/leveldb_prot...
components/leveldb_proto.gypi:8: 'target_name': 'leveldb_proto_core',
On 2014/06/17 18:37:05, blundell wrote:
> This actually doesn't really make sense, it should be just leveldb_proto.
Sorry
> :\.

Done.

[Mathieu, 2014-06-17 19:34:01]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-06-17 19:36:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mathp@chromium.org/330833002/160001

[Mathieu, 2014-06-17 20:32:21]

The CQ bit was unchecked by mathp@chromium.org

[Mathieu, 2014-06-17 20:54:07]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-06-17 20:57:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mathp@chromium.org/330833002/180001

[Mathieu, 2014-06-18 02:53:54]

The CQ bit was unchecked by mathp@chromium.org

[Mathieu, 2014-06-18 02:54:05]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-06-18 02:54:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mathp@chromium.org/330833002/180001

[commit-bot: I haz the power, 2014-06-18 05:12:51]

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

[commit-bot: I haz the power, 2014-06-18 05:12:52]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[Mathieu, 2014-06-18 12:10:43]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-06-18 12:11:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mathp@chromium.org/330833002/240001

[commit-bot: I haz the power, 2014-06-18 16:22:37]

Change committed as 278096