Reading List create protobuf store

Reading List create protobuf store
BUG=

[Olivier, 2016-09-27 15:52:12]

olivierrobin@chromium.org changed reviewers:
+ gambard@chromium.org, jif@chromium.org, zea@chromium.org

[Olivier, 2016-09-27 15:58:17]

On 2016/09/27 15:52:13, Olivier Robin wrote:

I will also update unittests

[jif-google, 2016-09-27 16:39:02]

jif@google.com changed reviewers:
+ jif@google.com

[jif-google, 2016-09-27 16:39:03]

first pass

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_entry.cc (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.cc:15: static
base::TickClock* clock = new base::DefaultTickClock();
Wouldn't that work:

static base::DefaultTickClock clock;
return &clock;

?

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.cc:225:
std::unique_ptr<ReadingListEntry> entry(base::MakeUnique<ReadingListEntry>(
this would be slightly more readable:

std::unique_ptr<ReadingListEntry> entry = base::MakeUnique<ReadingListEntry>(...

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.cc:238:
pb_entry->mutable_entry()->set_entry_id(URL().spec());
How about adding a comment such as "there can only be one entry per URL" ?

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_entry.h (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:66: // The last update time
of the entry. Thes value may be used to sort the
s/Thes/These/

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:68: int64_t UpdateTime()
const;
use base::time instead of int64_t ?
If not, then put in the comment which unit the time is in and the reference
point.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:73: // Converts a
ReadingListEntry to the protobuf format.
How about:
// Returns a protobuf encoding the content of this ReadingListEntry.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:105: int64_t
creation_time_us_;
base::time ?

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:111: typedef
std::vector<ReadingListEntry> ReadingListEntries;
Use "using" instead of "typedef". It's better, and it's in the style guide:

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/8dOAMzgR4ao

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:111: typedef
std::vector<ReadingListEntry> ReadingListEntries;
move ReadingListEntries definition to where it's used, i.e.
reading_list_model_impl.h ?

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_factory.cc (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_factory.cc:62:
std::unique_ptr<ReadingListDB> db(
std::unique_ptr<ReadingListDB> db =
base::MakeUnique<leveldb_proto::ProtoDatabaseImpl<sync_pb::ReadingListLocal>>(background_task_runner);
?

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_factory.cc:69: // 
std::unique_ptr<ReadingListModelStorage> storage(
remove

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:69: return
unread_size() && has_unseen_;
At first glance it seems weird that "return unread_size();" isn't enough and
that an additional boolean is needed.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_pref_names.cc (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_pref_names.cc:10: // Boolean which
if some reading list entries have never been seen on this
How about:
// Key for a boolean storing whether some reading list entries have never been
seen on this device. Not synced.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_store.h (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.h:18: typedef
leveldb_proto::ProtoDatabase<sync_pb::ReadingListLocal> ReadingListDB;
s/typedef/using/

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_store.mm (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.mm:90: for
(sync_pb::ReadingListLocal& pb_entry : *entries) {
might as well make |pb_entry| be const.

[gambard, 2016-09-28 09:19:08]

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_entry.cc (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.cc:14: base::TickClock*
ReadingListTickClock() {
Do you need with? You are always using the default clock in the constructors,
use nullptr instead.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.cc:168:
std::unique_ptr<ReadingListEntry> ReadingListEntry::FromReadingListLocal(
Please add // static.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_entry.h (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:66: // The last update time
of the entry. Thes value may be used to sort the
On 2016/09/27 16:39:03, jif-google wrote:
> s/Thes/These/

I think it is s/Thes/This/

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:85: // Compares two entries
using update time, most recent first.
Maybe be a bit more explicit? Like // returns whether |lhs| is more recent than
|rhs|

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:105: int64_t
creation_time_us_;
what do the "us" part of the variable name mean? Can you add a comment?

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_pref_names.cc (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_pref_names.cc:10: // Boolean which
if some reading list entries have never been seen on this
Is it "never seen" or "in unread state"? Is this true if the user take a read
entry and mark it as unread?

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_store.h (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.h:20: class ReadingListStore
: public ReadingListModelStorage {
Comment here and for the methods?

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_store.mm (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.mm:29: return;
Is this return needed?

[Olivier, 2016-09-28 11:20:06]

Thanks. PTAL

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_entry.cc (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.cc:14: base::TickClock*
ReadingListTickClock() {
On 2016/09/28 09:19:08, gambard wrote:
> Do you need with? You are always using the default clock in the constructors,
> use nullptr instead.

This is needed for the serialization (not sure why)...

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.cc:15: static
base::TickClock* clock = new base::DefaultTickClock();
On 2016/09/27 16:39:03, jif-google wrote:
> Wouldn't that work:
> 
> static base::DefaultTickClock clock;
> return &clock;
> 
> ?

Done.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.cc:168:
std::unique_ptr<ReadingListEntry> ReadingListEntry::FromReadingListLocal(
On 2016/09/28 09:19:08, gambard wrote:
> Please add // static.

Done.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.cc:225:
std::unique_ptr<ReadingListEntry> entry(base::MakeUnique<ReadingListEntry>(
On 2016/09/27 16:39:03, jif-google wrote:
> this would be slightly more readable:
> 
> std::unique_ptr<ReadingListEntry> entry =
base::MakeUnique<ReadingListEntry>(...

Done.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.cc:238:
pb_entry->mutable_entry()->set_entry_id(URL().spec());
On 2016/09/27 16:39:03, jif-google wrote:
> How about adding a comment such as "there can only be one entry per URL" ?

Done.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_entry.h (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:66: // The last update time
of the entry. Thes value may be used to sort the
On 2016/09/28 09:19:08, gambard wrote:
> On 2016/09/27 16:39:03, jif-google wrote:
> > s/Thes/These/
> 
> I think it is s/Thes/This/

Done.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:68: int64_t UpdateTime()
const;
On 2016/09/27 16:39:03, jif-google wrote:
> use base::time instead of int64_t ?
> If not, then put in the comment which unit the time is in and the reference
> point.

I did use base::time, but as the value is used only for sorting and must be
converted to int64 each time it is read/written to database, I opted for int64.
Adding comment.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:73: // Converts a
ReadingListEntry to the protobuf format.
On 2016/09/27 16:39:03, jif-google wrote:
> How about:
> // Returns a protobuf encoding the content of this ReadingListEntry.

Done.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:85: // Compares two entries
using update time, most recent first.
On 2016/09/28 09:19:08, gambard wrote:
> Maybe be a bit more explicit? Like // returns whether |lhs| is more recent
than
> |rhs|

Done.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:105: int64_t
creation_time_us_;
On 2016/09/27 16:39:03, jif-google wrote:
> base::time ?

same comment as above.
This value is fundamentally used for storage and sorting the database.
Converting it each time to base::time seems useless to me (but I can do it if
you really want).

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:111: typedef
std::vector<ReadingListEntry> ReadingListEntries;
On 2016/09/27 16:39:03, jif-google wrote:
> move ReadingListEntries definition to where it's used, i.e.
> reading_list_model_impl.h ?

Done.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_factory.cc (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_factory.cc:62:
std::unique_ptr<ReadingListDB> db(
On 2016/09/27 16:39:03, jif-google wrote:
> std::unique_ptr<ReadingListDB> db =
>
base::MakeUnique<leveldb_proto::ProtoDatabaseImpl<sync_pb::ReadingListLocal>>(background_task_runner);
> ?

Done.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_factory.cc:69: // 
std::unique_ptr<ReadingListModelStorage> storage(
On 2016/09/27 16:39:03, jif-google wrote:
> remove

oops, done

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:69: return
unread_size() && has_unseen_;
On 2016/09/27 16:39:03, jif-google wrote:
> At first glance it seems weird that "return unread_size();" isn't enough and
> that an additional boolean is needed.

It is HasUnseenEntries, not HasUnreadEntries.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_pref_names.cc (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_pref_names.cc:10: // Boolean which
if some reading list entries have never been seen on this
On 2016/09/28 09:19:08, gambard wrote:
> Is it "never seen" or "in unread state"? Is this true if the user take a read
> entry and mark it as unread?

unseen means "never opened the view since entry was added".
I think the case you describe should not result in the an unseen state.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_store.mm (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.mm:29: return;
On 2016/09/28 09:19:08, gambard wrote:
> Is this return needed?

Done.

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.mm:90: for
(sync_pb::ReadingListLocal& pb_entry : *entries) {
On 2016/09/27 16:39:03, jif-google wrote:
> might as well make |pb_entry| be const.

Done.

[gambard, 2016-09-28 16:19:48]

Quick review as this made me crash, I will do a second pass later.

https://codereview.chromium.org/2369303002/diff/80001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2369303002/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:160:
storage_layer_->SaveEntry(entry, false);
You cannot do this. The entry has just been moved.

https://codereview.chromium.org/2369303002/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:186:
unread_->erase(result);
You moved result. Are you sure this will work?

[Olivier, 2016-09-28 17:03:37]

Thanks
PTAL

https://codereview.chromium.org/2369303002/diff/80001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2369303002/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:186:
unread_->erase(result);
On 2016/09/28 16:19:47, gambard wrote:
> You moved result. Are you sure this will work?

Done.

[Olivier, 2016-10-03 07:28:42]

olivierrobin@chromium.org changed reviewers:
+ pavely@chromium.org

[Olivier, 2016-10-03 07:28:43]

+pavel for USS integration

[jif, 2016-10-03 07:50:03]

lgtm

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2369303002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:69: return
unread_size() && has_unseen_;
On 2016/09/28 11:20:06, Olivier Robin wrote:
> On 2016/09/27 16:39:03, jif-google wrote:
> > At first glance it seems weird that "return unread_size();" isn't enough and
> > that an additional boolean is needed.
> 
> It is HasUnseenEntries, not HasUnreadEntries.

Acknowledged.

[gambard, 2016-10-03 08:08:43]

lgtm with one comment

https://codereview.chromium.org/2369303002/diff/140001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_entry.cc (right):

https://codereview.chromium.org/2369303002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_entry.cc:14: base::TickClock*
ReadingListTickClock() {
I really think it should be ok to pass nullptr instead of the default clock. You
can check the implementation.

[Olivier, 2016-10-06 09:30:24]

olivierrobin@chromium.org changed reviewers:
+ noyau@chromium.org, sdefresne@chromium.org

[Olivier, 2016-10-06 09:30:25]

+noyau for RL
+sdefresne for gn and prefs
+pavely for sync and USS compatibility

[noyau (Ping after 24h), 2016-10-06 09:56:20]

https://codereview.chromium.org/2369303002/diff/200001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_entry.h (right):

https://codereview.chromium.org/2369303002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_entry.h:42:
std::unique_ptr<net::BackoffEntry> backoff);
If it is only used by the protobuf decoding static method, this can probably be
a private method. If it is used by anything else, it probably should not be :)

https://codereview.chromium.org/2369303002/diff/200001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_store.cc (right):

https://codereview.chromium.org/2369303002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:79:
pending_keys_to_save_->push_back(std::make_pair(local_key, *pb_entry));
What's the behavior if you push an entry where the URL already exists? Aka, you
add the same URL from two devices and sync?

[Olivier, 2016-10-06 11:58:16]

https://codereview.chromium.org/2369303002/diff/200001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_entry.h (right):

https://codereview.chromium.org/2369303002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_entry.h:42:
std::unique_ptr<net::BackoffEntry> backoff);
On 2016/10/06 09:56:19, noyau wrote:
> If it is only used by the protobuf decoding static method, this can probably
be
> a private method. If it is used by anything else, it probably should not be :)

Done.

https://codereview.chromium.org/2369303002/diff/200001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_store.cc (right):

https://codereview.chromium.org/2369303002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:79:
pending_keys_to_save_->push_back(std::make_pair(local_key, *pb_entry));
We don't resolve with the store but with the model.
This will be done when adding sync.
The last entry added will be the one saved.

Note: This will change when the backend becomes a ModelTypeStore.

https://codereview.chromium.org/2369303002/diff/220001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_store.cc (right):

https://codereview.chromium.org/2369303002/diff/220001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:21:
database_->Init("ReadingList", database_dir,
Convert database to ModelTypeStore.

https://codereview.chromium.org/2369303002/diff/220001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_store.h (right):

https://codereview.chromium.org/2369303002/diff/220001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:37:
std::unique_ptr<ReadingListDB> database_;
This will become a ModelTypeStore with USS

[noyau (Ping after 24h), 2016-10-06 13:01:25]

lgtm

[sdefresne, 2016-10-06 15:11:24]

not lgtm as is

https://codereview.chromium.org/2369303002/diff/240001/ios/chrome/BUILD.gn
File ios/chrome/BUILD.gn (right):

https://codereview.chromium.org/2369303002/diff/240001/ios/chrome/BUILD.gn#ne...
ios/chrome/BUILD.gn:142: include_dirs = [
"$root_gen_dir/components/sync/protocol" ]
You should never have to do that. I think you are missing a dependency (or the
target that generate this should use public_configs instead of configs).

I think ios/chrome/browser/reading_list/proto probably need to also define a
config with this include_dirs, and add it to its public_configs (see my comment
in the other CL).

Even then, I think it strange that you use
"$root_gen_dir/components/sync/protocol", it should be ""$root_gen_dir" and then
you should prefix your paths with "components/sync/protocol".

https://codereview.chromium.org/2369303002/diff/240001/ios/chrome/browser/BUI...
File ios/chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2369303002/diff/240001/ios/chrome/browser/BUI...
ios/chrome/browser/BUILD.gn:749: include_dirs = [
"$root_gen_dir/components/sync/protocol" ]
You should never have to do that. I think you are missing a dependency (or the
target that generate this should use public_configs instead of configs).

https://codereview.chromium.org/2369303002/diff/240001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_pref_names.cc (right):

https://codereview.chromium.org/2369303002/diff/240001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_pref_names.cc:10: // Boolean which
if some reading list entries have never been seen on this
I don't understand this comment. Did you forget some words?

[Olivier, 2016-10-07 07:29:19]

Patchset #14 (id:260001) has been deleted

[Olivier, 2016-10-07 07:30:13]

Thanks!
PTAL

https://codereview.chromium.org/2369303002/diff/240001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_pref_names.cc (right):

https://codereview.chromium.org/2369303002/diff/240001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_pref_names.cc:10: // Boolean which
if some reading list entries have never been seen on this
On 2016/10/06 15:11:24, sdefresne wrote:
> I don't understand this comment. Did you forget some words?

Done.

[sdefresne, 2016-10-07 09:44:30]

Looked in more details, and there are a few design decision I don't really
understand, in particular the use of std::unique_ptr<std::vector<T>>. Can you
reconsider or explain why this is better than just using std::move() on
std::vector<T>?

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_entry.cc (right):

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_entry.cc:15: static
base::DefaultTickClock clock;
This is not thread-safe when building Chromium code (static initialization is
supposed to be thread-safe, but some of the platform we target have buggy
compiler, and thus we pass a flag to the compiler to disable the thread-safety
of static initialization to have the same behaviour on all platforms).

Do you really need to pass a non-null base::TickClock* to the net::BackoffEntry?
As far as I can tell, if you pass null, it will just use the system clock. If
this was already discussed in previous review comments, then maybe you need to
add a comment to this function explaining why this is required.

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_entry.cc:245:
DCHECK(creation_time_us_ > 100000000);
Why this DCHECK? What does it mean. What does 100000000 correspond to?

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_model.cc (right):

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model.cc:16:
DCHECK_CURRENTLY_ON(web::WebThread::UI);
nit: this would probably be simpler (especially for test) to inherit from
base::NonThreadSafe and to do DCHECK(CalledOnValidThread());

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_model_factory.cc (right):

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_factory.cc:50: // Register a
not syncable pref.
nit: this comment has no value in my opinion (if you want to put emphasis on the
fact that the pref is not syncable, pass "NO_REGISTRATION_FLAGS" instead of "0"
for the flag).

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_factory.cc:63:
std::unique_ptr<ReadingListDB> db = base::MakeUnique<
nit: auto db = base::MakeUnique<...>(...) is also correct.

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_factory.cc:73: new
ReadingListModelImpl(std::move(store),
Use base::MakeUnique<> as the constructor is public.

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_factory.cc:75: return
std::move(reading_list_model);
Do not use "return std::move(...)" with a std::unique_ptr<> as it prevent the
compiler from making some optimisation. Instead just do:

return base::MakeUnique<ReadingListModelImpl>(
    std::move(store), chrome_browser_state->GetPrefs());

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_model_impl.h (right):

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.h:64: virtual void
ModelLoaded(std::unique_ptr<ReadingListEntries> unread,
Why use std::unique_ptr<std::vector<ReadingListEntries>>? std::vector<> is
movable, so if you really want to move the value, pass them by value and tell
client code to std::move() them.

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_store.h (right):

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:17: using EntryVector =
std::vector<reading_list::ReadingListLocal>;
Move below the "private:" line (as it is not used before that point).

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:34: void
OnDatabaseLoad(bool success, std::unique_ptr<EntryVector> entries);
Why use a std::unique_ptr<std::vector<reading_list::ReadingListLocal>>? Why not
take a std::vector directly (std::vector<> is movable).

[jif-google, 2016-10-07 09:51:34]

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_model_impl.h (right):

https://codereview.chromium.org/2369303002/diff/280001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.h:64: virtual void
ModelLoaded(std::unique_ptr<ReadingListEntries> unread,
On 2016/10/07 09:44:30, sdefresne wrote:
> Why use std::unique_ptr<std::vector<ReadingListEntries>>? std::vector<> is
> movable, so if you really want to move the value, pass them by value and tell
> client code to std::move() them.

1/ The fact that ReadingListEntries is movable is an implementation detail.
2/ Passing by value makes it easy to accidentally copy the object.

[pavely, 2016-10-09 04:13:59]

My main comment is about use of ProtoDatabase as a storage.

In USS we want to store data records and sync metadata in the same database and
update them atomically. Sync metadata has different structure from data records
and cannot be stored in the same record with data, it needs to be stored in
separate records.

ProtoDatabase stores homogeneous records, it is not designed to store records
with different protobufs for values. Because of the above requirements
ProtoDatabase cannot be used to store synced data in USS based component.

We implemented ModelTypeStore that is also leveldb based. I think it will work
for Reading List scenarios. Could you take a look at it:
https://cs.chromium.org/chromium/src/components/sync/api/model_type_store.h

[olivierrobin, 2016-10-09 06:42:57]

On 2016/10/09 04:13:59, pavely wrote:
> My main comment is about use of ProtoDatabase as a storage.
> 
> In USS we want to store data records and sync metadata in the same database
and
> update them atomically. Sync metadata has different structure from data
records
> and cannot be stored in the same record with data, it needs to be stored in
> separate records.
> 
> ProtoDatabase stores homogeneous records, it is not designed to store records
> with different protobufs for values. Because of the above requirements
> ProtoDatabase cannot be used to store synced data in USS based component.
> 
> We implemented ModelTypeStore that is also leveldb based. I think it will work
> for Reading List scenarios. Could you take a look at it:
> https://cs.chromium.org/chromium/src/components/sync/api/model_type_store.h

Yes.
This CL was done before the USS change and I have a CL to use a ModelTypeStore
instead.
I kept them separated for review purpose, but the store in this CL will never be
used.

ModelTypeStore seems to work, but I am now working on merging data (and trying
to understand what to do exactly with metadata).
I will send you the next CL soon.

[Olivier, 2016-11-02 14:52:32]

Description was changed from

==========
Reading List create protobuf store
BUG=
==========

to

==========
Reading List create protobuf store
BUG=
==========