Refactor the offline page storage to include client namespace and id.

Refactor the offline page storage to include client namespace and id.

BUG=

Committed: https://crrev.com/16d153334496e8eb7aeeb7352515a6dbe97d95d3
Cr-Commit-Position: refs/heads/master@{#379153}

[bburns, 2016-02-12 21:19:45]

bburns@chromium.org changed reviewers:
+ dewittj@chromium.org, dimich@chromium.org, fgorski@chromium.org

[bburns, 2016-02-12 21:19:46]

Please take a look.

This is part #1, it doesn't actually enable multiple namespaces, or move the
Java code away from BookmarkId.  That will be part #2.

[fgorski, 2016-02-12 21:43:41]

I'll take a second look when you sort out compilation issues. But it looks
pretty good.

https://codereview.chromium.org/1694863003/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (right):

https://codereview.chromium.org/1694863003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_bridge.cc:19: #include
"components/offline_pages/proto/offline_pages.pb.h"
per discussion, perhaps we should limit protobuf code to storage only?

https://codereview.chromium.org/1694863003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_bridge.cc:184: const char *s =
env->GetStringUTFChars(client_id_namespace, NULL);
ConvertUTF8ToJavaString is what we use typically, that is once you operate on
strings. (when you introduce shadow struct)

https://codereview.chromium.org/1694863003/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_bridge.h (right):

https://codereview.chromium.org/1694863003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_bridge.h:49: jlong
bookmark_id);
nit: offline_id?

https://codereview.chromium.org/1694863003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_bridge.h:61: jstring
client_id_namespace,
nit: j_ for j_strings, as they cannot be directly used.

https://codereview.chromium.org/1694863003/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
(right):

https://codereview.chromium.org/1694863003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:41: const
char* kTestPage2ClientId = "1234";
did you keep values equal on purpose?

https://codereview.chromium.org/1694863003/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_metadata_store_impl.cc (right):

https://codereview.chromium.org/1694863003/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_impl.cc:64:
OfflinePageItem* item) {
this should also read the client id

https://codereview.chromium.org/1694863003/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1694863003/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:638: :
DeletePageResult::STORE_FAILURE);
nit: align

[Dmitry Titov, 2016-02-12 21:46:51]

https://codereview.chromium.org/1694863003/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1694863003/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:155: int64_t offline_id,
I don't know if you want to remove this parameter in one of the next patches,
but with ClientId there is no need to pass offline_id. It can be cooked up when
a page is saved and perhaps returned in SavePageCallback, right?

https://codereview.chromium.org/1694863003/diff/1/components/offline_pages/pr...
File components/offline_pages/proto/offline_pages.proto (right):

https://codereview.chromium.org/1694863003/diff/1/components/offline_pages/pr...
components/offline_pages/proto/offline_pages.proto:15: // ClientID represents an
identified in the API Client's namespace.  It is mapped to an offline_id which
Is there a need to map this client-provided info into offline_id? I'd rather
not, because we want the offline_id to be unique and that's it. The data
provided by clients is the client's business. For example, "Last N pages"
feature would simply provide namespace and empty id since it does not need to
store additional data separately.

I think we should just autogenerate offline_id rather that compute it form
ClientId.

[bburns, 2016-02-20 01:14:18]

Comments addressed. (and more...)

This should now match the design doc.

Please take another look.

Thanks!
--brendan

https://codereview.chromium.org/1694863003/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (right):

https://codereview.chromium.org/1694863003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_bridge.cc:19: #include
"components/offline_pages/proto/offline_pages.pb.h"
On 2016/02/12 21:43:40, fgorski wrote:
> per discussion, perhaps we should limit protobuf code to storage only?

Done.

https://codereview.chromium.org/1694863003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_bridge.cc:184: const char *s =
env->GetStringUTFChars(client_id_namespace, NULL);
On 2016/02/12 21:43:40, fgorski wrote:
> ConvertUTF8ToJavaString is what we use typically, that is once you operate on
> strings. (when you introduce shadow struct)

Done.

https://codereview.chromium.org/1694863003/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_bridge.h (right):

https://codereview.chromium.org/1694863003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_bridge.h:49: jlong
bookmark_id);
On 2016/02/12 21:43:40, fgorski wrote:
> nit: offline_id?

Done.

https://codereview.chromium.org/1694863003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_bridge.h:61: jstring
client_id_namespace,
On 2016/02/12 21:43:40, fgorski wrote:
> nit: j_ for j_strings, as they cannot be directly used.

Done I think?

https://codereview.chromium.org/1694863003/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
(right):

https://codereview.chromium.org/1694863003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:41: const
char* kTestPage2ClientId = "1234";
On 2016/02/12 21:43:41, fgorski wrote:
> did you keep values equal on purpose?

fixed.

https://codereview.chromium.org/1694863003/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_metadata_store_impl.cc (right):

https://codereview.chromium.org/1694863003/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_impl.cc:64:
OfflinePageItem* item) {
On 2016/02/12 21:43:41, fgorski wrote:
> this should also read the client id

Done.

https://codereview.chromium.org/1694863003/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1694863003/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:638: :
DeletePageResult::STORE_FAILURE);
On 2016/02/12 21:43:41, fgorski wrote:
> nit: align

Done.

https://codereview.chromium.org/1694863003/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1694863003/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:155: int64_t offline_id,
On 2016/02/12 21:46:51, Dmitry Titov wrote:
> I don't know if you want to remove this parameter in one of the next patches,
> but with ClientId there is no need to pass offline_id. It can be cooked up
when
> a page is saved and perhaps returned in SavePageCallback, right?

