[Offline] Creates metadata store interface for offline pages

[Offline] Creates metadata store interface for offline pages

Offline pages metadata store:
* Creates interface for the store
* The following actions are available on the store:
  - AddOfflinePage,
  - RemoveOfflinePage,
  - Load,
* Updates to the components_unittests to include offline
  pages

BUG=491352
Committed: https://crrev.com/2475986c9476407f0cec00d2eb660b480db30aae
Cr-Commit-Position: refs/heads/master@{#333697}

[fgorski, 2015-06-03 18:05:31]

fgorski@chromium.org changed reviewers:
+ dimich@chromium.org

[fgorski, 2015-06-03 18:08:24]

fgorski@chromium.org changed reviewers:
+ blundell@chromium.org

[fgorski, 2015-06-03 18:08:25]

PTAL

dimich@: components/offline_pages

blundell@: components/*.gn and *.gyp

Thanks.
Filip

[fgorski, 2015-06-03 18:09:07]

Patchset #1 (id:1) has been deleted

[Dmitry Titov, 2015-06-04 00:25:53]

https://codereview.chromium.org/1160283003/diff/20001/components/offline_page...
File components/offline_pages/DEPS (right):

https://codereview.chromium.org/1160283003/diff/20001/components/offline_page...
components/offline_pages/DEPS:4: "+third_party/leveldatabase",
If we can avoid this, it'd be great. For example, we might need to implement a
DB layer in Java on Android to be able to serve it via Content Provider to other
apps (like Stars or Icicle) with good perf even if Chrome is not loaded. If we
abstract the OfflinePagesModel from data storing layer, we migh tbe able to do
it simply in the future.

https://codereview.chromium.org/1160283003/diff/20001/components/offline_page...
File components/offline_pages/offline_page_metadata_store_impl.cc (right):

https://codereview.chromium.org/1160283003/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:5: #include
"components/offline_pages/offline_page_metadata_store_impl.h"
If we move the dependency on leveldb to embedder, this file will likely live
under chrome/browser/offline_pages...

[blundell, 2015-06-04 10:52:55]

//components/BUILD.gn and //components/components_tests.gyp lgtm

[fgorski, 2015-06-05 21:28:42]

Dmitry, PTAL,

I've followed your recommendation and will submit a patch with implementation in
chrome/browser/offline_pages

Thanks.
Filip

https://codereview.chromium.org/1160283003/diff/20001/components/offline_page...
File components/offline_pages/DEPS (right):

https://codereview.chromium.org/1160283003/diff/20001/components/offline_page...
components/offline_pages/DEPS:4: "+third_party/leveldatabase",
On 2015/06/04 00:25:52, Dmitry Titov wrote:
> If we can avoid this, it'd be great. For example, we might need to implement a
> DB layer in Java on Android to be able to serve it via Content Provider to
other
> apps (like Stars or Icicle) with good perf even if Chrome is not loaded. If we
> abstract the OfflinePagesModel from data storing layer, we migh tbe able to do
> it simply in the future.

Done.

https://codereview.chromium.org/1160283003/diff/20001/components/offline_page...
File components/offline_pages/offline_page_metadata_store_impl.cc (right):

https://codereview.chromium.org/1160283003/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:5: #include
"components/offline_pages/offline_page_metadata_store_impl.h"
On 2015/06/04 00:25:53, Dmitry Titov wrote:
> If we move the dependency on leveldb to embedder, this file will likely live
> under chrome/browser/offline_pages...

Done.

[Dmitry Titov, 2015-06-08 21:47:21]

https://codereview.chromium.org/1160283003/diff/60001/components/offline_page...
File components/offline_pages/offline_page_metadata_store.h (right):

https://codereview.chromium.org/1160283003/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store.h:18: //
OfflinePageMetadataStore keeps metadata for the offline pages.
Would be nice to add details, for example:
- is it used as singleton or multiple independent ones will be created?
- Lifetime - for example, will the AddOfflinePage be successful if the caller
destroys an instance of this class w/o waiting for UpdateCallback?

https://codereview.chromium.org/1160283003/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store.h:28: // Get all of the
offline pages from the store.
Since these methods are async, it's useful to document the allowed usage
patterns:
- can those be issued in parallel
- what is expected result of load/update/load sequence that does not wait for
callbacks in between?

https://codereview.chromium.org/1160283003/diff/60001/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1160283003/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.cc:26:
store_.reset(store.release());
store_ = store.Pass();
?

[fgorski, 2015-06-09 20:12:18]

PTAL

https://codereview.chromium.org/1160283003/diff/60001/components/offline_page...
File components/offline_pages/offline_page_metadata_store.h (right):

https://codereview.chromium.org/1160283003/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store.h:18: //
OfflinePageMetadataStore keeps metadata for the offline pages.
On 2015/06/08 21:47:21, Dmitry Titov wrote:
> Would be nice to add details, for example:
> - is it used as singleton or multiple independent ones will be created?
> - Lifetime - for example, will the AddOfflinePage be successful if the caller
> destroys an instance of this class w/o waiting for UpdateCallback?

Done.

I added a comment that nothing like that is expected at the interface level, but
that calling operations in parallel should be possible.

https://codereview.chromium.org/1160283003/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store.h:28: // Get all of the
offline pages from the store.
On 2015/06/08 21:47:21, Dmitry Titov wrote:
> Since these methods are async, it's useful to document the allowed usage
> patterns:
> - can those be issued in parallel
> - what is expected result of load/update/load sequence that does not wait for
> callbacks in between?

Done.

https://codereview.chromium.org/1160283003/diff/60001/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1160283003/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.cc:26:
store_.reset(store.release());
On 2015/06/08 21:47:21, Dmitry Titov wrote:
> store_ = store.Pass();
> ?

Done.

[Dmitry Titov, 2015-06-09 21:58:05]

Almost there! 2 more:

https://codereview.chromium.org/1160283003/diff/80001/components/offline_page...
File components/offline_pages/offline_page_metadata_store.h (right):

https://codereview.chromium.org/1160283003/diff/80001/components/offline_page...
components/offline_pages/offline_page_metadata_store.h:20: // asynchronous
opertaions when the object is being destroyed, before such
typo: opertaions

https://codereview.chromium.org/1160283003/diff/80001/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1160283003/diff/80001/components/offline_page...
components/offline_pages/offline_page_model.cc:26: store_ = store.Pass();
This looks like part of a constructor now. Can it merge into OfflinePageModel
ctor?

[fgorski, 2015-06-09 22:11:13]

Addressed

https://codereview.chromium.org/1160283003/diff/80001/components/offline_page...
File components/offline_pages/offline_page_metadata_store.h (right):

https://codereview.chromium.org/1160283003/diff/80001/components/offline_page...
components/offline_pages/offline_page_metadata_store.h:20: // asynchronous
opertaions when the object is being destroyed, before such
On 2015/06/09 21:58:05, Dmitry Titov wrote:
> typo: opertaions

Done.

https://codereview.chromium.org/1160283003/diff/80001/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1160283003/diff/80001/components/offline_page...
components/offline_pages/offline_page_model.cc:26: store_ = store.Pass();
On 2015/06/09 21:58:05, Dmitry Titov wrote:
> This looks like part of a constructor now. Can it merge into OfflinePageModel
> ctor?

Done.

[Dmitry Titov, 2015-06-09 23:24:45]

lgtm

[fgorski, 2015-06-10 06:42:25]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2015-06-10 06:42:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1160283003/#ps100001
(title: "Addressing more feedback")

[commit-bot: I haz the power, 2015-06-10 06:42:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1160283003/100001

[commit-bot: I haz the power, 2015-06-10 07:45:19]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-06-10 07:46:16]

Patchset 5 (id:??) landed as
https://crrev.com/2475986c9476407f0cec00d2eb660b480db30aae
Cr-Commit-Position: refs/heads/master@{#333697}