New issue
Advanced search Search tips

Issue 762587 link

Starred by 1 user
Status: Fixed
Owner:
cmumford@chromium.org
Closed: Oct 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Consolidate leveldb Options objects

Project Member Reported by cmumford@chromium.org, Sep 6 2017 
The Options structure for the leveldb service are redundantly implemented. Specifically:

1. The stock leveldb::Options object (from leveldb).
2. components/leveldb/public/interfaces/leveldb.mojom produces
   leveldb::mojom::OpenOptions. This is a (mostly) subset of 
   leveldb::Options.
3. leveldb_proto::Options (An even smaller subset of leveldb::Options).

These can all be combined so that the various leveldb wrappers use only the stock leveldb::Options object.
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 6 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3e2763058569e3ce28b0cdd52f4d362035446f4f

commit 3e2763058569e3ce28b0cdd52f4d362035446f4f
Author: Chris Mumford <cmumford@chromium.org>
Date: Wed Sep 06 20:49:05 2017

leveldb_proto: Added struct-trait for leveldb_env::Options.

Adding a typemap for leveldb.mojom.OpenOptions ↔ leveldb_env::Options provides
the benefit of having the Options default values specified in one and only one
place: the C++ constructors.

Bug:  762587 
Change-Id: Iab9bd12eb3758f626b04808a37b748ef13f42e97
Reviewed-on: https://chromium-review.googlesource.com/638051
Commit-Queue: Chris Mumford <cmumford@chromium.org>
Reviewed-by: Elliot Glaysher <erg@chromium.org>
Reviewed-by: Michael Nordman <michaeln@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500079}
[modify] https://crrev.com/3e2763058569e3ce28b0cdd52f4d362035446f4f/components/leveldb/BUILD.gn
[modify] https://crrev.com/3e2763058569e3ce28b0cdd52f4d362035446f4f/components/leveldb/OWNERS
[add] https://crrev.com/3e2763058569e3ce28b0cdd52f4d362035446f4f/components/leveldb/leveldb.typemap
[add] https://crrev.com/3e2763058569e3ce28b0cdd52f4d362035446f4f/components/leveldb/leveldb_mojo_unittest.cc
[modify] https://crrev.com/3e2763058569e3ce28b0cdd52f4d362035446f4f/components/leveldb/leveldb_service_impl.cc
[modify] https://crrev.com/3e2763058569e3ce28b0cdd52f4d362035446f4f/components/leveldb/leveldb_service_impl.h
[modify] https://crrev.com/3e2763058569e3ce28b0cdd52f4d362035446f4f/components/leveldb/leveldb_service_unittest.cc
[add] https://crrev.com/3e2763058569e3ce28b0cdd52f4d362035446f4f/components/leveldb/leveldb_struct_traits.cc
[add] https://crrev.com/3e2763058569e3ce28b0cdd52f4d362035446f4f/components/leveldb/leveldb_struct_traits.h
[modify] https://crrev.com/3e2763058569e3ce28b0cdd52f4d362035446f4f/components/leveldb/public/interfaces/leveldb.mojom
[modify] https://crrev.com/3e2763058569e3ce28b0cdd52f4d362035446f4f/components/typemaps.gni
[modify] https://crrev.com/3e2763058569e3ce28b0cdd52f4d362035446f4f/content/browser/dom_storage/local_storage_context_mojo.cc
[modify] https://crrev.com/3e2763058569e3ce28b0cdd52f4d362035446f4f/content/browser/dom_storage/local_storage_context_mojo_unittest.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 12 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/328d3beea8bbc40028bf0f6b664ec62b36104d29

commit 328d3beea8bbc40028bf0f6b664ec62b36104d29
Author: Chris Mumford <cmumford@chromium.org>
Date: Tue Sep 12 17:44:24 2017

leveldb: Deleted leveldb_proto::Options.