yes, I will do this in the next CL.

https://codereview.chromium.org/1694863003/diff/1/components/offline_pages/pr...
File components/offline_pages/proto/offline_pages.proto (right):

https://codereview.chromium.org/1694863003/diff/1/components/offline_pages/pr...
components/offline_pages/proto/offline_pages.proto:15: // ClientID represents an
identified in the API Client's namespace.  It is mapped to an offline_id which
On 2016/02/12 21:46:51, Dmitry Titov wrote:
> Is there a need to map this client-provided info into offline_id? I'd rather
> not, because we want the offline_id to be unique and that's it. The data
> provided by clients is the client's business. For example, "Last N pages"
> feature would simply provide namespace and empty id since it does not need to
> store additional data separately.
> 
> I think we should just autogenerate offline_id rather that compute it form
> ClientId.

Done.

[dewittj, 2016-02-22 19:23:16]

If I understand correctly, we intend to just autogenerate offline_ids on behalf
of clients.  (My info might be out of date from my vacation).

However, this patch begins storing bookmark IDs into the offline_id proto field,
which could complicate things for us later if we ever decided to e.g. use an
autoincrement value or some other nonrandom scheme.  What is the proposed
upgrade path from this patch to the final patch where everyone uses the correct
scheme for offline IDs?  It might make sense not to populate the offline_id
field on disk for now, relying on deprecated_bookmark_id until all clients are
in agreement about what sort of scheme is in use.

https://codereview.chromium.org/1694863003/diff/40001/components/offline_page...
File components/offline_pages/offline_page_item.h (right):

