Add Store+Sync to reading list.

Add Store+Sync to reading list.

This CL creates a ModelTypeService for reading list.
This service will store persistent data for the Reading list model and sync
data between devices.

The previous store using NSUserDefaults will not be cleaned.
As it was only accessible from canary behind an experimental flag, no user
should be affected.

BUG=661668
Committed: https://crrev.com/faa080c524c6a80c2ac276359e29dec8cf7946c4
Committed: https://crrev.com/01318f7c035d2f17ddbfc987b27db1dd6e6e1625
Cr-Original-Commit-Position: refs/heads/master@{#432845}
Cr-Commit-Position: refs/heads/master@{#433162}

[Olivier, 2016-10-28 15:17:38]

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

[Olivier, 2016-10-28 15:17:39]

Hi pavely,

The CL is not ready for full review yet, but can you take a look at the
ModelTypeService ?

It is mainly the reading_list_store.cc file.
Let me know if there are big mistake in the usage (specially in the metadata and
the GetClientTag method, I dont exactly know what to retyrn there.

Thanks

[skym, 2016-10-28 16:30:55]

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

[skym, 2016-10-28 16:30:56]

The comment on the .h file is the important one. Maybe you and Pavel have
already talked about this, and there are reasons that force you to go this way.
But if possible, we should strive to store data and metadata somewhere which
allows us to guarantee consistency between data and metadata.

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_store.cc (right):

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:76: if
(!change_processor() || from_sync) {
I'm sorry. This has changed recently, the processor is now always non-null. Can
you do !change_processor()->IsTrackingMetadata() instead?

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:102: if
(!change_processor() || from_sync) {
Same as above.

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:163: return;
It looks like non of your initialization error cases call OnMetadataLoaded with
a set SyncError. This is likely a problem, but, I don't think you should worry
about it. We (sync team) are still a little bit hazy on the exact details and
requirements for how ModelTypeServices should behave on errors. I think we (sync
team) can come in here and tweak things once we have a really precise
understanding of what/when errors should be communicated.

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:171: // TODO(skym): How
bad is this scenario? We may be able to just give an
Why did I leave TODOs in DeviceInfoService aaah X_X

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:235: syncer::EntityDataMap
entity_data_map) {
Should there be a BeginTransaction() up here?

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:264:
last_update[entry.URL()] >= entry.UpdateTime()) {
What about the other direction?

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:276:
->TransferChanges(store_.get(), batch_.get());
This transfer change should get moved outside (and below) of the for loop. It
takes everything in the SimipleMetadataChangeList and moves it into the
WriteBatch. If you moved this down to after the change_processor()-Put(...)
call, it would be technically correct, but still kinda weird.

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:279: }
I think there should be a commit here.

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:362: return
entity_data.specifics.reading_list().url();
I'd call GetStorageKey(entity_data) here so you don't repeat yourself.

The value you're returning here is perfect. This method doesn't really make
sense for your use case, since you don't have clients out there with legacy
client tag hashes. But for model types that used to be on the Directory based
implementation of Sync, their client tags and storage keys are not the same.

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

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.h:121:
std::unique_ptr<syncer::ModelTypeStore> store_;
Hmm. This concerns me. It seems like |model_| and |store_| both have durable
storage. This is exactly the scenario we're trying to stop with USS.

We want both metadata and real data to be stored in the same persistent storage,
such that you can guarantee they are either atomically stored to disk, or
they're stored in a strict order such that crashing in the middle does not leave
us in a bad state. (On local change it should be metadata -> real data, on
remote change it should be real data -> metadata)

https://docs.google.com/document/d/1QvfB9XTonsnnJf1PiPuT0nwaGObql5G04C6ccw2LA...

I'd much prefer a solution with a custom MetadataChangeList implementation that
batches changes that are going to eventually make their way into
reading_list_model_storage_defaults.mm's |backend_| somehow.

Or, completely remove reading_list_model_storage_defaults.mm and use the
syncer::ModelTypeStore leveldb as your model's storage.

[pavely, 2016-11-01 06:14:10]

Thanks for digging through details of how to implement USS based type.

GetClientTag is correct, for new types without legacy clients it should just
return whatever value is used as primary key (URL in this case).

I left comment in ApplySuncChanges and Sky commented on MergeSyncData.

In general it is unfortunate that data is persisted in ReadingListStore, but in
memory representation is managed in ReadingListModelImpl. This causes awkward
interactions between the two where store calls model to save entry relying on
model to call back to persist it.  Also because of this interaction GetData
implementation is unintuitive.

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

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:306: }
metadata_change_list contains metadata updates and progress marker corresponding
to updates in entity_changes. This function should transfer them to batch_ and
commit all changes as part of the same transaction.

The same is true for MergeSyncData (see Sky's comments).

[skym, 2016-11-01 16:40:24]

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

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.h:121:
std::unique_ptr<syncer::ModelTypeStore> store_;
On 2016/10/28 16:30:56, skym wrote:
> Hmm. This concerns me. It seems like |model_| and |store_| both have durable
> storage. This is exactly the scenario we're trying to stop with USS.
> 
> We want both metadata and real data to be stored in the same persistent
storage,
> such that you can guarantee they are either atomically stored to disk, or
> they're stored in a strict order such that crashing in the middle does not
leave
> us in a bad state. (On local change it should be metadata -> real data, on
> remote change it should be real data -> metadata)
> 
>
https://docs.google.com/document/d/1QvfB9XTonsnnJf1PiPuT0nwaGObql5G04C6ccw2LA...
> 
> I'd much prefer a solution with a custom MetadataChangeList implementation
that
> batches changes that are going to eventually make their way into
> reading_list_model_storage_defaults.mm's |backend_| somehow.
> 
> Or, completely remove reading_list_model_storage_defaults.mm and use the
> syncer::ModelTypeStore leveldb as your model's storage.

Pavel has informed me that I was confused. You are in fact deleting
reading_list_model_storage_defaults.mm! Awesome. Taking another look at things.

[skym, 2016-11-01 18:27:26]

Mostly nits and comments to myself/sync team. I got a little bit side tracked
looking at reading_list_model_impl.cc, not sure if you wanted comments there
yet.

The only thing that really needs to change is you want a single transfer
metadata and commit at the very end of both MergeSyncData and ApplySyncChanges.

It's a tiny bit weird how the ReadingListStore receives changes, passes them to
the model, then gets called back to store to disk. I kind of wonder if it'd be
possible to split those two responsibilities into two separate classes. But I
can hardly fault you, the DeviceInfo approach is doing three jobs.

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_store.cc (right):

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:264:
last_update[entry.URL()] >= entry.UpdateTime()) {
On 2016/10/28 16:30:55, skym wrote:
> What about the other direction?

Disregard this, I was confused. You're always sending the sync entries to
|model_|, and it has the corresponding update time check and update.

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:321:
callback.Run(syncer::SyncError(), std::move(batch));
All these syncer::SyncError() calls are pretty useless here. I think we (sync
team) should probably remove them from the interface. And just let the model
types call inform the proc of errors always out of band.

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

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model.h:63: // TODO(659099): Remove
methods.
Does that mean you won't need to sort the elements by update time then?

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

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:114: bool& read)
const {
I think this is against Google C++ style,
https://google.github.io/styleguide/cppguide.html#Reference_Arguments , raw
pointer is preferred when modifying.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:160: bool
is_entry_read;
Can you rename to make it clear this is for the old/existing one?

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:179:
observer.ReadingListDidApplyChanges(this);
I don't understand what these various observer methods are, but it seems odd
that nothing gets sent to the observers when updating an existing element. Only
on new elements. If this is intentional, can you add a comment?

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:192: auto result =
std::find(read_->begin(), read_->end(), *existing_entry);
There's a lot of std::find calls in here. They're all O(n), right? Is the
assumption that the total number of elements should always be fairly small?

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:194:
read_->erase(result);
If |is_entry_read| == |read|, then updating the existing record would be more
efficient. Though keeping the code as simple as possible may be more valuable,
especially since the above find is already O(n).

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:204:
storage_layer_->SaveEntry(*entry, true, read);
Are the arguments |true| and |read| swapped here?

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:204:
storage_layer_->SaveEntry(*entry, true, read);
Also, kind of weird that the storage call is in the middle here. Is there a
specific reason? I feel like it'd make more sense to keep the |read_| and
|unread_| updating as close together as possible. Unless I'm missing something.

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

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:65: BeginTransaction();
Aaah, I finally understand why you have multiple transactions, you want to
ensure that you always have a batch_ object when you need it. I kinda wish there
was a more elegant solution out there but it seems fine to me as is.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:69:
std::unique_ptr<sync_pb::ReadingListSpecifics> pb_entry_sync =
I'd personally move this down to below the if(..) {.. return; } block.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:71: // Unref the URL
before making asynchronous call.
Is this really needed? I'm having a difficult time understanding why this would
be necessary.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:89:
metadata_change_list.get());
Max/Pavel, this use case of MetadataChangeList could use a pass through
implementation instead of one that holds things in memory! Would remove the
creation and transfer lines in any situation where we have known store_ and
batch_ objects. DeviceInfo has this use case as well.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:100: std::string local_key
= entry.URL().spec();
Same as above.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:258: for (int index = 0;
index < unread_count + read_count; index++) {
This is a lot of complexity to just iterate over all the entries model_ is
hanging onto. Though I don't have a better solution. :/

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:271:
std::unique_ptr<syncer::EntityData> entity_data(new syncer::EntityData());
auto and base::MakeUnique?

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:310: // Asynchronously
retrieve the corresponding sync data for |storage_keys|.
This implementation is not async.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:324: // Asynchronously
retrieve all of the local sync data.
This implementation is not async.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:329: for (int index = 0;
index < unread_count + read_count; index++) {
I feel like I've seen this logic before! :)

[Olivier, 2016-11-02 14:57:11]

Thanks for the feedback.
I am happy the general approach did not raise big issues.

Would you prefer the
ReadingListModel::SyncAddEntry(std::unique_ptr<ReadingListEntry> entry,bool
read)
to return the merged entry so that we can update the store directly from
MergeSyncData? This would avoid a store->model->store call.

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_store.cc (right):

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:76: if
(!change_processor() || from_sync) {
On 2016/10/28 16:30:56, skym wrote:
> I'm sorry. This has changed recently, the processor is now always non-null.
Can
> you do !change_processor()->IsTrackingMetadata() instead?

Done.

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:102: if
(!change_processor() || from_sync) {
On 2016/10/28 16:30:56, skym wrote:
> Same as above.

Done.

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:235: syncer::EntityDataMap
entity_data_map) {
On 2016/10/28 16:30:55, skym wrote:
> Should there be a BeginTransaction() up here?

It would not hurt but the BeginBatchUpdates do it on line 238.

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:276:
->TransferChanges(store_.get(), batch_.get());
On 2016/10/28 16:30:56, skym wrote:
> This transfer change should get moved outside (and below) of the for loop. It
> takes everything in the SimipleMetadataChangeList and moves it into the
> WriteBatch. If you moved this down to after the change_processor()-Put(...)
> call, it would be technically correct, but still kinda weird.

Done.

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:279: }
On 2016/10/28 16:30:55, skym wrote:
> I think there should be a commit here.

There is a hidden one when batch (renames to model_batch_updates) goes out of
scope.
Added one anyway to balance with the BeginTransaction.

https://codereview.chromium.org/2451843002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:362: return
entity_data.specifics.reading_list().url();
On 2016/10/28 16:30:56, skym wrote:
> I'd call GetStorageKey(entity_data) here so you don't repeat yourself.
> 
> The value you're returning here is perfect. This method doesn't really make
> sense for your use case, since you don't have clients out there with legacy
> client tag hashes. But for model types that used to be on the Directory based
> implementation of Sync, their client tags and storage keys are not the same.

If new model don't need it, should this method be optional? Should it be
NOTREACHED() instead of returning a value?

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

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model.h:63: // TODO(659099): Remove
methods.
On 2016/11/01 18:27:25, skym wrote:
> Does that mean you won't need to sort the elements by update time then?

No. We will always need to sort them.
But we are discussing changing this API to return a 
GetReadEntries returning all read entries, then alway access the entries by URL
(which is the key).

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

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:114: bool& read)
const {
On 2016/11/01 18:27:25, skym wrote:
> I think this is against Google C++ style,
> https://google.github.io/styleguide/cppguide.html#Reference_Arguments , raw
> pointer is preferred when modifying.

Done.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:160: bool
is_entry_read;
On 2016/11/01 18:27:25, skym wrote:
> Can you rename to make it clear this is for the old/existing one?

Done.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:179:
observer.ReadingListDidApplyChanges(this);
On 2016/11/01 18:27:25, skym wrote:
> I don't understand what these various observer methods are, but it seems odd
> that nothing gets sent to the observers when updating an existing element.
Only
> on new elements. If this is intentional, can you add a comment?

You are right. Added ReadingListWillMoveEntry

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:192: auto result =
std::find(read_->begin(), read_->end(), *existing_entry);
On 2016/11/01 18:27:25, skym wrote:
> There's a lot of std::find calls in here. They're all O(n), right? Is the
> assumption that the total number of elements should always be fairly small?

Yes, the number of entries should stay small

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:204:
storage_layer_->SaveEntry(*entry, true, read);
On 2016/11/01 18:27:25, skym wrote:
> Also, kind of weird that the storage call is in the middle here. Is there a
> specific reason? I feel like it'd make more sense to keep the |read_| and
> |unread_| updating as close together as possible. Unless I'm missing
something.

It is there because there is a std::move on the next call.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:204:
storage_layer_->SaveEntry(*entry, true, read);
On 2016/11/01 18:27:25, skym wrote:
> Are the arguments |true| and |read| swapped here?

Yes, thanks! Done.

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

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:69:
std::unique_ptr<sync_pb::ReadingListSpecifics> pb_entry_sync =
On 2016/11/01 18:27:26, skym wrote:
> I'd personally move this down to below the if(..) {.. return; } block.

Done.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:71: // Unref the URL
before making asynchronous call.
On 2016/11/01 18:27:26, skym wrote:
> Is this really needed? I'm having a difficult time understanding why this
would
> be necessary.

This was part of a fix that revealed to be unrelated. Removed.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:100: std::string local_key
= entry.URL().spec();
On 2016/11/01 18:27:26, skym wrote:
> Same as above.

Done.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:271:
std::unique_ptr<syncer::EntityData> entity_data(new syncer::EntityData());
On 2016/11/01 18:27:26, skym wrote:
> auto and base::MakeUnique?

Done.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:306: }
On 2016/11/01 06:14:10, pavely wrote:
> metadata_change_list contains metadata updates and progress marker
corresponding
> to updates in entity_changes. This function should transfer them to batch_ and
> commit all changes as part of the same transaction.
> 
> The same is true for MergeSyncData (see Sky's comments).

Done.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:310: // Asynchronously
retrieve the corresponding sync data for |storage_keys|.
On 2016/11/01 18:27:26, skym wrote:
> This implementation is not async.

Done.

https://codereview.chromium.org/2451843002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.cc:324: // Asynchronously
retrieve all of the local sync data.
On 2016/11/01 18:27:26, skym wrote:
> This implementation is not async.

Done.

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

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

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

+jif for iOS reading list.

[Olivier, 2016-11-02 17:40:49]

Description was changed from

==========
Sync reading list

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

to

==========
Sync reading list

BUG=661668
==========

[Olivier, 2016-11-02 17:43:29]

Description was changed from

==========
Sync reading list

BUG=661668
==========

to

==========
Add Store+Sync to reading list.

This CL creates a ModelTypeService for reading list.
This service will store persistent data for the Reading list model and sync
data between devices.

The previous store using NSUserDefaults will not be cleaned.
As it was only accessible from canary behind an experimental flag, no user
should be affected.

BUG=661668
==========

[jif, 2016-11-07 12:30:52]

first pass, did not look at the unittests yet.

https://codereview.chromium.org/2451843002/diff/140001/components/browser_syn...
File components/browser_sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/2451843002/diff/140001/components/browser_syn...
components/browser_sync/profile_sync_components_factory_impl.cc:287: if
(!disabled_types.Has(syncer::READING_LIST)) {
Shouldn't it be:
// Reading list sync is enabled by default.
?

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/pre...
File ios/chrome/browser/prefs/browser_prefs.mm (right):

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/pre...
ios/chrome/browser/prefs/browser_prefs.mm:96:
ReadingListModelFactory::RegisterProfilePrefs(registry);
one line up?

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

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_entry.h:70:
std::unique_ptr<reading_list::ReadingListLocal> AsReadingListLocal(
|AsReadingListSpecifics| is "for sync."
what is |AsReadingListLocal| for?

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_entry.h:87: void
MergeLocalStateFrom(ReadingListEntry& other);
with "ReadingListEntry&&" you wouldn't have to write "|other| must not be used
after this point.", and it would be obvious to the caller not to reuse the
parameter because they will have had to std::move it.

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

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model.cc:60:
DCHECK(IsPerformingBatchUpdates());
DCHECK(current_batch_updates_count_ > 0);

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

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:17: :
ReadingListModelImpl(NULL, NULL) {}
s/NULL/nullptr/

(TIL that we can chain constructors in C++)

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:111: return
GetMutableEntryFromURL(gurl, &read);
s/&read/nullptr/ ?

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:129: if (read) {
I really dislike the fact that we are forbidden to use references for input
parameters.

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:190: // Existing
entry is newer thatn sync one. Do not update it.
s/thatn/than the/

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:201: this,
std::distance(read_->begin(), result), true);
Optional nit: writing:
observer.ReadingListWillMoveEntry(this, index, true);
would be more readable.

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:202: if (result !=
read_->end()) {
I don't think "result != read_->end()" is supposed to happen, so how about a
DCHECK(result != read_->end()) ?

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:210: if (result !=
unread_->end()) {
Same here: replace with DCHECK?

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:288:
unread_->insert(unread_->begin(), std::move(entry));
Would push_back(std::move(entry)) work?
If yes, change line 292 as well.

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:454:
ReadingListModel::LeavingBatchUpdates();
Shouldn't |ReadingListModel::LeavingBatchUpdates()| be called at the end of the
method?

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:467:
ReadingListModel::EnteringBatchUpdates();
And this, at the beginning of the method?

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:479: bool
ReadingListModelImpl::LoadPersistentHasUnseen() {
Tiny nit: we aren't loading anything here.
How about GetLoadPersistentHasUnseen?
And by symmetry, s/SavePersistentHasUnseen/SetPersistentHasUnseen/ ?

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:490: return
storage_layer_->GetModelTypeService();
Would this:

if (!storage_layer_)
  return nullptr;

be useful?

[Olivier, 2016-11-07 18:18:46]

Thanks.
I added unittests and fixed all comments.
pavely or skym, could you take another look at the sync part?

https://codereview.chromium.org/2451843002/diff/140001/components/browser_syn...
File components/browser_sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/2451843002/diff/140001/components/browser_syn...
components/browser_sync/profile_sync_components_factory_impl.cc:287: if
(!disabled_types.Has(syncer::READING_LIST)) {
On 2016/11/07 12:30:51, jif wrote:
> Shouldn't it be:
> // Reading list sync is enabled by default.
> ?

Done.

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/pre...
File ios/chrome/browser/prefs/browser_prefs.mm (right):

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/pre...
ios/chrome/browser/prefs/browser_prefs.mm:96:
ReadingListModelFactory::RegisterProfilePrefs(registry);
On 2016/11/07 12:30:51, jif wrote:
> one line up?

Done.

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

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_entry.h:70:
std::unique_ptr<reading_list::ReadingListLocal> AsReadingListLocal(
On 2016/11/07 12:30:51, jif wrote:
> |AsReadingListSpecifics| is "for sync."
> what is |AsReadingListLocal| for?

for local storage. Added to comment.

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_entry.h:87: void
MergeLocalStateFrom(ReadingListEntry& other);
On 2016/11/07 12:30:51, jif wrote:
> with "ReadingListEntry&&" you wouldn't have to write "|other| must not be used
> after this point.", and it would be obvious to the caller not to reuse the
> parameter because they will have had to std::move it.

This is more complicated than  that :)

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

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model.cc:60:
DCHECK(IsPerformingBatchUpdates());
On 2016/11/07 12:30:51, jif wrote:
> DCHECK(current_batch_updates_count_ > 0);

Done.

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

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:17: :
ReadingListModelImpl(NULL, NULL) {}
On 2016/11/07 12:30:52, jif wrote:
> s/NULL/nullptr/
> 
> (TIL that we can chain constructors in C++)

Done.

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:111: return
GetMutableEntryFromURL(gurl, &read);
On 2016/11/07 12:30:52, jif wrote:
> s/&read/nullptr/ ?

Done.

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:190: // Existing
entry is newer thatn sync one. Do not update it.
On 2016/11/07 12:30:52, jif wrote:
> s/thatn/than the/

Done.

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:201: this,
std::distance(read_->begin(), result), true);
On 2016/11/07 12:30:51, jif wrote:
> Optional nit: writing:
> observer.ReadingListWillMoveEntry(this, index, true);
> would be more readable.

Done.

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:202: if (result !=
read_->end()) {
On 2016/11/07 12:30:52, jif wrote:
> I don't think "result != read_->end()" is supposed to happen, so how about a
> DCHECK(result != read_->end()) ?

Done.

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:210: if (result !=
unread_->end()) {
On 2016/11/07 12:30:52, jif wrote:
> Same here: replace with DCHECK?

Done.

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:288:
unread_->insert(unread_->begin(), std::move(entry));
On 2016/11/07 12:30:52, jif wrote:
> Would push_back(std::move(entry)) work?
> If yes, change line 292 as well.

It would, but may be bad in term of performances as it will require a sorting
after.
We know that the correct place is in front, and I hope std::sort will find out
hte vector is already sorted.

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:454:
ReadingListModel::LeavingBatchUpdates();
On 2016/11/07 12:30:52, jif wrote:
> Shouldn't |ReadingListModel::LeavingBatchUpdates()| be called at the end of
the
> method?

Done.

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:467:
ReadingListModel::EnteringBatchUpdates();
On 2016/11/07 12:30:52, jif wrote:
> And this, at the beginning of the method?

Done.

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:479: bool
ReadingListModelImpl::LoadPersistentHasUnseen() {
On 2016/11/07 12:30:52, jif wrote:
> Tiny nit: we aren't loading anything here.
> How about GetLoadPersistentHasUnseen?
> And by symmetry, s/SavePersistentHasUnseen/SetPersistentHasUnseen/ ?

Done.

https://codereview.chromium.org/2451843002/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:490: return
storage_layer_->GetModelTypeService();
On 2016/11/07 12:30:51, jif wrote:
> Would this:
> 
> if (!storage_layer_)
>   return nullptr;
> 
> be useful?

Yes, thanks !

[pavely, 2016-11-08 02:30:43]

Olivier. 

I thought about relationship between store and model objects, I don’t think we
can avoid store->model->store call. We (sync team) will work on suggesting
elegant pattern for such interactions as we anticipate it will be common for
other services implementations, but we don’t have good recommendation at the
moment.

The thing that worries me about this sequence and implementation of
store::SaveEntry though is that it has BeginTransaction()/CommitTransaction()
pair. The main issue is that transaction shouldn’t be committed in the middle of
processing server updates in store::ApplySyncChanges. I understand that your
implementation of nested transactions prevents the ones in SaveEntry from
committing, but it makes implementation fragile.

The fact is, model always knows when to begin/commit transaction. When handling
local modifications model::AddEntry() should begin transaction, call
store::SaveEntry() and then commit transaction. When handling remote changes
from sync store::ApplySyncChanges() should begin transaction, call
model::SyncAddEntry() which in turn calls store::SaveEntry() and at the end
commit transaction. In both these scenarios store::SaveEntry() doesn’t need to
do anything with transactions. If you expose
BeginTransaction()/CommitTransaction() through ReadingListModelStorage you could
avoid logic that handles nested transactions and instead assert that they never
happen.

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

https://codereview.chromium.org/2451843002/diff/160001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:326:
model_->CallbackEntryReadStatusURL(GURL(url_string), add_to_batch_callback);
You don't need to use model::CallbackEntryReadStatusURL here. You can just call
model::GetEntryFromURL and then call AddEntryToBatch on it. Less callbacks will
make code more readable.

[Olivier, 2016-11-08 09:12:19]

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

https://codereview.chromium.org/2451843002/diff/160001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:326:
model_->CallbackEntryReadStatusURL(GURL(url_string), add_to_batch_callback);
On 2016/11/08 02:30:43, pavely wrote:
> You don't need to use model::CallbackEntryReadStatusURL here. You can just
call
> model::GetEntryFromURL and then call AddEntryToBatch on it. Less callbacks
will
> make code more readable.

GetEntryFromURL does not tell me if the entry is read or not.
We can do without callback, but this will require one more call (IsEntryRead).

[Olivier, 2016-11-08 09:12:56]

On 2016/11/08 02:30:43, pavely wrote:
> Olivier. 
> 
> I thought about relationship between store and model objects, I don’t think we
> can avoid store->model->store call. We (sync team) will work on suggesting
> elegant pattern for such interactions as we anticipate it will be common for
> other services implementations, but we don’t have good recommendation at the
> moment.
> 
> The thing that worries me about this sequence and implementation of
> store::SaveEntry though is that it has BeginTransaction()/CommitTransaction()
> pair. The main issue is that transaction shouldn’t be committed in the middle
of
> processing server updates in store::ApplySyncChanges. I understand that your
> implementation of nested transactions prevents the ones in SaveEntry from
> committing, but it makes implementation fragile.
> 
> The fact is, model always knows when to begin/commit transaction. When
handling
> local modifications model::AddEntry() should begin transaction, call
> store::SaveEntry() and then commit transaction. When handling remote changes
> from sync store::ApplySyncChanges() should begin transaction, call
> model::SyncAddEntry() which in turn calls store::SaveEntry() and at the end
> commit transaction. In both these scenarios store::SaveEntry() doesn’t need to
> do anything with transactions. If you expose
> BeginTransaction()/CommitTransaction() through ReadingListModelStorage you
could
> avoid logic that handles nested transactions and instead assert that they
never
> happen.


The problem I will get by not handling nested transaction is that there is
another source of batch updates.
Other applications can add reading list entries on disk and on launching Chrome,
we check what is sent by the other apps.
That is asynchronous and is dons in a single batch (which can take a few
seconds).
If a sync happens in the middle of this sync, the sync batch will be synced with
it.
The other solution is to lock the model when one source of ReadingListEntries
receives a batch but this seems more complicated as this would require to buffer
the data.
WDYT?

> 
>
https://codereview.chromium.org/2451843002/diff/160001/ios/chrome/browser/rea...
> File ios/chrome/browser/reading_list/reading_list_store.cc (right):
> 
>
https://codereview.chromium.org/2451843002/diff/160001/ios/chrome/browser/rea...
> ios/chrome/browser/reading_list/reading_list_store.cc:326:
> model_->CallbackEntryReadStatusURL(GURL(url_string), add_to_batch_callback);
> You don't need to use model::CallbackEntryReadStatusURL here. You can just
call
> model::GetEntryFromURL and then call AddEntryToBatch on it. Less callbacks
will
> make code more readable.

[Olivier, 2016-11-08 14:58:12]

I removed the callback call and change the Begin/CommitTransaction to be a
scoped token.

Frankly, I am not sure this is simpler, but may be less error prone (token
always go out of scope).
Do you find this better?

[pavely, 2016-11-09 06:47:07]

On 2016/11/08 14:58:12, Olivier Robin wrote:
> I removed the callback call and change the Begin/CommitTransaction to be a
> scoped token.
> 
> Frankly, I am not sure this is simpler, but may be less error prone (token
> always go out of scope).
> Do you find this better?

Did you mean to upload updated patch?

I think approach with passing token will work. If I guess correctly you'll
create variable in the function where transaction should be started and
committed and pass pointer to it to indicate that called functions already
execute in context of the transaction.

As for callback, in the end it is up to you to decide which approach fits all
scenarios better.

[Olivier, 2016-11-09 08:13:52]

On 2016/11/09 06:47:07, pavely wrote:
> On 2016/11/08 14:58:12, Olivier Robin wrote:
> > I removed the callback call and change the Begin/CommitTransaction to be a
> > scoped token.
> > 
> > Frankly, I am not sure this is simpler, but may be less error prone (token
> > always go out of scope).
> > Do you find this better?
> 
> Did you mean to upload updated patch?
> 
> I think approach with passing token will work. If I guess correctly you'll
> create variable in the function where transaction should be started and
> committed and pass pointer to it to indicate that called functions already
> execute in context of the transaction.
> 
> As for callback, in the end it is up to you to decide which approach fits all
> scenarios better.

Yes, sorry, patch was not uploaded.

[Olivier, 2016-11-09 17:16:37]

I did a full rebase including the latest sync change.
I hope I did that correctly.
Main changes are
syncer::ModelTypeService -> syncer::ModelTypeSyncBridge

simple_metadata_change_list.h -> accumulating_metadata_change_list.h

The API also changed a bit.

[pavely, 2016-11-14 07:45:34]

Olivier, I left bunch of cleanup nits and one important comment in
reading_list_store.cc. PTAL

https://codereview.chromium.org/2451843002/diff/200001/components/browser_syn...
File components/browser_sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/2451843002/diff/200001/components/browser_syn...
components/browser_sync/profile_sync_components_factory_impl.cc:282:
syncer::READING_LIST, base::Bind(&base::debug::DumpWithoutCrashing),
It's Ok to pass base::debug::DumpWithoutCrashing during development as it will
upload dump for every unrecoverable error. As this code gets to stable you
should switch to error_callback which is bound to
syncer::ReportUnrecoverableError. The difference is that
ReportUnrecoverableError only uploads dump on canary/dev channels and only in 1%
of errors.

Could you add TODO here to switch to error_callback.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/BUILD.gn (right):

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/BUILD.gn:35: "//components/leveldb_proto",
Do you need dependency on leveldb_proto?

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

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_entry.h:9: #include <vector>
Do you need vector header?

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_entry.h:12: #include
"base/time/default_tick_clock.h"
Same with default_tick_clock.h.

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

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model.cc:9: #include
"ios/web/public/web_thread.h"
web_tread.h is not used.

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

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model.h:23: using
ReadingListEntries = std::vector<ReadingListEntry>;
reading_list_model.h, reading_list_store_delegate.h and
reading_list_model_impl.h introduce global name for ReadingListEntries.
It is better to either move them inside corresponding classes or move "using"
declaration into reading_list_entry.h.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model.h:37: class ReadingListModel
: public base::NonThreadSafe {
You need to include non_thread_safe.h.

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

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_factory.cc:13: #include
"components/leveldb_proto/proto_database_impl.h"
You shouldn't need these two headers.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_factory.cc:74:
database_dir.AsUTF8Unsafe(), background_task_runner),
This code creates store in a location separate from sync's device info. It is
beneficial to share store location between multiple datatypes as it will reduce
overhead. I'll need to adjust store implementation to use location of shared
store. Meanwhile let's land this change as is, only pleas add TODO with my name
to pass shared location to CreateStore.

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

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:15: #include
"ios/web/public/web_thread.h"
ReadingListModelImpl interacts with store through ReadingListModelStorage
interface. including reading_list_store.h shouldn't be necessary.
web_thread.h doesn't seem to be used either.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:186: if
(is_existing_entry_read) {
Each branch of if statement can be factored out into helper function that takes
vector and |read| flag. This will avoid code duplication.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:193:
read_->erase(result);
Naive question... I'm just curious: With the rest of the entries in |read_|
vector moved, do you need to notify observers?

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:204: if
(storage_layer_) {
SyncAddEntry should be invoked from ReadingListStore meaning storage_layer_
should be valid. It is better to assert that with DCHECK. Same applies to line
159.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:343: void
ReadingListModelImpl::SetEntryTitle(const GURL& url,
It is typical pattern for update functions in this class to first scan unread
list looking for entry, then apply update. If entry is not in unread_ list then
do the same for read_ list. You could use GetMutableEntryFromURL to find entry
pointer and whether it is in read or unread list. Then you can update property,
call SaveEntry, notify observers. If only you had ReadingListWillUpdateEntry
with |read| parameter instead of pair
ReadingListWillUpdateUnreadEntry/ReadingListWillUpdateReadEntry in observer
interface. This would collapse the last switch by |read| value in this sequence.

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

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.h:10: #include
"base/threading/non_thread_safe.h"
non_thread_safe.h is not needed here.

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

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:8: #include
"base/files/file_path.h"
file_path.h is not used.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:127: return;
When initialization fails, bridge should notify change_processor about failure.
It in turn will pass failure to sync_engine.
Call
change_processor()->OnMetadataLoaded(change_processor()->CreateAndUploadError(FROM_HERE,
"ERROR_MESSAGE_GOES_HERE"), nullptr);

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:135:
std::unique_ptr<reading_list::ReadingListLocal> proto =
You can create proto object on the stack instead of unique_ptr. You only use
this object locally.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:165:
change_processor()->OnMetadataLoaded(syncer::SyncError(),
You should pass sync_error to OnMetadataLoaded()

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:177: store_ =
std::move(store);
If store creation fails (result != SUCCESS) then store will be nullptr. This
function should handle failure.
I'm fine with leaving this for a separate change, just leave TODO here.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:264: syncer::SyncError
ReadingListStore::ApplySyncChanges(
Important comment about ApplySyncChanges:
metadata_change_list contains sync metadata about entities from entity_changes,
including their specifics_hash. This hash is used to detect changes to local
copy. Therefore the assumption is that entry saved in local store should match
its specifics hash in sync metadata.
For ApplySyncChanges (and MergeSyncData) this means that it should either store
entity in the batch verbatim, or store updated entity along with its updated
metadata. 
I think ApplySyncChanges should create batch, iterate over entity_changes
writing them to the batch verbatim, then transfer metadata_change_list there and
then notify model about these changes. In turn, model can decide to update
entity and store different version of it. These modifications should still be
forwarded to change_processor as they might affect specifics hash. I think
functions SaveEntry/RemoveEntry should still notify change_processor even if
call came from sync.

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

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:9: #include
"components/leveldb_proto/proto_database.h"
proto_database.h and ReadingListDB are not used.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:24: typedef
base::Callback<void(
Could you move this typedef inside ReadingListStore class. The way it is
currently written it introduces global name StoreFactoryFunction.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:32: using EntryVector =
std::vector<reading_list::ReadingListLocal>;
EntryVector is not used.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:42: void
SetReadingListModel(ReadingListModel* model,
Could you add comment to this function here or in ReadingListModelStorage noting
that it triggers store initialization. From looking at the interface it gives
impression of a trivial setter method.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:127: void
OnDatabaseInit(bool success);
OnDatabaseInit is not defined.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:140: bool database_loaded_;
database_loaded_ doesn't seem to be read anywhere.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:146: int
pending_transaction_;
I think pending_transaction_count_ would be a better name.

[Olivier, 2016-11-14 11:36:49]

Thanks!
I understand that the sync framework will prevent sync loop (that was the reason
of from_sync). Is that correct?

Thanks again for the review.

https://codereview.chromium.org/2451843002/diff/200001/components/browser_syn...
File components/browser_sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/2451843002/diff/200001/components/browser_syn...
components/browser_sync/profile_sync_components_factory_impl.cc:282:
syncer::READING_LIST, base::Bind(&base::debug::DumpWithoutCrashing),
On 2016/11/14 07:45:33, pavely wrote:
> It's Ok to pass base::debug::DumpWithoutCrashing during development as it will
> upload dump for every unrecoverable error. As this code gets to stable you
> should switch to error_callback which is bound to
> syncer::ReportUnrecoverableError. The difference is that
> ReportUnrecoverableError only uploads dump on canary/dev channels and only in
1%
> of errors.
> 
> Could you add TODO here to switch to error_callback.

Directly switched to error_callback

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/BUILD.gn (right):

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/BUILD.gn:35: "//components/leveldb_proto",
On 2016/11/14 07:45:33, pavely wrote:
> Do you need dependency on leveldb_proto?

Cleaned includes and removed dep.

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

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_entry.h:9: #include <vector>
On 2016/11/14 07:45:33, pavely wrote:
> Do you need vector header?

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_entry.h:12: #include
"base/time/default_tick_clock.h"
On 2016/11/14 07:45:33, pavely wrote:
> Same with default_tick_clock.h.

Done.

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

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model.cc:9: #include
"ios/web/public/web_thread.h"
On 2016/11/14 07:45:33, pavely wrote:
> web_tread.h is not used.

Done.

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

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model.h:23: using
ReadingListEntries = std::vector<ReadingListEntry>;
On 2016/11/14 07:45:33, pavely wrote:
> reading_list_model.h, reading_list_store_delegate.h and
> reading_list_model_impl.h introduce global name for ReadingListEntries.
> It is better to either move them inside corresponding classes or move "using"
> declaration into reading_list_entry.h.

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model.h:37: class ReadingListModel
: public base::NonThreadSafe {
On 2016/11/14 07:45:33, pavely wrote:
> You need to include non_thread_safe.h.

Done.

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

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_factory.cc:13: #include
"components/leveldb_proto/proto_database_impl.h"
On 2016/11/14 07:45:33, pavely wrote:
> You shouldn't need these two headers.

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_factory.cc:74:
database_dir.AsUTF8Unsafe(), background_task_runner),
On 2016/11/14 07:45:33, pavely wrote:
> This code creates store in a location separate from sync's device info. It is
> beneficial to share store location between multiple datatypes as it will
reduce
> overhead. I'll need to adjust store implementation to use location of shared
> store. Meanwhile let's land this change as is, only pleas add TODO with my
name
> to pass shared location to CreateStore.

Done.
Will there be a loss of data when the path is changed?

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

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:15: #include
"ios/web/public/web_thread.h"
On 2016/11/14 07:45:33, pavely wrote:
> ReadingListModelImpl interacts with store through ReadingListModelStorage
> interface. including reading_list_store.h shouldn't be necessary.
> web_thread.h doesn't seem to be used either.

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:186: if
(is_existing_entry_read) {
On 2016/11/14 07:45:33, pavely wrote:
> Each branch of if statement can be factored out into helper function that
takes
> vector and |read| flag. This will avoid code duplication.

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:193:
read_->erase(result);
On 2016/11/14 07:45:33, pavely wrote:
> Naive question... I'm just curious: With the rest of the entries in |read_|
> vector moved, do you need to notify observers?

This follow the notifications in other models like bookmarks. When we move an
entry, this implicitly pack the remaining ones.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:204: if
(storage_layer_) {
On 2016/11/14 07:45:33, pavely wrote:
> SyncAddEntry should be invoked from ReadingListStore meaning storage_layer_
> should be valid. It is better to assert that with DCHECK. Same applies to line
> 159.

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:343: void
ReadingListModelImpl::SetEntryTitle(const GURL& url,
On 2016/11/14 07:45:33, pavely wrote:
> It is typical pattern for update functions in this class to first scan unread
> list looking for entry, then apply update. If entry is not in unread_ list
then
> do the same for read_ list. You could use GetMutableEntryFromURL to find entry
> pointer and whether it is in read or unread list. Then you can update
property,
> call SaveEntry, notify observers. If only you had ReadingListWillUpdateEntry
> with |read| parameter instead of pair
> ReadingListWillUpdateUnreadEntry/ReadingListWillUpdateReadEntry in observer
> interface. This would collapse the last switch by |read| value in this
sequence.
> 

This is something we definitely want to do. We dont know yet if we will just
merge read and unread and put the read parameter in ReadingListEntry or if we
will add parameter to the notification.
Filed crbug.com/664924 for tracking.
Files crbug.

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

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model_impl.h:10: #include
"base/threading/non_thread_safe.h"
On 2016/11/14 07:45:33, pavely wrote:
> non_thread_safe.h is not needed here. 

Done.

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

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:8: #include
"base/files/file_path.h"
On 2016/11/14 07:45:33, pavely wrote:
> file_path.h is not used.

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:127: return;
On 2016/11/14 07:45:34, pavely wrote:
> When initialization fails, bridge should notify change_processor about
failure.
> It in turn will pass failure to sync_engine.
> Call
>
change_processor()->OnMetadataLoaded(change_processor()->CreateAndUploadError(FROM_HERE,
> "ERROR_MESSAGE_GOES_HERE"), nullptr);

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:135:
std::unique_ptr<reading_list::ReadingListLocal> proto =
On 2016/11/14 07:45:34, pavely wrote:
> You can create proto object on the stack instead of unique_ptr. You only use
> this object locally.

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:165:
change_processor()->OnMetadataLoaded(syncer::SyncError(),
On 2016/11/14 07:45:33, pavely wrote:
> You should pass sync_error to OnMetadataLoaded()

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:177: store_ =
std::move(store);
On 2016/11/14 07:45:34, pavely wrote:
> If store creation fails (result != SUCCESS) then store will be nullptr. This
> function should handle failure.
> I'm fine with leaving this for a separate change, just leave TODO here.

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:264: syncer::SyncError
ReadingListStore::ApplySyncChanges(
On 2016/11/14 07:45:33, pavely wrote:
> Important comment about ApplySyncChanges:
> metadata_change_list contains sync metadata about entities from
entity_changes,
> including their specifics_hash. This hash is used to detect changes to local
> copy. Therefore the assumption is that entry saved in local store should match
> its specifics hash in sync metadata.
> For ApplySyncChanges (and MergeSyncData) this means that it should either
store
> entity in the batch verbatim, or store updated entity along with its updated
> metadata. 
> I think ApplySyncChanges should create batch, iterate over entity_changes
> writing them to the batch verbatim, then transfer metadata_change_list there
and
> then notify model about these changes. In turn, model can decide to update
> entity and store different version of it. These modifications should still be
> forwarded to change_processor as they might affect specifics hash. I think
> functions SaveEntry/RemoveEntry should still notify change_processor even if
> call came from sync.

I did two passes and removed the from_sync parameter.

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

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:9: #include
"components/leveldb_proto/proto_database.h"
On 2016/11/14 07:45:34, pavely wrote:
> proto_database.h and ReadingListDB are not used.

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:24: typedef
base::Callback<void(
On 2016/11/14 07:45:34, pavely wrote:
> Could you move this typedef inside ReadingListStore class. The way it is
> currently written it introduces global name StoreFactoryFunction.

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:32: using EntryVector =
std::vector<reading_list::ReadingListLocal>;
On 2016/11/14 07:45:34, pavely wrote:
> EntryVector is not used.

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:42: void
SetReadingListModel(ReadingListModel* model,
On 2016/11/14 07:45:34, pavely wrote:
> Could you add comment to this function here or in ReadingListModelStorage
noting
> that it triggers store initialization. From looking at the interface it gives
> impression of a trivial setter method.

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:127: void
OnDatabaseInit(bool success);
On 2016/11/14 07:45:34, pavely wrote:
> OnDatabaseInit is not defined.

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:140: bool database_loaded_;
On 2016/11/14 07:45:34, pavely wrote:
> database_loaded_ doesn't seem to be read anywhere.

Done.

https://codereview.chromium.org/2451843002/diff/200001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.h:146: int
pending_transaction_;
On 2016/11/14 07:45:34, pavely wrote:
> I think pending_transaction_count_ would be a better name.

Done.

[jif, 2016-11-15 16:28:33]

lgtm

[pavely, 2016-11-16 01:54:54]

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

https://codereview.chromium.org/2451843002/diff/240001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:194: // Perform the
initial merge between local and sync data. This should only be
I reshuffled implementation of MergeSyncData and ApplySyncChanges. Could you
take a look:
http://gpaste/5931954100764672

[Olivier, 2016-11-16 12:27:52]

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

https://codereview.chromium.org/2451843002/diff/260001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:236:
batch_->WriteData(entry->URL().spec(), pb_entry->SerializeAsString());
pb_entry does not contain the local info

https://codereview.chromium.org/2451843002/diff/260001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:289:
delegate_->SyncRemoveEntry(GURL(change.storage_key()));
Adding this compared to the gpaste.

https://codereview.chromium.org/2451843002/diff/260001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:300:
entry->AsReadingListLocal(read);
This is suspicious as pb_entry does not contain distillation info.

[Olivier, 2016-11-16 15:03:16]

Hi pavely,

I copied the change you put in gpaste (patch 14).
I think this change will temporarily delete the local data from the local store.
This could be a problem if a crash happen just at this moment and some local
data will be unreachable on disk.

I tried to implement what we talked about on Tuesday.
The store method now do the merge and transmit it to the model.
This means that the method from the ReadingListStoreDelegate won't call back the
store.

I think this is clearer like that.

You will notice that I voluntarily let redundancies in the code of
ApplySyncChanges and MergeSyncData.
This is to make code path more clear in case we have comments and we want to
change the behavior in a particular case.
Once this CL looks good and is landed, we can refactor this and make it mode
compact in a future CL.

Let me know if this change is clear and if this new behavior looks good.

[pavely, 2016-11-17 06:39:23]

lgtm.

I left a comment in reading_list_store. I don't propose to change anything in
this CL, but this is something to be aware of.

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

https://codereview.chromium.org/2451843002/diff/300001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:364:
change_processor()->Put(entry_sync_pb->entry_id(),
Important consideration about passing accepted sync change back to
change_processor is to avoid ping-pong type scenarios:
1. Client A makes change
2. Client B receives it from server, accepts it, but passes it to
change_processor
3. This change gets committed to server and client A does the same thing
4. Repeat from step 2.

If the entry passed to change_processor is the same that was just received then
change_processor will notice that specifics hash didn't change and will not
schedule change for syncing, thus avoiding above situation.

[Olivier, 2016-11-17 08:54:35]

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

https://codereview.chromium.org/2451843002/diff/300001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_store.cc:364:
change_processor()->Put(entry_sync_pb->entry_id(),
On 2016/11/17 06:39:23, pavely wrote:
> Important consideration about passing accepted sync change back to
> change_processor is to avoid ping-pong type scenarios:
> 1. Client A makes change
> 2. Client B receives it from server, accepts it, but passes it to
> change_processor
> 3. This change gets committed to server and client A does the same thing
> 4. Repeat from step 2.
> 
> If the entry passed to change_processor is the same that was just received
then
> change_processor will notice that specifics hash didn't change and will not
> schedule change for syncing, thus avoiding above situation.

Added a todo

[Olivier, 2016-11-17 08:54:43]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2016-11-17 08:54:44]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org,
pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2451843002/#ps320001
(title: "added todo")

[commit-bot: I haz the power, 2016-11-17 08:55:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Olivier, 2016-11-17 08:55:50]

Thanks a lot for these reviews!

[commit-bot: I haz the power, 2016-11-17 08:57:44]

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

[commit-bot: I haz the power, 2016-11-17 08:57:45]

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

[Olivier, 2016-11-17 09:02:53]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2016-11-17 09:02:53]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org,
pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2451843002/#ps340001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-17 09:03:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 09:44:24]

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

[commit-bot: I haz the power, 2016-11-17 09:44:25]

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

[Olivier, 2016-11-17 10:08:05]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2016-11-17 10:08:05]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org,
pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2451843002/#ps360001
(title: "disable by default")

[commit-bot: I haz the power, 2016-11-17 10:08:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 10:38:29]

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

[commit-bot: I haz the power, 2016-11-17 10:38:31]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Olivier, 2016-11-17 10:42:23]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2016-11-17 10:42:24]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org,
pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2451843002/#ps380001
(title: "disable by default")

[commit-bot: I haz the power, 2016-11-17 10:42:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 11:35:12]

Description was changed from

==========
Add Store+Sync to reading list.

This CL creates a ModelTypeService for reading list.
This service will store persistent data for the Reading list model and sync
data between devices.

The previous store using NSUserDefaults will not be cleaned.
As it was only accessible from canary behind an experimental flag, no user
should be affected.

BUG=661668
==========

to

==========
Add Store+Sync to reading list.

This CL creates a ModelTypeService for reading list.
This service will store persistent data for the Reading list model and sync
data between devices.

The previous store using NSUserDefaults will not be cleaned.
As it was only accessible from canary behind an experimental flag, no user
should be affected.

BUG=661668
==========

[commit-bot: I haz the power, 2016-11-17 11:35:14]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-11-17 11:39:28]

Description was changed from

==========
Add Store+Sync to reading list.

This CL creates a ModelTypeService for reading list.
This service will store persistent data for the Reading list model and sync
data between devices.

The previous store using NSUserDefaults will not be cleaned.
As it was only accessible from canary behind an experimental flag, no user
should be affected.

BUG=661668
==========

to

==========
Add Store+Sync to reading list.

This CL creates a ModelTypeService for reading list.
This service will store persistent data for the Reading list model and sync
data between devices.

The previous store using NSUserDefaults will not be cleaned.
As it was only accessible from canary behind an experimental flag, no user
should be affected.

BUG=661668

Committed: https://crrev.com/faa080c524c6a80c2ac276359e29dec8cf7946c4
Cr-Commit-Position: refs/heads/master@{#432845}
==========

[commit-bot: I haz the power, 2016-11-17 11:39:29]

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

[Matt Giuca, 2016-11-18 01:05:46]

A revert of this CL (patchset #20 id:380001) has been created in
https://codereview.chromium.org/2507053005/ by mgiuca@chromium.org.

The reason for reverting is: Failing compile on ios-simulator-xcode-clang:

reading_list_store_unittest.cc:75:12: error: no viable conversion from returned
value of type 'unique_ptr<TestModelTypeChangeProcessor>' to function return type
'unique_ptr<syncer::ModelTypeChangeProcessor>'
    return processor;
           ^~~~~~~~~

It isn't legal to return a unique_ptr<B> from a function that returns
unique_ptr<A> even if A is a base class. You need to explicitly declare
|processor| to have type unique_ptr<syncer::ModelTypeChangeProcessor>.
I'm not sure why this wasn't picked up by the try bots..

[Matt Giuca, 2016-11-18 01:07:11]

On 2016/11/18 01:05:46, Matt Giuca wrote:
> A revert of this CL (patchset #20 id:380001) has been created in
> https://codereview.chromium.org/2507053005/ by mailto:mgiuca@chromium.org.
> 
> The reason for reverting is: Failing compile on ios-simulator-xcode-clang:
> 
> reading_list_store_unittest.cc:75:12: error: no viable conversion from
returned
> value of type 'unique_ptr<TestModelTypeChangeProcessor>' to function return
type
> 'unique_ptr<syncer::ModelTypeChangeProcessor>'
>     return processor;
>            ^~~~~~~~~
> 
> It isn't legal to return a unique_ptr<B> from a function that returns
> unique_ptr<A> even if A is a base class. You need to explicitly declare
> |processor| to have type unique_ptr<syncer::ModelTypeChangeProcessor>.
> I'm not sure why this wasn't picked up by the try bots..

The full error log:

[1821/8084] CXX
obj/ios/chrome/browser/reading_list/unit_tests/reading_list_store_unittest.o
FAILED:
obj/ios/chrome/browser/reading_list/unit_tests/reading_list_store_unittest.o 
/b/build/slave/cache/cipd/goma/gomacc clang++ -MMD -MF
obj/ios/chrome/browser/reading_list/unit_tests/reading_list_store_unittest.o.d
-DV8_DEPRECATION_WARNINGS -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD
-DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=284979-2
-DCR_XCODE_VERSION=0800 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_DEBUG
-DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1
-DGTEST_HAS_POSIX_RE=0 -DGTEST_LANG_CXX11=1 -DGOOGLE_PROTOBUF_NO_RTTI
-DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DHAVE_PTHREAD -DU_USING_ICU_NAMESPACE=0
-DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION
-DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DUNIT_TEST -I../.. -Igen
-I../../testing/gtest/include -I../../third_party/protobuf/src -Igen/protoc_out
-I../../third_party/protobuf/src -I../../third_party/ced/src
-I../../third_party/icu/source/common -I../../third_party/icu/source/i18n
-I../../testing/gmock_custom -I../../testing/gmock/include -fno-strict-aliasing
--param=ssp-buffer-size=4 -fstack-protector -fcolor-diagnostics -arch x86_64
-Wall -Werror -Wextra -Wno-missing-field-initializers -Wno-unused-parameter
-Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register
-Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override
-Wno-shift-negative-value -O0 -gdwarf-2 -isysroot
/Applications/Xcode8.0.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator10.0.sdk
-stdlib=libc++ -mios-simulator-version-min=9.0 -fvisibility=hidden
-Wheader-hygiene -Wstring-conversion -Wno-unused-function
-fno-threadsafe-statics -fvisibility-inlines-hidden
-Wno-undefined-bool-conversion -Wno-tautological-undefined-compare -std=c++11
-fno-rtti -fno-exceptions -c
../../ios/chrome/browser/reading_list/reading_list_store_unittest.cc -o
obj/ios/chrome/browser/reading_list/unit_tests/reading_list_store_unittest.o
../../ios/chrome/browser/reading_list/reading_list_store_unittest.cc:75:12:
error: no viable conversion from returned value of type
'unique_ptr<TestModelTypeChangeProcessor,
default_delete<TestModelTypeChangeProcessor>>' to function return type
'unique_ptr<syncer::ModelTypeChangeProcessor,
default_delete<syncer::ModelTypeChangeProcessor>>'
    return processor;
           ^~~~~~~~~
/Applications/Xcode8.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2553:29:
note: candidate constructor (the implicit copy constructor) not viable: no known
conversion from 'std::__1::unique_ptr<TestModelTypeChangeProcessor,
std::__1::default_delete<TestModelTypeChangeProcessor> >' to 'const
std::__1::unique_ptr<syncer::ModelTypeChangeProcessor,
std::__1::default_delete<syncer::ModelTypeChangeProcessor> > &' for 1st argument
class _LIBCPP_TYPE_VIS_ONLY unique_ptr
                            ^
/Applications/Xcode8.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2582:49:
note: candidate constructor not viable: no known conversion from
'std::__1::unique_ptr<TestModelTypeChangeProcessor,
std::__1::default_delete<TestModelTypeChangeProcessor> >' to 'nullptr_t' for 1st
argument
    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR unique_ptr(nullptr_t) _NOEXCEPT
                                                ^
/Applications/Xcode8.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2609:31:
note: candidate constructor not viable: no known conversion from
'std::__1::unique_ptr<TestModelTypeChangeProcessor,
std::__1::default_delete<TestModelTypeChangeProcessor> >' to
'std::__1::unique_ptr<syncer::ModelTypeChangeProcessor,
std::__1::default_delete<syncer::ModelTypeChangeProcessor> > &&' for 1st
argument
    _LIBCPP_INLINE_VISIBILITY unique_ptr(unique_ptr&& __u) _NOEXCEPT
                              ^
/Applications/Xcode8.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2613:9:
note: candidate constructor [with _Up = TestModelTypeChangeProcessor, _Ep =
std::__1::default_delete<TestModelTypeChangeProcessor>] not viable: no known
conversion from 'std::__1::unique_ptr<TestModelTypeChangeProcessor,
std::__1::default_delete<TestModelTypeChangeProcessor> >' to
'unique_ptr<TestModelTypeChangeProcessor,
std::__1::default_delete<TestModelTypeChangeProcessor> > &&' for 1st argument
        unique_ptr(unique_ptr<_Up, _Ep>&& __u,
        ^
/Applications/Xcode8.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2628:35:
note: candidate template ignored: could not match 'auto_ptr' against
'unique_ptr'
        _LIBCPP_INLINE_VISIBILITY unique_ptr(auto_ptr<_Up>&& __p,
                                  ^
1 error generated.

[Matt Giuca, 2016-11-18 01:07:36]

On 2016/11/18 01:07:11, Matt Giuca wrote:
> On 2016/11/18 01:05:46, Matt Giuca wrote:
> > A revert of this CL (patchset #20 id:380001) has been created in
> > https://codereview.chromium.org/2507053005/ by mailto:mgiuca@chromium.org.
> > 
> > The reason for reverting is: Failing compile on ios-simulator-xcode-clang:
> > 
> > reading_list_store_unittest.cc:75:12: error: no viable conversion from
> returned
> > value of type 'unique_ptr<TestModelTypeChangeProcessor>' to function return
> type
> > 'unique_ptr<syncer::ModelTypeChangeProcessor>'
> >     return processor;
> >            ^~~~~~~~~
> > 
> > It isn't legal to return a unique_ptr<B> from a function that returns
> > unique_ptr<A> even if A is a base class. You need to explicitly declare
> > |processor| to have type unique_ptr<syncer::ModelTypeChangeProcessor>.
> > I'm not sure why this wasn't picked up by the try bots..
> 
> The full error log:
> 
> [1821/8084] CXX
> obj/ios/chrome/browser/reading_list/unit_tests/reading_list_store_unittest.o
> FAILED:
> obj/ios/chrome/browser/reading_list/unit_tests/reading_list_store_unittest.o 
> /b/build/slave/cache/cipd/goma/gomacc clang++ -MMD -MF
> obj/ios/chrome/browser/reading_list/unit_tests/reading_list_store_unittest.o.d
> -DV8_DEPRECATION_WARNINGS -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD
> -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=284979-2
> -DCR_XCODE_VERSION=0800 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS
-D_DEBUG
> -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1
> -DGTEST_HAS_POSIX_RE=0 -DGTEST_LANG_CXX11=1 -DGOOGLE_PROTOBUF_NO_RTTI
> -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DHAVE_PTHREAD
-DU_USING_ICU_NAMESPACE=0
> -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION
> -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DUNIT_TEST -I../.. -Igen
> -I../../testing/gtest/include -I../../third_party/protobuf/src
-Igen/protoc_out
> -I../../third_party/protobuf/src -I../../third_party/ced/src
> -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n
> -I../../testing/gmock_custom -I../../testing/gmock/include
-fno-strict-aliasing
> --param=ssp-buffer-size=4 -fstack-protector -fcolor-diagnostics -arch x86_64
> -Wall -Werror -Wextra -Wno-missing-field-initializers -Wno-unused-parameter
> -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register
> -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override
> -Wno-shift-negative-value -O0 -gdwarf-2 -isysroot
>
/Applications/Xcode8.0.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator10.0.sdk
> -stdlib=libc++ -mios-simulator-version-min=9.0 -fvisibility=hidden
> -Wheader-hygiene -Wstring-conversion -Wno-unused-function
> -fno-threadsafe-statics -fvisibility-inlines-hidden
> -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare -std=c++11
> -fno-rtti -fno-exceptions -c
> ../../ios/chrome/browser/reading_list/reading_list_store_unittest.cc -o
> obj/ios/chrome/browser/reading_list/unit_tests/reading_list_store_unittest.o
> ../../ios/chrome/browser/reading_list/reading_list_store_unittest.cc:75:12:
> error: no viable conversion from returned value of type
> 'unique_ptr<TestModelTypeChangeProcessor,
> default_delete<TestModelTypeChangeProcessor>>' to function return type
> 'unique_ptr<syncer::ModelTypeChangeProcessor,
> default_delete<syncer::ModelTypeChangeProcessor>>'
>     return processor;
>            ^~~~~~~~~
>
/Applications/Xcode8.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2553:29:
> note: candidate constructor (the implicit copy constructor) not viable: no
known
> conversion from 'std::__1::unique_ptr<TestModelTypeChangeProcessor,
> std::__1::default_delete<TestModelTypeChangeProcessor> >' to 'const
> std::__1::unique_ptr<syncer::ModelTypeChangeProcessor,
> std::__1::default_delete<syncer::ModelTypeChangeProcessor> > &' for 1st
argument
> class _LIBCPP_TYPE_VIS_ONLY unique_ptr
>                             ^
>
/Applications/Xcode8.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2582:49:
> note: candidate constructor not viable: no known conversion from
> 'std::__1::unique_ptr<TestModelTypeChangeProcessor,
> std::__1::default_delete<TestModelTypeChangeProcessor> >' to 'nullptr_t' for
1st
> argument
>     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR unique_ptr(nullptr_t)
_NOEXCEPT
>                                                 ^
>
/Applications/Xcode8.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2609:31:
> note: candidate constructor not viable: no known conversion from
> 'std::__1::unique_ptr<TestModelTypeChangeProcessor,
> std::__1::default_delete<TestModelTypeChangeProcessor> >' to
> 'std::__1::unique_ptr<syncer::ModelTypeChangeProcessor,
> std::__1::default_delete<syncer::ModelTypeChangeProcessor> > &&' for 1st
> argument
>     _LIBCPP_INLINE_VISIBILITY unique_ptr(unique_ptr&& __u) _NOEXCEPT
>                               ^
>
/Applications/Xcode8.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2613:9:
> note: candidate constructor [with _Up = TestModelTypeChangeProcessor, _Ep =
> std::__1::default_delete<TestModelTypeChangeProcessor>] not viable: no known
> conversion from 'std::__1::unique_ptr<TestModelTypeChangeProcessor,
> std::__1::default_delete<TestModelTypeChangeProcessor> >' to
> 'unique_ptr<TestModelTypeChangeProcessor,
> std::__1::default_delete<TestModelTypeChangeProcessor> > &&' for 1st argument
>         unique_ptr(unique_ptr<_Up, _Ep>&& __u,
>         ^
>
/Applications/Xcode8.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2628:35:
> note: candidate template ignored: could not match 'auto_ptr' against
> 'unique_ptr'
>         _LIBCPP_INLINE_VISIBILITY unique_ptr(auto_ptr<_Up>&& __p,
>                                   ^
> 1 error generated.

https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xc...

[smut, 2016-11-18 02:22:43]

On 2016/11/18 01:07:36, Matt Giuca wrote:
> On 2016/11/18 01:07:11, Matt Giuca wrote:
> > On 2016/11/18 01:05:46, Matt Giuca wrote:
> > > A revert of this CL (patchset #20 id:380001) has been created in
> > > https://codereview.chromium.org/2507053005/ by mailto:mgiuca@chromium.org.
> > > 
> > > The reason for reverting is: Failing compile on ios-simulator-xcode-clang:
> > > 
> > > reading_list_store_unittest.cc:75:12: error: no viable conversion from
> > returned
> > > value of type 'unique_ptr<TestModelTypeChangeProcessor>' to function
return
> > type
> > > 'unique_ptr<syncer::ModelTypeChangeProcessor>'
> > >     return processor;
> > >            ^~~~~~~~~
> > > 
> > > It isn't legal to return a unique_ptr<B> from a function that returns
> > > unique_ptr<A> even if A is a base class. You need to explicitly declare
> > > |processor| to have type unique_ptr<syncer::ModelTypeChangeProcessor>.
> > > I'm not sure why this wasn't picked up by the try bots..

ios-simulator-xcode-clang is a CQ experiment, not part of the CQ. Failures on it
don't block CLs. Since this failed on ios-simulator-xcode-clang but succeeded on
the CQ try bot ios-simulator [1], I'm assuming this is legal in the version of
clang used by Chromium but not the version of clang supplied by Xcode?

[1]
https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bu...

[Matt Giuca, 2016-11-18 02:25:33]

On 2016/11/18 02:22:43, smut wrote:
> On 2016/11/18 01:07:36, Matt Giuca wrote:
> > On 2016/11/18 01:07:11, Matt Giuca wrote:
> > > On 2016/11/18 01:05:46, Matt Giuca wrote:
> > > > A revert of this CL (patchset #20 id:380001) has been created in
> > > > https://codereview.chromium.org/2507053005/ by
mailto:mgiuca@chromium.org.
> > > > 
> > > > The reason for reverting is: Failing compile on
ios-simulator-xcode-clang:
> > > > 
> > > > reading_list_store_unittest.cc:75:12: error: no viable conversion from
> > > returned
> > > > value of type 'unique_ptr<TestModelTypeChangeProcessor>' to function
> return
> > > type
> > > > 'unique_ptr<syncer::ModelTypeChangeProcessor>'
> > > >     return processor;
> > > >            ^~~~~~~~~
> > > > 
> > > > It isn't legal to return a unique_ptr<B> from a function that returns
> > > > unique_ptr<A> even if A is a base class. You need to explicitly declare
> > > > |processor| to have type unique_ptr<syncer::ModelTypeChangeProcessor>.
> > > > I'm not sure why this wasn't picked up by the try bots..
> 
> ios-simulator-xcode-clang is a CQ experiment, not part of the CQ. Failures on
it
> don't block CLs. Since this failed on ios-simulator-xcode-clang but succeeded
on
> the CQ try bot ios-simulator [1], I'm assuming this is legal in the version of
> clang used by Chromium but not the version of clang supplied by Xcode?
> 
> [1]
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bu...

I just tested this some more. You're probably right: this is legal C++11 but
fails on old versions of clang++ (3.4) and g++. It's likely that the ios
simulator is doing the wrong thing.

I didn't know it wasn't part of the CQ. But all the same, you can fix it by
adding std::move(processor) so you might try that.

[Olivier, 2016-11-18 07:40:11]

Description was changed from

==========
Add Store+Sync to reading list.

This CL creates a ModelTypeService for reading list.
This service will store persistent data for the Reading list model and sync
data between devices.

The previous store using NSUserDefaults will not be cleaned.
As it was only accessible from canary behind an experimental flag, no user
should be affected.

BUG=661668

Committed: https://crrev.com/faa080c524c6a80c2ac276359e29dec8cf7946c4
Cr-Commit-Position: refs/heads/master@{#432845}
==========

to

==========
Add Store+Sync to reading list.

This CL creates a ModelTypeService for reading list.
This service will store persistent data for the Reading list model and sync
data between devices.

The previous store using NSUserDefaults will not be cleaned.
As it was only accessible from canary behind an experimental flag, no user
should be affected.

BUG=661668

Committed: https://crrev.com/faa080c524c6a80c2ac276359e29dec8cf7946c4
Cr-Commit-Position: refs/heads/master@{#432845}
==========

[Olivier, 2016-11-18 07:54:01]

On 2016/11/18 02:25:33, Matt Giuca wrote:
> On 2016/11/18 02:22:43, smut wrote:
> > On 2016/11/18 01:07:36, Matt Giuca wrote:
> > > On 2016/11/18 01:07:11, Matt Giuca wrote:
> > > > On 2016/11/18 01:05:46, Matt Giuca wrote:
> > > > > A revert of this CL (patchset #20 id:380001) has been created in
> > > > > https://codereview.chromium.org/2507053005/ by
> mailto:mgiuca@chromium.org.
> > > > > 
> > > > > The reason for reverting is: Failing compile on
> ios-simulator-xcode-clang:
> > > > > 
> > > > > reading_list_store_unittest.cc:75:12: error: no viable conversion from
> > > > returned
> > > > > value of type 'unique_ptr<TestModelTypeChangeProcessor>' to function
> > return
> > > > type
> > > > > 'unique_ptr<syncer::ModelTypeChangeProcessor>'
> > > > >     return processor;
> > > > >            ^~~~~~~~~
> > > > > 
> > > > > It isn't legal to return a unique_ptr<B> from a function that returns
> > > > > unique_ptr<A> even if A is a base class. You need to explicitly
declare
> > > > > |processor| to have type unique_ptr<syncer::ModelTypeChangeProcessor>.
> > > > > I'm not sure why this wasn't picked up by the try bots..
> > 
> > ios-simulator-xcode-clang is a CQ experiment, not part of the CQ. Failures
on
> it
> > don't block CLs. Since this failed on ios-simulator-xcode-clang but
succeeded
> on
> > the CQ try bot ios-simulator [1], I'm assuming this is legal in the version
of
> > clang used by Chromium but not the version of clang supplied by Xcode?
> > 
> > [1]
> >
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bu...
> 
> I just tested this some more. You're probably right: this is legal C++11 but
> fails on old versions of clang++ (3.4) and g++. It's likely that the ios
> simulator is doing the wrong thing.
> 
> I didn't know it wasn't part of the CQ. But all the same, you can fix it by
> adding std::move(processor) so you might try that.

This is a bug in clang xcode, not in the code of this CL.

For the fix, you should not return std::move(processor) as this is preventing
compilation optimization.
The correct fix is to declare the unique_ptr as std::unique_ptr<A>

[Olivier, 2016-11-18 07:54:19]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2016-11-18 07:54:20]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org,
pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2451843002/#ps400001
(title: "fix xcode clang")

[commit-bot: I haz the power, 2016-11-18 07:54:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Olivier, 2016-11-18 07:55:29]

The CQ bit was unchecked by olivierrobin@chromium.org

[Olivier, 2016-11-18 08:09:10]

Patchset #21 (id:400001) has been deleted

[Olivier, 2016-11-18 08:12:02]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2016-11-18 08:12:03]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org,
pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2451843002/#ps420001
(title: "fix xcode clang")

[commit-bot: I haz the power, 2016-11-18 08:12:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 09:45:40]

Description was changed from

==========
Add Store+Sync to reading list.

This CL creates a ModelTypeService for reading list.
This service will store persistent data for the Reading list model and sync
data between devices.

The previous store using NSUserDefaults will not be cleaned.
As it was only accessible from canary behind an experimental flag, no user
should be affected.

BUG=661668

Committed: https://crrev.com/faa080c524c6a80c2ac276359e29dec8cf7946c4
Cr-Commit-Position: refs/heads/master@{#432845}
==========

to

==========
Add Store+Sync to reading list.

This CL creates a ModelTypeService for reading list.
This service will store persistent data for the Reading list model and sync
data between devices.

The previous store using NSUserDefaults will not be cleaned.
As it was only accessible from canary behind an experimental flag, no user
should be affected.

BUG=661668

Committed: https://crrev.com/faa080c524c6a80c2ac276359e29dec8cf7946c4
Cr-Commit-Position: refs/heads/master@{#432845}
==========

[commit-bot: I haz the power, 2016-11-18 09:45:42]

Committed patchset #21 (id:420001)

[commit-bot: I haz the power, 2016-11-18 09:48:03]

Description was changed from

==========
Add Store+Sync to reading list.

This CL creates a ModelTypeService for reading list.
This service will store persistent data for the Reading list model and sync
data between devices.

The previous store using NSUserDefaults will not be cleaned.
As it was only accessible from canary behind an experimental flag, no user
should be affected.

BUG=661668

Committed: https://crrev.com/faa080c524c6a80c2ac276359e29dec8cf7946c4
Cr-Commit-Position: refs/heads/master@{#432845}
==========

to

==========
Add Store+Sync to reading list.

This CL creates a ModelTypeService for reading list.
This service will store persistent data for the Reading list model and sync
data between devices.

The previous store using NSUserDefaults will not be cleaned.
As it was only accessible from canary behind an experimental flag, no user
should be affected.

BUG=661668

Committed: https://crrev.com/faa080c524c6a80c2ac276359e29dec8cf7946c4
Committed: https://crrev.com/01318f7c035d2f17ddbfc987b27db1dd6e6e1625
Cr-Original-Commit-Position: refs/heads/master@{#432845}
Cr-Commit-Position: refs/heads/master@{#433162}
==========

[commit-bot: I haz the power, 2016-11-18 09:48:05]

Patchset 21 (id:??) landed as
https://crrev.com/01318f7c035d2f17ddbfc987b27db1dd6e6e1625
Cr-Commit-Position: refs/heads/master@{#433162}

[Matt Giuca, 2016-11-20 23:26:31]

On 2016/11/18 09:48:05, commit-bot: I haz the power wrote:
> Patchset 21 (id:??) landed as
> https://crrev.com/01318f7c035d2f17ddbfc987b27db1dd6e6e1625
> Cr-Commit-Position: refs/heads/master@{#433162}

Hi Olivier,

> For the fix, you should not return std::move(processor) as this is preventing
> compilation optimization.

It looks like you did go with "std::move(processor)"... but anyway it's a test
so extreme optimization isn't that necessary.

Thanks for fixing this.