leveldb_proto::Options is a small subset (mostly) of leveldb::Options.
It had two leveldb::Options members (shared_cache and write_buffer_size),
and a (possibly empty) database_name. This change deletes
leveldb_proto::Options in lieu of using leveldb::Options. This simplifies
the code as well as adds support for the remaining options specified
in leveldb::Options.

Bug:  762587 
Change-Id: Ia7872ecb09b2b181354c820ac3296ea6304a80b0
Reviewed-on: https://chromium-review.googlesource.com/655697
Reviewed-by: Mathieu Perreault <mathp@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: John Mellor <johnme@chromium.org>
Commit-Queue: Chris Mumford <cmumford@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501330}
[modify] https://crrev.com/328d3beea8bbc40028bf0f6b664ec62b36104d29/chrome/browser/budget_service/budget_database.cc
[modify] https://crrev.com/328d3beea8bbc40028bf0f6b664ec62b36104d29/components/download/internal/download_store.cc
[modify] https://crrev.com/328d3beea8bbc40028bf0f6b664ec62b36104d29/components/leveldb_proto/BUILD.gn
[modify] https://crrev.com/328d3beea8bbc40028bf0f6b664ec62b36104d29/components/leveldb_proto/leveldb_database.cc
[modify] https://crrev.com/328d3beea8bbc40028bf0f6b664ec62b36104d29/components/leveldb_proto/leveldb_database.h
[delete] https://crrev.com/fc369f2ae8c48e1a214ad71bb752c60deb8618fb/components/leveldb_proto/options.h
[modify] https://crrev.com/328d3beea8bbc40028bf0f6b664ec62b36104d29/components/leveldb_proto/proto_database.h
[modify] https://crrev.com/328d3beea8bbc40028bf0f6b664ec62b36104d29/components/leveldb_proto/proto_database_impl.h
[modify] https://crrev.com/328d3beea8bbc40028bf0f6b664ec62b36104d29/components/leveldb_proto/proto_database_impl_unittest.cc
[modify] https://crrev.com/328d3beea8bbc40028bf0f6b664ec62b36104d29/components/leveldb_proto/testing/fake_db.h
[modify] https://crrev.com/328d3beea8bbc40028bf0f6b664ec62b36104d29/components/ntp_snippets/remote/remote_suggestions_database.cc

Comment 3 by cmumford@chromium.org, Sep 12 2017 

This issue is technically resolved with the first two CL's. I'm going to keep it open for https://chromium-review.googlesource.com/c/chromium/src/+/655145, which is related, and then mark as fixed.
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 21 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/29f2cf57dc2259579d2a21ab935391f893efef8c

commit 29f2cf57dc2259579d2a21ab935391f893efef8c
Author: Chris Mumford <cmumford@chromium.org>
Date: Thu Sep 21 00:35:40 2017

Merged InitWithOptions() & Init() for LevelDB and ProtoDatabase.

LevelDB and ProtoDatabase had each had Init() and InitWithOptions()
members. LevelDB::Init() had it's own default option values whereas
ProtoDatabase::Init() used the stock leveldb_env::Options() instance.

This change simplifies the API by combining Init() and InitWithOptions()
into a single Init() that is passed an options instance where all
options values are honored *except* |env| and only if database_name
is empty (ie in-memory db).