https://codereview.chromium.org/1694863003/diff/40001/components/offline_page...
components/offline_pages/offline_page_item.h:19: struct ClientId {
To me this name is a little confusing, it seems like it's the id for a given
client but in fact it's an ID that is namespaced by client.  Is there another
name that doesn't have this problem?

https://codereview.chromium.org/1694863003/diff/40001/components/offline_page...
components/offline_pages/offline_page_item.h:21: std::string space;
maybe name_space is a better option here.

https://codereview.chromium.org/1694863003/diff/40001/components/offline_page...
components/offline_pages/offline_page_item.h:66: std::string
client_id_namespace;
if you rename ClientId.space -> ClientId.name_space then this should also get an
underscore.

[bburns, 2016-02-22 21:02:31]

Comments (mostly) addressed.  please take another look.

https://codereview.chromium.org/1694863003/diff/40001/components/offline_page...
File components/offline_pages/offline_page_item.h (right):

https://codereview.chromium.org/1694863003/diff/40001/components/offline_page...
components/offline_pages/offline_page_item.h:19: struct ClientId {
On 2016/02/22 19:23:16, dewittj wrote:
> To me this name is a little confusing, it seems like it's the id for a given
> client but in fact it's an ID that is namespaced by client.  Is there another
> name that doesn't have this problem?

This is the best I could come up with, I'm open to other names...

https://codereview.chromium.org/1694863003/diff/40001/components/offline_page...
components/offline_pages/offline_page_item.h:21: std::string space;
On 2016/02/22 19:23:16, dewittj wrote:
> maybe name_space is a better option here.

Done.

https://codereview.chromium.org/1694863003/diff/40001/components/offline_page...
components/offline_pages/offline_page_item.h:66: std::string
client_id_namespace;
On 2016/02/22 19:23:16, dewittj wrote:
> if you rename ClientId.space -> ClientId.name_space then this should also get
an
> underscore.

Done.

[fgorski, 2016-02-23 17:15:31]

https://codereview.chromium.org/1694863003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1694863003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:91:
public void offlinePageDeleted(BookmarkId bookmarkId) {}
I think this is already an offlineId on the C++ side.

https://codereview.chromium.org/1694863003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:228:
public long[] getOfflineIdsForClientId(String clientIdNamespace, String
clientId) {
Looking at all the other methods in this class, and across chromium codebase,
this should be a Set<Long>.

It should still be fine to get an array from native side, but you should wrap it
in a collection here.

https://codereview.chromium.org/1694863003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:241:
public OfflinePageItem getPageByBookmarkId(BookmarkId bookmarkId) {
It bothers me that we are not converting the bookmarkId to the offline page ID
here properly. At least add a todo. And we have quite a few places in this file
with the same problem.

https://codereview.chromium.org/1694863003/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (right):

https://codereview.chromium.org/1694863003/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_bridge.cc:185:
client_id_namespace,
did git cl format break this line like that?

also, j_client_id_namespace

https://codereview.chromium.org/1694863003/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_bridge.cc:186: const
JavaParamRef<jstring>& client_id) {
j_client_id

https://codereview.chromium.org/1694863003/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_bridge.cc:201: cid.name_space
= ConvertJavaStringToUTF8(env, client_id_namespace);
seems like we need a better name than namespace here.

https://codereview.chromium.org/1694863003/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_utils.cc (right):

https://codereview.chromium.org/1694863003/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:109: if
(offline_page->client_id_name_space != "bookmark") {
this should be declared someplace in offline pages as a const. Perhaps in the
model.

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

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
components/offline_pages/offline_page_item.h:21: std::string name_space;
that earlier namespace comment should be here.

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
components/offline_pages/offline_page_item.h:62: // The Offline (internal) ID
related to the offline page.
If it is internal, does that mean we don't share it outside of the internals of
offline pages?

(I'd be happy with that, but please consider updating the documentation if that
is not the case.

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
components/offline_pages/offline_page_item.h:66: std::string
client_id_name_space;
why not just hold ClientId here?

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

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:102: &&
item_proto.deprecated_bookmark_id() != 0) {
we need to check for has_deprecated_bookmark_id, as 0 is a valid id.
-1 is the invalid one in case of bookmarks.

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:103:
item->client_id_name_space = "bookmark";
refer to const for bookmarks namespace

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

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:154:
EXPECT_EQ(offline_page.access_count, offline_pages_[0].access_count);
add checks for client ID related fields.

Applies to other tests.

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

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:50: class
OfflinePageModelTest
Any chances of a test where query for offline_page_id returns multiple results
for a single Client ID?

And if not, let's have that captured by a bug/todo/design doc with work items.

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
File components/offline_pages/proto/offline_pages.proto (right):

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
components/offline_pages/proto/offline_pages.proto:20: optional int64
deprecated_bookmark_id = 2;
specify default to -1 as that is the invalid bookmark id, per:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

[bburns, 2016-02-23 19:25:39]

Comments addressed.  TODO'd a couple of things.

Please take another look.

Thanks!
--brendan

https://codereview.chromium.org/1694863003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1694863003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:91:
public void offlinePageDeleted(BookmarkId bookmarkId) {}
On 2016/02/23 17:15:30, fgorski wrote:
> I think this is already an offlineId on the C++ side.

This changes the consumer API, I'd rather change that later.  (see below for the
conversion code.)

TODO'd

https://codereview.chromium.org/1694863003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:228:
public long[] getOfflineIdsForClientId(String clientIdNamespace, String
clientId) {
On 2016/02/23 17:15:30, fgorski wrote:
> Looking at all the other methods in this class, and across chromium codebase,
> this should be a Set<Long>.
> 
> It should still be fine to get an array from native side, but you should wrap
it
> in a collection here.

Done.

https://codereview.chromium.org/1694863003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:241:
public OfflinePageItem getPageByBookmarkId(BookmarkId bookmarkId) {
On 2016/02/23 17:15:30, fgorski wrote:
> It bothers me that we are not converting the bookmarkId to the offline page ID
> here properly. At least add a todo. And we have quite a few places in this
file
> with the same problem.

Ok, I fixed that here.

https://codereview.chromium.org/1694863003/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (right):

https://codereview.chromium.org/1694863003/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_bridge.cc:185:
client_id_namespace,
On 2016/02/23 17:15:30, fgorski wrote:
> did git cl format break this line like that?

No, its > 80 cols, so we have to line break, that's the correct indent.  (I
agree its ugly)
 
> also, j_client_id_namespace

Done.

https://codereview.chromium.org/1694863003/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_bridge.cc:186: const
JavaParamRef<jstring>& client_id) {
On 2016/02/23 17:15:30, fgorski wrote:
> j_client_id

Done.

https://codereview.chromium.org/1694863003/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_bridge.cc:201: cid.name_space
= ConvertJavaStringToUTF8(env, client_id_namespace);
On 2016/02/23 17:15:30, fgorski wrote:
> seems like we need a better name than namespace here.

Namespace seems like the correct name, but I'm ok with other suggestions.

https://codereview.chromium.org/1694863003/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_utils.cc (right):

https://codereview.chromium.org/1694863003/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:109: if
(offline_page->client_id_name_space != "bookmark") {
On 2016/02/23 17:15:30, fgorski wrote:
> this should be declared someplace in offline pages as a const. Perhaps in the
> model.

Done.

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

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
components/offline_pages/offline_page_item.h:21: std::string name_space;
On 2016/02/23 17:15:30, fgorski wrote:
> that earlier namespace comment should be here.

which comment?

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
components/offline_pages/offline_page_item.h:62: // The Offline (internal) ID
related to the offline page.
On 2016/02/23 17:15:30, fgorski wrote:
> If it is internal, does that mean we don't share it outside of the internals
of
> offline pages?
> 
> (I'd be happy with that, but please consider updating the documentation if
that
> is not the case.

Done.

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
components/offline_pages/offline_page_item.h:66: std::string
client_id_name_space;
On 2016/02/23 17:15:30, fgorski wrote:
> why not just hold ClientId here?

Done.

(historical note, it was from before when the object was from a proto and you
couldn't forward declare it..)

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

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:102: &&
item_proto.deprecated_bookmark_id() != 0) {
On 2016/02/23 17:15:30, fgorski wrote:
> we need to check for has_deprecated_bookmark_id, as 0 is a valid id.
> -1 is the invalid one in case of bookmarks.

Done.

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:103:
item->client_id_name_space = "bookmark";
On 2016/02/23 17:15:30, fgorski wrote:
> refer to const for bookmarks namespace

Done.

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

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:154:
EXPECT_EQ(offline_page.access_count, offline_pages_[0].access_count);
On 2016/02/23 17:15:30, fgorski wrote:
> add checks for client ID related fields.
> 
> Applies to other tests.

Done.

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

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:50: class
OfflinePageModelTest
On 2016/02/23 17:15:31, fgorski wrote:
> Any chances of a test where query for offline_page_id returns multiple results
> for a single Client ID?
> 
> And if not, let's have that captured by a bug/todo/design doc with work items.

TODO'd I promise to do it in the next CL, but this one is already pretty big...

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
File components/offline_pages/proto/offline_pages.proto (right):

https://codereview.chromium.org/1694863003/diff/60001/components/offline_page...
components/offline_pages/proto/offline_pages.proto:20: optional int64
deprecated_bookmark_id = 2;
On 2016/02/23 17:15:31, fgorski wrote:
> specify default to -1 as that is the invalid bookmark id, per:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

Done.

[Dmitry Titov, 2016-02-23 20:35:20]

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

https://codereview.chromium.org/1694863003/diff/80001/components/offline_page...
components/offline_pages/offline_page_model.cc:504: cid.name_space = "bookmark";
This should be pulled into a constant. You already have one in the .h file.

https://codereview.chromium.org/1694863003/diff/80001/components/offline_page...
components/offline_pages/offline_page_model.cc:519: cid.name_space = "bookmark";
same as above

https://codereview.chromium.org/1694863003/diff/80001/components/offline_page...
components/offline_pages/offline_page_model.cc:542: cid.name_space = "bookmark";
same ^^

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

https://codereview.chromium.org/1694863003/diff/80001/components/offline_page...
components/offline_pages/offline_page_model.h:157: int64_t offline_id,
Was the intent to return the freshly-minted offlline-id in the callback for
savePage()? It is a byproduct of saving the page, not something the clients come
up with ahead of time, correct?

[bburns, 2016-02-23 20:58:30]

comments addressed, please take another look.

thanks!
--brendan

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

https://codereview.chromium.org/1694863003/diff/80001/components/offline_page...
components/offline_pages/offline_page_model.cc:504: cid.name_space = "bookmark";
On 2016/02/23 20:35:20, Dmitry Titov wrote:
> This should be pulled into a constant. You already have one in the .h file.

Done.

https://codereview.chromium.org/1694863003/diff/80001/components/offline_page...
components/offline_pages/offline_page_model.cc:519: cid.name_space = "bookmark";
On 2016/02/23 20:35:20, Dmitry Titov wrote:
> same as above

Done.

https://codereview.chromium.org/1694863003/diff/80001/components/offline_page...
components/offline_pages/offline_page_model.cc:542: cid.name_space = "bookmark";
On 2016/02/23 20:35:20, Dmitry Titov wrote:
> same ^^

Done.

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

https://codereview.chromium.org/1694863003/diff/80001/components/offline_page...
components/offline_pages/offline_page_model.h:157: int64_t offline_id,
On 2016/02/23 20:35:20, Dmitry Titov wrote:
> Was the intent to return the freshly-minted offlline-id in the callback for
> savePage()? It is a byproduct of saving the page, not something the clients
come
> up with ahead of time, correct? 

Yes, though I was planning on doing this on the Java side, not the C++ side, but
I don't really care.

Also, I was planning on doing that in the next CL in a (somewhat failed) attempt
to limit the scope of this CL.

[bburns, 2016-02-23 22:06:49]

ugh, there are some more bugs to wring out.  I will ping this CL when it is
ready to review again...

[bburns, 2016-02-24 00:16:24]

Ok, this has been validated to work both on the upgrade case and the clean
install case.

I actually ended up adding the offline ID generation to the Java code (at the
risk of delaying this CL even more...)

Please take another look.

Thanks!
--brendan

[Dmitry Titov, 2016-02-24 01:26:18]

This is ok, it's better to iterate on it before it lands! Almost there.

This has 2 areas that can be improved, relatively simply:

1. It generated offline_id in 2 places (OfflinePageItemFromEntry on C++ and on
SavePage on Java side) - differently. We should have one single place that vends
offline_ids, I htink on C++ side. Can be trivially implemented for now via
base::RandUint64().

2. We probably should upgrade DB in a specific step in
OfflinePageMetadataStoreImpl::LoadDone() - by filling in offline_id and saving
entries back with new keys. This can replace the logic currently in
OfflinePageItemFromEntry.

[bburns, 2016-02-24 18:42:26]

Ok, I've done both of the suggestions.  please take another look.

Thanks!
--brendan

[Dmitry Titov, 2016-02-25 03:31:53]

Here we go! This can be the final round!

https://codereview.chromium.org/1694863003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1694863003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:284:
callback.onSavePageDone(SavePageResult.CONTENT_UNAVAILABLE, null, 0);
Can use INVALID_OFFLINE_ID defined at the top of the file.

https://codereview.chromium.org/1694863003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:308:
nativeSavePage(mNativeOfflinePageBridge, callbackWrapper, webContents,
offlineId, namespace,
Lets just remove offlineId parameter here since it is not needed. The native
method is only used by this place...

https://codereview.chromium.org/1694863003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:312:
private long calculateOfflineId(String namespace, String clientId, String url,
long timestamp) {
This can be removed now.

https://codereview.chromium.org/1694863003/diff/140001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (right):

https://codereview.chromium.org/1694863003/diff/140001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.cc:199:
offline_pages::ClientId cid;
client_id (to avoid abbreviation and also confusion with Content Id which is
used in Chrome as well...

https://codereview.chromium.org/1694863003/diff/140001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
(right):

https://codereview.chromium.org/1694863003/diff/140001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:131:
offline_pages::ClientId cid;
client_id would be a bit better

https://codereview.chromium.org/1694863003/diff/140001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_impl.cc (right):

https://codereview.chromium.org/1694863003/diff/140001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_impl.cc:48:
item_proto->set_deprecated_bookmark_id(item.offline_id);
Do we need this now? Looks like not.

https://codereview.chromium.org/1694863003/diff/140001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_impl.cc:104: // Legacy
storage.
Lets move this client_id initialization to the piece of code in LoadDone() whcih
updates offline_id. It will be the DB-upgrading logic, with update on disk, in
one place.

https://codereview.chromium.org/1694863003/diff/140001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_impl.cc:175: if
(!entry.has_offline_id()) {
Lets add a comment in front of this, telling it's an DB entry version upgrade...
It's unclear when we can remove this code, probably not very soon.

https://codereview.chromium.org/1694863003/diff/140001/components/offline_pag...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1694863003/diff/140001/components/offline_pag...
components/offline_pages/offline_page_model.cc:175: if (offline_id ==
INVALID_OFFLINE_ID) {
If all offline_ids are generated here, there is no need to check for
INVALID_OFFLINE_ID, or even pass offline_id into this method. Why not remove the
parameter and just generate the offline_id here always? It actually can be
generated in OnCreateArchiveDone or even later in the chain of saving the
offline page, since the value is only needed before writing a protobuf into the
storage.
This CL is already at the point when it is easy to remove this parameter from
SavePage's signature.

https://codereview.chromium.org/1694863003/diff/140001/components/offline_pag...
File components/offline_pages/offline_page_model_unittest.cc (right):

https://codereview.chromium.org/1694863003/diff/140001/components/offline_pag...
components/offline_pages/offline_page_model_unittest.cc:242: url, offline_id,
std::move(archiver),
I'm not sure how it compiles... SavePage now takes ClientId as well, and it is
not passed here... 
BTW, by passing existing bookmarkId as part of ClientId, you would get rid of
the last place that needs offline_id to be passed into
OfflinePageModel::SavePage, and thus simply remove the offline_id parameter for
that method. Then this CL will be more complete.

[bburns, 2016-02-26 00:14:36]

Comments addressed, please take another look.

Thanks!
--brendan

https://codereview.chromium.org/1694863003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1694863003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:284:
callback.onSavePageDone(SavePageResult.CONTENT_UNAVAILABLE, null, 0);
On 2016/02/25 03:31:52, Dmitry Titov wrote:
> Can use INVALID_OFFLINE_ID defined at the top of the file.

Done.

https://codereview.chromium.org/1694863003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:284:
callback.onSavePageDone(SavePageResult.CONTENT_UNAVAILABLE, null, 0);
On 2016/02/25 03:31:52, Dmitry Titov wrote:
> Can use INVALID_OFFLINE_ID defined at the top of the file.

Done.

https://codereview.chromium.org/1694863003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:308:
nativeSavePage(mNativeOfflinePageBridge, callbackWrapper, webContents,
offlineId, namespace,
On 2016/02/25 03:31:52, Dmitry Titov wrote:
> Lets just remove offlineId parameter here since it is not needed. The native
> method is only used by this place...

Done.

https://codereview.chromium.org/1694863003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:312:
private long calculateOfflineId(String namespace, String clientId, String url,
long timestamp) {
On 2016/02/25 03:31:52, Dmitry Titov wrote:
> This can be removed now.

Done.

https://codereview.chromium.org/1694863003/diff/140001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (right):

https://codereview.chromium.org/1694863003/diff/140001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.cc:199:
offline_pages::ClientId cid;
On 2016/02/25 03:31:52, Dmitry Titov wrote:
> client_id (to avoid abbreviation and also confusion with Content Id which is
> used in Chrome as well...

Done.

https://codereview.chromium.org/1694863003/diff/140001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
(right):

https://codereview.chromium.org/1694863003/diff/140001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:131:
offline_pages::ClientId cid;
On 2016/02/25 03:31:52, Dmitry Titov wrote:
> client_id would be a bit better 

Done.

https://codereview.chromium.org/1694863003/diff/140001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_impl.cc (right):

https://codereview.chromium.org/1694863003/diff/140001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_impl.cc:48:
item_proto->set_deprecated_bookmark_id(item.offline_id);
On 2016/02/25 03:31:52, Dmitry Titov wrote:
> Do we need this now? Looks like not.

Done.

https://codereview.chromium.org/1694863003/diff/140001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_impl.cc:104: // Legacy
storage.
On 2016/02/25 03:31:52, Dmitry Titov wrote:
> Lets move this client_id initialization to the piece of code in LoadDone()
whcih
> updates offline_id. It will be the DB-upgrading logic, with update on disk, in
> one place. 

Done.

https://codereview.chromium.org/1694863003/diff/140001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_impl.cc:175: if
(!entry.has_offline_id()) {
On 2016/02/25 03:31:52, Dmitry Titov wrote:
> Lets add a comment in front of this, telling it's an DB entry version
upgrade...
> It's unclear when we can remove this code, probably not very soon.

Done.

https://codereview.chromium.org/1694863003/diff/140001/components/offline_pag...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1694863003/diff/140001/components/offline_pag...
components/offline_pages/offline_page_model.cc:175: if (offline_id ==
INVALID_OFFLINE_ID) {
On 2016/02/25 03:31:52, Dmitry Titov wrote:
> If all offline_ids are generated here, there is no need to check for
> INVALID_OFFLINE_ID, or even pass offline_id into this method. Why not remove
the
> parameter and just generate the offline_id here always? It actually can be
> generated in OnCreateArchiveDone or even later in the chain of saving the
> offline page, since the value is only needed before writing a protobuf into
the
> storage.
> This CL is already at the point when it is easy to remove this parameter from
> SavePage's signature.

Done.

https://codereview.chromium.org/1694863003/diff/140001/components/offline_pag...
File components/offline_pages/offline_page_model_unittest.cc (right):

https://codereview.chromium.org/1694863003/diff/140001/components/offline_pag...
components/offline_pages/offline_page_model_unittest.cc:242: url, offline_id,
std::move(archiver),
On 2016/02/25 03:31:52, Dmitry Titov wrote:
> I'm not sure how it compiles... SavePage now takes ClientId as well, and it is
> not passed here... 
> BTW, by passing existing bookmarkId as part of ClientId, you would get rid of
> the last place that needs offline_id to be passed into
> OfflinePageModel::SavePage, and thus simply remove the offline_id parameter
for
> that method. Then this CL will be more complete.

Done (I hadn't compiled tests yet until we got closure on the other stuff...)

[Dmitry Titov, 2016-02-26 01:25:11]

LGTM

You need lgtm from both fgorski and dewittj as well before landing (all
reviewers w/o specified role must lgtm).

[Dmitry Titov, 2016-02-26 01:27:44]

One tiny nit (I think it's a stray keystroke):

https://codereview.chromium.org/1694863003/diff/160001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_impl.h (right):

https://codereview.chromium.org/1694863003/diff/160001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_impl.h:28: // Implements
OfflinePageMetadataStore using leveldb_proto::Protoatabase
Protoatabase -> back to ProtoDatabase

[bburns, 2016-02-26 17:26:39]

typo fixed.

fgorski, dewittj, I can haz lgtmz?

Thanks!
--brendan

https://codereview.chromium.org/1694863003/diff/160001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_impl.h (right):

https://codereview.chromium.org/1694863003/diff/160001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_impl.h:28: // Implements
OfflinePageMetadataStore using leveldb_proto::Protoatabase
On 2016/02/26 01:27:43, Dmitry Titov wrote:
> Protoatabase -> back to ProtoDatabase

Done.

[dewittj, 2016-02-26 18:42:56]

https://codereview.chromium.org/1694863003/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1694863003/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:232:
public Set<Long> getOfflineIdsForClientId(String clientIdNamespace, String
clientId) {
This can be private, so let's make it so for now.

https://codereview.chromium.org/1694863003/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:256:
Long offlineId = ids.iterator().next();
I think that bookmarks code prevents more than one offline ID per bookmark ID
but we should have at least a TODO to handle this a little better by using e.g.
the most recent of all the offline pages in the set.

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_utils.cc (right):

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_utils.cc:7: #include <cstdlib>
I don't think you use this anymore.

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_impl.cc (right):

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_impl.cc:177:
base::Int64ToString(entry.deprecated_bookmark_id());
I don't think this should be mandatory to land this code, but if you want it
could be cleaner to make a single routine that lives in the bookmarks code that
knows how to do bookmark_id -> client_id.id conversion.  That'd make it clearer
that this is bookmarks-specific and that all these int64tostring calls are for
the same purpose.

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/offline_page_model.cc:778: int64_t result =
base::RandUint64();
I think this can result in a negative |result| since it is generating a random
uint and stuffing it into an int.  Do we want negative offline IDs?

Perhaps 
base::RandGenerator(std::numeric_limits<int64_t>::max()) + 1 would be better so
you can avoid the loop and the negative integers?

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
File components/offline_pages/offline_page_model_unittest.cc (right):

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/offline_page_model_unittest.cc:941: // TODO(bburns):
Add a unit test for returning multiple results for a
Why is this a TODO and not just a test now?  There should be at least one test
for the GetOfflineIdsForClientId public API method.

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
File components/offline_pages/proto/offline_pages.proto (right):

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/proto/offline_pages.proto:19: // Bookmark ID of the
offline page.
nit: Change this comment to refer to legacy behavior rather than seemingly
current behavior.

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/proto/offline_pages.proto:22: // Offline ID of the
page.  Offline ID has replaced Bookmark ID as the primary
Primary key is kinda undefined in this context.  Perhaps it's just better to say
that every OfflinePageEntry has a unique offline_id?

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/proto/offline_pages.proto:59: optional string client_id
= 12;
Please document that this is provided by consumers of offline pages.

[fgorski, 2016-02-26 19:29:22]

lgtm

https://codereview.chromium.org/1694863003/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1694863003/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:357:
long[] ids = new long[idList.size()];
Will this work? (Not sure if type conversions would kick in appropriately.)

long[] ids = idList.toArray(new long[idList.size()]);

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (right):

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.cc:143: const
JavaParamRef<jstring>& j_client_id_namespace,
OH, I see you already did it here. Acknowledged ;)

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_bridge.h (right):

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.h:49: const
base::android::JavaParamRef<jstring>& client_id_namespace,
nit: j_client_id_namespace

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.h:50: const
base::android::JavaParamRef<jstring>& client_id);
nit: ditto

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.h:66: const
base::android::JavaParamRef<jstring>& client_id_namespace,
nit: ditto

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.h:67: const
base::android::JavaParamRef<jstring>& client_id);
nit: ditto

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.h:82: const
base::android::JavaParamRef<jlongArray>& offline_ids_array);
nit: ditto.


Notice I skipped the jlongs, as they are usable in the C++ code.

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_impl.h (right):

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_impl.h:72: // TODO: Remove
this eventually.
TODO(bburns): ?
Also this is part of an upgrade path, I don't know if we want to remove it ever.

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/offline_page_model.cc:331: if
(offline_pages[i].client_id.name_space == cid.name_space &&
Use client_id equals operator defined here:
https://codereview.chromium.org/1694863003/diff/200001/components/offline_pag...
line 21

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/offline_page_model.cc:778: int64_t result =
base::RandUint64();
On 2016/02/26 18:42:56, dewittj wrote:
> I think this can result in a negative |result| since it is generating a random
> uint and stuffing it into an int.  Do we want negative offline IDs?
> 
> Perhaps 
> base::RandGenerator(std::numeric_limits<int64_t>::max()) + 1 would be better
so
> you can avoid the loop and the negative integers?

base::RandGenerator(std::numeric_limits<int64_t>::max() - 1) + 1 perhaps? :)

On the other hand, why do we care about not using negative numbers here?

[dewittj, 2016-02-26 19:39:26]

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/offline_page_model.cc:778: int64_t result =
base::RandUint64();
On 2016/02/26 19:29:22, fgorski wrote:
> On 2016/02/26 18:42:56, dewittj wrote:
> > I think this can result in a negative |result| since it is generating a
random
> > uint and stuffing it into an int.  Do we want negative offline IDs?
> > 
> > Perhaps 
> > base::RandGenerator(std::numeric_limits<int64_t>::max()) + 1 would be better
> so
> > you can avoid the loop and the negative integers?
> 
> base::RandGenerator(std::numeric_limits<int64_t>::max() - 1) + 1 perhaps? :)
> 
> On the other hand, why do we care about not using negative numbers here?

No, RandGenerator generates numbers from [0, range), so we can add 1 to the
result which will be exactly max().

I don't have a reason, other than the completely emotional feeling that negative
IDs are weird.

[bburns, 2016-02-27 00:48:20]

comments addressed, please re-check.

Thanks!
--brendan

https://codereview.chromium.org/1694863003/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1694863003/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:256:
Long offlineId = ids.iterator().next();
On 2016/02/26 18:42:56, dewittj wrote:
> I think that bookmarks code prevents more than one offline ID per bookmark ID
> but we should have at least a TODO to handle this a little better by using
e.g.
> the most recent of all the offline pages in the set.

TODO'd

https://codereview.chromium.org/1694863003/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:357:
long[] ids = new long[idList.size()];
On 2016/02/26 19:29:21, fgorski wrote:
> Will this work? (Not sure if type conversions would kick in appropriately.)
> 
> long[] ids = idList.toArray(new long[idList.size()]);

I tried, sadly the answer is no.  Autoboxing doesn't kick in far enough and it
can't figure out that it can autobox Long[] into long[].

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (right):

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.cc:143: const
JavaParamRef<jstring>& j_client_id_namespace,
On 2016/02/26 19:29:22, fgorski wrote:
> OH, I see you already did it here. Acknowledged ;)

Acknowledged.

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_bridge.h (right):

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.h:49: const
base::android::JavaParamRef<jstring>& client_id_namespace,
On 2016/02/26 19:29:22, fgorski wrote:
> nit: j_client_id_namespace

Done.

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.h:50: const
base::android::JavaParamRef<jstring>& client_id);
On 2016/02/26 19:29:22, fgorski wrote:
> nit: ditto

Done.

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.h:66: const
base::android::JavaParamRef<jstring>& client_id_namespace,
On 2016/02/26 19:29:22, fgorski wrote:
> nit: ditto

Done.

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.h:67: const
base::android::JavaParamRef<jstring>& client_id);
On 2016/02/26 19:29:22, fgorski wrote:
> nit: ditto

Done.

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.h:82: const
base::android::JavaParamRef<jlongArray>& offline_ids_array);
On 2016/02/26 19:29:22, fgorski wrote:
> nit: ditto.
> 
> 
> Notice I skipped the jlongs, as they are usable in the C++ code.

Done.

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_utils.cc (right):

https://codereview.chromium.org/1694863003/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_utils.cc:7: #include <cstdlib>
On 2016/02/26 18:42:56, dewittj wrote:
> I don't think you use this anymore.

Done.

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_impl.cc (right):

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_impl.cc:177:
base::Int64ToString(entry.deprecated_bookmark_id());
On 2016/02/26 18:42:56, dewittj wrote:
> I don't think this should be mandatory to land this code, but if you want it
> could be cleaner to make a single routine that lives in the bookmarks code
that
> knows how to do bookmark_id -> client_id.id conversion.  That'd make it
clearer
> that this is bookmarks-specific and that all these int64tostring calls are for
> the same purpose.

hrm, I'm not sure that belongs in bookmark code, given that this is a historical
accident on our part, not the bookmark folks.  When we refactor their code to
talk to the offline pages api w/o bookmark ids, maybe we'll add it.

I did simplify this code so that it removed one Int64ToString call.

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_impl.h (right):

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_impl.h:72: // TODO: Remove
this eventually.
On 2016/02/26 19:29:22, fgorski wrote:
> TODO(bburns): ?
> Also this is part of an upgrade path, I don't know if we want to remove it
ever.

Done.  Softened the comment.  Seems like its cruft we'll want to remove in
_sometime_ (e.g. maybe years...)

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/offline_page_model.cc:331: if
(offline_pages[i].client_id.name_space == cid.name_space &&
On 2016/02/26 19:29:22, fgorski wrote:
> Use client_id equals operator defined here:
>
https://codereview.chromium.org/1694863003/diff/200001/components/offline_pag...
> line 21

Done.

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/offline_page_model.cc:778: int64_t result =
base::RandUint64();
On 2016/02/26 19:39:25, dewittj wrote:
> On 2016/02/26 19:29:22, fgorski wrote:
> > On 2016/02/26 18:42:56, dewittj wrote:
> > > I think this can result in a negative |result| since it is generating a
> random
> > > uint and stuffing it into an int.  Do we want negative offline IDs?
> > > 
> > > Perhaps 
> > > base::RandGenerator(std::numeric_limits<int64_t>::max()) + 1 would be
better
> > so
> > > you can avoid the loop and the negative integers?
> > 
> > base::RandGenerator(std::numeric_limits<int64_t>::max() - 1) + 1 perhaps? :)
> > 
> > On the other hand, why do we care about not using negative numbers here?
> 
> No, RandGenerator generates numbers from [0, range), so we can add 1 to the
> result which will be exactly max().
> 
> I don't have a reason, other than the completely emotional feeling that
negative
> IDs are weird.

Done.

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
File components/offline_pages/offline_page_model_unittest.cc (right):

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/offline_page_model_unittest.cc:941: // TODO(bburns):
Add a unit test for returning multiple results for a
On 2016/02/26 18:42:56, dewittj wrote:
> Why is this a TODO and not just a test now?  There should be at least one test
> for the GetOfflineIdsForClientId public API method.

Test added.

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
File components/offline_pages/proto/offline_pages.proto (right):

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/proto/offline_pages.proto:19: // Bookmark ID of the
offline page.
On 2016/02/26 18:42:56, dewittj wrote:
> nit: Change this comment to refer to legacy behavior rather than seemingly
> current behavior.

Done.

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/proto/offline_pages.proto:22: // Offline ID of the
page.  Offline ID has replaced Bookmark ID as the primary
On 2016/02/26 18:42:56, dewittj wrote:
> Primary key is kinda undefined in this context.  Perhaps it's just better to
say
> that every OfflinePageEntry has a unique offline_id?

Done.

https://codereview.chromium.org/1694863003/diff/180001/components/offline_pag...
components/offline_pages/proto/offline_pages.proto:59: optional string client_id
= 12;
On 2016/02/26 18:42:56, dewittj wrote:
> Please document that this is provided by consumers of offline pages.

Done.

[dewittj, 2016-02-27 01:06:39]

lgtm

I'm a little wary of using a randint as a unique id, but based on my
back-of-the-envelope birthday attack calculations, a user with 10000 offline
pages would have a 1e-11 chance of having a hash collision.  This shouldn't be a
problem until we get more people using offline pages.

[bburns, 2016-03-01 20:55:35]

The CQ bit was checked by bburns@chromium.org

[bburns, 2016-03-01 20:55:35]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org,
fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1694863003/#ps220001
(title: "address comments.")

[commit-bot: I haz the power, 2016-03-01 20:56:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1694863003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1694863003/220001

[commit-bot: I haz the power, 2016-03-01 21:03:26]

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

[commit-bot: I haz the power, 2016-03-01 21:03:27]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bburns, 2016-03-03 19:41:27]

The CQ bit was checked by bburns@chromium.org

[bburns, 2016-03-03 19:41:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
fgorski@chromium.org, dimich@chromium.org, dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/1694863003/#ps380001
(title: "address changes")

[commit-bot: I haz the power, 2016-03-03 19:42:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1694863003/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1694863003/380001

[commit-bot: I haz the power, 2016-03-03 19:53:52]

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

[commit-bot: I haz the power, 2016-03-03 19:53:53]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[bburns, 2016-03-03 20:24:00]

bburns@chromium.org changed reviewers:
+ ianwen@chromium.org

[bburns, 2016-03-03 20:24:01]

ianwen: for OWNERS on bookmarks

[Ian Wen, 2016-03-03 22:38:23]

lgtm.

[bburns, 2016-03-04 00:26:37]

The CQ bit was checked by bburns@chromium.org

[commit-bot: I haz the power, 2016-03-04 00:27:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1694863003/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1694863003/380001

[commit-bot: I haz the power, 2016-03-04 00:34:25]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-03-04 00:36:40]

Description was changed from

==========
Refactor the offline page storage to include client namespace and id.

BUG=
==========

to

==========
Refactor the offline page storage to include client namespace and id.

BUG=

Committed: https://crrev.com/16d153334496e8eb7aeeb7352515a6dbe97d95d3
Cr-Commit-Position: refs/heads/master@{#379153}
==========

[commit-bot: I haz the power, 2016-03-04 00:36:41]

Patchset 20 (id:??) landed as
https://crrev.com/16d153334496e8eb7aeeb7352515a6dbe97d95d3
Cr-Commit-Position: refs/heads/master@{#379153}

[jianli, 2016-03-05 00:58:56]

jianli@chromium.org changed reviewers:
+ jianli@chromium.org

[jianli, 2016-03-05 00:58:57]

https://codereview.chromium.org/1694863003/diff/380001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_impl.cc (right):

https://codereview.chromium.org/1694863003/diff/380001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_impl.cc:183:
keys_to_remove->push_back(item.client_id.id);
What if new auto-generated offline id happens to be same as old bookmark id,
though this should be rare? We could end up with deleting the entry instead.

https://codereview.chromium.org/1694863003/diff/380001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_impl.cc:307: // If the
update failed, log and keep going.  We'll try to
We're having some sort of inconsistency between store and cache and this could
potentially cause problem.

For example, if we fail to update an entry but it still lives in the cache. If
we change a property for this item, we're going to write back to the store.
Assume the write succeeds. Next time when store is loaded, we still detect the
old entry and try to create a new one for it.