Bug:  762587 
Change-Id: I70b547b4c1bbc5542c58fb31faf12d0ba2de3f0b
Reviewed-on: https://chromium-review.googlesource.com/655145
Reviewed-by: Xing Liu <xingliu@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Nicolas Zea <zea@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Commit-Queue: Chris Mumford <cmumford@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503294}
[modify] https://crrev.com/29f2cf57dc2259579d2a21ab935391f893efef8c/chrome/browser/budget_service/budget_database.cc
[modify] https://crrev.com/29f2cf57dc2259579d2a21ab935391f893efef8c/components/dom_distiller/core/dom_distiller_store.cc
[modify] https://crrev.com/29f2cf57dc2259579d2a21ab935391f893efef8c/components/download/internal/download_store.cc
[modify] https://crrev.com/29f2cf57dc2259579d2a21ab935391f893efef8c/components/feature_engagement/internal/persistent_availability_store.cc
[modify] https://crrev.com/29f2cf57dc2259579d2a21ab935391f893efef8c/components/feature_engagement/internal/persistent_event_store.cc
[modify] https://crrev.com/29f2cf57dc2259579d2a21ab935391f893efef8c/components/gcm_driver/crypto/gcm_key_store.cc
[modify] https://crrev.com/29f2cf57dc2259579d2a21ab935391f893efef8c/components/leveldb_proto/BUILD.gn
[modify] https://crrev.com/29f2cf57dc2259579d2a21ab935391f893efef8c/components/leveldb_proto/leveldb_database.cc
[modify] https://crrev.com/29f2cf57dc2259579d2a21ab935391f893efef8c/components/leveldb_proto/leveldb_database.h
[add] https://crrev.com/29f2cf57dc2259579d2a21ab935391f893efef8c/components/leveldb_proto/proto_database.cc
[modify] https://crrev.com/29f2cf57dc2259579d2a21ab935391f893efef8c/components/leveldb_proto/proto_database.h
[modify] https://crrev.com/29f2cf57dc2259579d2a21ab935391f893efef8c/components/leveldb_proto/proto_database_impl.h
[modify] https://crrev.com/29f2cf57dc2259579d2a21ab935391f893efef8c/components/leveldb_proto/proto_database_impl_unittest.cc
[modify] https://crrev.com/29f2cf57dc2259579d2a21ab935391f893efef8c/components/leveldb_proto/testing/fake_db.h
[modify] https://crrev.com/29f2cf57dc2259579d2a21ab935391f893efef8c/components/ntp_snippets/remote/remote_suggestions_database.cc
[modify] https://crrev.com/29f2cf57dc2259579d2a21ab935391f893efef8c/components/suggestions/image_manager.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 26 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/96e48af52beed52e443c8368392ebdd8878a2d14

commit 96e48af52beed52e443c8368392ebdd8878a2d14
Author: Chris Mumford <cmumford@chromium.org>
Date: Tue Sep 26 16:40:03 2017

leveldb: Deleted unused forward declaration of SharedReadCache.

In 328d3beea8bb InitWithOptions was modified to no longer require
a SharedReadCache parameter. A few leftover forward declarations
of leveldb_env::SharedReadCache were not removed.

TBR=johnme@chromium.org,qinmin@chromium.org,nyquist@chromium.org,jkrcal@chromium.org

TBRing reviewers:
johnme@chromium.org: chrome/browser/budget_service/budget_database.cc
qinmin@chromium.org: components/download/internal/download_store.cc
nyquist@chromium.org: components/leveldb_proto/proto_database_impl_unittest.cc
jkrcal@chromium.org: components/ntp_snippets/remote/remote_suggestions_database.cc

Bug:  762587 
Change-Id: Ib0b515b0d6bf7b11bbee05fcabc60c59d0981d1a
Reviewed-on: https://chromium-review.googlesource.com/683002
Reviewed-by: Chris Mumford <cmumford@chromium.org>
Commit-Queue: Chris Mumford <cmumford@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504396}
[modify] https://crrev.com/96e48af52beed52e443c8368392ebdd8878a2d14/chrome/browser/budget_service/budget_database.cc
[modify] https://crrev.com/96e48af52beed52e443c8368392ebdd8878a2d14/components/download/internal/download_store.cc
[modify] https://crrev.com/96e48af52beed52e443c8368392ebdd8878a2d14/components/leveldb_proto/proto_database_impl_unittest.cc
[modify] https://crrev.com/96e48af52beed52e443c8368392ebdd8878a2d14/components/ntp_snippets/remote/remote_suggestions_database.cc

Comment 6 by cmumford@chromium.org, Oct 11 2017

Status: Fixed (was: Started)
►

Sign in to add a comment