Remove all ReadingList entries on managed account signout.

Remove all ReadingList entries on managed account signout.

Clearing happens after the actual signout, so only local data is deleted.
Also interrupt current distillation on signout as profile may change.

BUG=699590
Review-Url: https://codereview.chromium.org/2745313004
Cr-Commit-Position: refs/heads/master@{#458377}
Committed: https://chromium.googlesource.com/chromium/src/+/85c6bf63ddea1f0a18f2404090acb96f67369ca2

[Olivier, 2017-03-14 12:51:37]

Description was changed from

==========
Remove all ReadingList entries on managed account signout.

Clearing happens after the actual signout, so only local data is deleted.

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

to

==========
Remove all ReadingList entries on managed account signout.

Clearing happens after the actual signout, so only local data is deleted.
Also interrupt current distillation on signout as profile may change.

BUG=699590
==========

[Olivier, 2017-03-14 12:52:01]

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

[jif, 2017-03-14 15:38:43]

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

https://codereview.chromium.org/2745313004/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:17: :
ReadingListModelObserver(), completion_(nullptr) {
Default constructor of base::Callback already initializes with nullptr.

https://codereview.chromium.org/2745313004/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:44: }
reading_list_model_ = nullptr;

https://codereview.chromium.org/2745313004/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:55:
reading_list_model_->AddObserver(this);
What happens if |this| is deleted after |RemoveAllUserReadingListItemsIOS| is
called, but before |ReadingListModelLoaded| is called?
I think we need to use a ScopedObserver.

https://codereview.chromium.org/2745313004/diff/80001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_remover_helper.h (right):

https://codereview.chromium.org/2745313004/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_remover_helper.h:34:
DISALLOW_COPY_AND_ASSIGN(ReadingListRemoverHelper);
DISALLOW_COPY_AND_ASSIGN( goes at the end of the private section, and is
preceded by a new line.

[Olivier, 2017-03-14 17:56:20]

Thanks

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

https://codereview.chromium.org/2745313004/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:17: :
ReadingListModelObserver(), completion_(nullptr) {
On 2017/03/14 15:38:43, jif wrote:
> Default constructor of base::Callback already initializes with nullptr.

Done.

https://codereview.chromium.org/2745313004/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:44: }
On 2017/03/14 15:38:43, jif wrote:
> reading_list_model_ = nullptr;

Done.

https://codereview.chromium.org/2745313004/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:55:
reading_list_model_->AddObserver(this);
On 2017/03/14 15:38:43, jif wrote:
> What happens if |this| is deleted after |RemoveAllUserReadingListItemsIOS| is
> called, but before |ReadingListModelLoaded| is called?
> I think we need to use a ScopedObserver.

Done.

https://codereview.chromium.org/2745313004/diff/80001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_remover_helper.h (right):

https://codereview.chromium.org/2745313004/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_remover_helper.h:34:
DISALLOW_COPY_AND_ASSIGN(ReadingListRemoverHelper);
On 2017/03/14 15:38:43, jif wrote:
> DISALLOW_COPY_AND_ASSIGN( goes at the end of the private section, and is
> preceded by a new line.

Oups, thanks.

[Olivier, 2017-03-14 17:56:43]

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

[jif, 2017-03-14 18:05:13]

lgtm

[msarda, 2017-03-14 18:46:06]

Looks good overall. Some comments and nits.

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_remover_helper.cc (right):

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:33:
completion_.Run(model_cleared);
How about std::move(completion_).Run()?

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:36:
reading_list_model_ = nullptr;
Here and above: Consider resetting the model before calling the callback (IMHO,
calling the callback should be the last thing that should be done)

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:51: if
(!reading_list_model_) {
I suppose reading_list_model_ can only be Null if
RemoveAllUserReadingListItemsIOS is called twice on the same
ReadingListRemoverHelper. I would say this should be illegal, so I would replace
this with a DCHECK.

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:55: completion_ =
std::move(completion);
Nit: Blank line for comment s that start at the same indent as the previous line
(easy reading in code review tool).

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:56: // This will
call ReadingListModelLoaded if model is already loaded.
Optional nit: s/This will call/Calls

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_remover_helper.h (right):

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_remover_helper.h:22: using Callback
= base::Callback<void(bool)>;
Consider using a OnceCallback (see
https://cs.chromium.org/chromium/src/base/callback_forward.h?rcl=b6eb9203f08c...
)

[jif, 2017-03-14 19:00:22]

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_remover_helper.cc (right):

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:33:
completion_.Run(model_cleared);
On 2017/03/14 18:46:05, msarda wrote:
> How about std::move(completion_).Run()?

std::move turns a lvalue into a rvalue.
In this case there's no point in having a rvalue, so std::move is unnecessary.

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:51: if
(!reading_list_model_) {
On 2017/03/14 18:46:06, msarda wrote:
> I suppose reading_list_model_ can only be Null if
> RemoveAllUserReadingListItemsIOS is called twice on the same
> ReadingListRemoverHelper. I would say this should be illegal, so I would
replace
> this with a DCHECK.

reading_list_model_ can also be null if |ReadingListModelBeingDeleted| is called
before |RemoveAllUserReadingListItemsIOS| is called.
It's true that this is not possible right now (because
RemoveAllUserReadingListItemsIOS happens to called right after the object is
created), but could be in the future.
One way to solve this would be to fold |RemoveAllUserReadingListItemsIOS| into
the constructor.

I think this would have the side benefit of us not needing the ivar
|reading_list_model_| anymore because the ReadingListModel is passed to us as a
parameter.

[Olivier, 2017-03-15 09:17:40]

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_remover_helper.cc (right):

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:36:
reading_list_model_ = nullptr;
On 2017/03/14 18:46:06, msarda wrote:
> Here and above: Consider resetting the model before calling the callback
(IMHO,
> calling the callback should be the last thing that should be done)

Done.

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:51: if
(!reading_list_model_) {
On 2017/03/14 19:00:21, jif wrote:
> On 2017/03/14 18:46:06, msarda wrote:
> > I suppose reading_list_model_ can only be Null if
> > RemoveAllUserReadingListItemsIOS is called twice on the same
> > ReadingListRemoverHelper. I would say this should be illegal, so I would
> replace
> > this with a DCHECK.
> 
> reading_list_model_ can also be null if |ReadingListModelBeingDeleted| is
called
> before |RemoveAllUserReadingListItemsIOS| is called.
> It's true that this is not possible right now (because
> RemoveAllUserReadingListItemsIOS happens to called right after the object is
> created), but could be in the future.
> One way to solve this would be to fold |RemoveAllUserReadingListItemsIOS| into
> the constructor.
> 
> I think this would have the side benefit of us not needing the ivar
> |reading_list_model_| anymore because the ReadingListModel is passed to us as
a
> parameter.

I don't want to put this in the constructor as potentially, completion_ will
delete the object synchronously (it is not the fact today, but it may happen).

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:55: completion_ =
std::move(completion);
On 2017/03/14 18:46:06, msarda wrote:
> Nit: Blank line for comment s that start at the same indent as the previous
line
> (easy reading in code review tool).

Done.

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:56: // This will
call ReadingListModelLoaded if model is already loaded.
On 2017/03/14 18:46:05, msarda wrote:
> Optional nit: s/This will call/Calls

Done.

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_remover_helper.h (right):

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_remover_helper.h:22: using Callback
= base::Callback<void(bool)>;
On 2017/03/14 18:46:06, msarda wrote:
> Consider using a OnceCallback (see
>
https://cs.chromium.org/chromium/src/base/callback_forward.h?rcl=b6eb9203f08c...
> )

I will take a look to OnceCallback

[msarda, 2017-03-16 00:06:03]

LGTM pending the change for OnceCallback.

[Olivier, 2017-03-16 11:56:34]

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_remover_helper.h (right):

https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_remover_helper.h:22: using Callback
= base::Callback<void(bool)>;
On 2017/03/14 18:46:06, msarda wrote:
> Consider using a OnceCallback (see
>
https://cs.chromium.org/chromium/src/base/callback_forward.h?rcl=b6eb9203f08c...
> )

Done.

[Olivier, 2017-03-20 13:55:40]

On 2017/03/16 11:56:34, Olivier Robin wrote:
>
https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
> File ios/chrome/browser/reading_list/reading_list_remover_helper.h (right):
> 
>
https://codereview.chromium.org/2745313004/diff/100001/ios/chrome/browser/rea...
> ios/chrome/browser/reading_list/reading_list_remover_helper.h:22: using
Callback
> = base::Callback<void(bool)>;
> On 2017/03/14 18:46:06, msarda wrote:
> > Consider using a OnceCallback (see
> >
>
https://cs.chromium.org/chromium/src/base/callback_forward.h?rcl=b6eb9203f08c...
> > )
> 
> Done.

Hi sdefresne,

Could you take a look?
Thanks.

[sdefresne, 2017-03-20 15:09:41]

lgtm

https://codereview.chromium.org/2745313004/diff/140001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.mm
(right):

https://codereview.chromium.org/2745313004/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.mm:62:
web_states_.push_back(std::move(std::move(web_state_unique)));
std:move(std:move(...)) -> std::move(...)

https://codereview.chromium.org/2745313004/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.mm:65:
dispatch_after(
optional: maybe add a comment explaining that this block will delete web_state
in keep_alive_second_ if it is not reused until then.

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

https://codereview.chromium.org/2745313004/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:52:
std::move(completion_).Run(false);
Maybe add the following comment here too:

  // |completion_| may delete this. Do not use the object after this call.

https://codereview.chromium.org/2745313004/diff/140001/ios/chrome/browser/sig...
File ios/chrome/browser/signin/browser_state_data_remover.mm (right):

https://codereview.chromium.org/2745313004/diff/140001/ios/chrome/browser/sig...
ios/chrome/browser/signin/browser_state_data_remover.mm:65: // Remove bookmarks
once all browsing data was removed.
Maybe change this to "Remove bookmarks and read list data once all browsing data
was removed.".

[Olivier, 2017-03-21 10:10:15]

Thanks

https://codereview.chromium.org/2745313004/diff/140001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.mm
(right):

https://codereview.chromium.org/2745313004/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.mm:62:
web_states_.push_back(std::move(std::move(web_state_unique)));
On 2017/03/20 15:09:40, sdefresne wrote:
> std:move(std:move(...)) -> std::move(...)

Done.

https://codereview.chromium.org/2745313004/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.mm:65:
dispatch_after(
On 2017/03/20 15:09:40, sdefresne wrote:
> optional: maybe add a comment explaining that this block will delete web_state
> in keep_alive_second_ if it is not reused until then.

Done.

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

https://codereview.chromium.org/2745313004/diff/140001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_remover_helper.cc:52:
std::move(completion_).Run(false);
On 2017/03/20 15:09:40, sdefresne wrote:
> Maybe add the following comment here too:
> 
>   // |completion_| may delete this. Do not use the object after this call.

Done.

https://codereview.chromium.org/2745313004/diff/140001/ios/chrome/browser/sig...
File ios/chrome/browser/signin/browser_state_data_remover.mm (right):

https://codereview.chromium.org/2745313004/diff/140001/ios/chrome/browser/sig...
ios/chrome/browser/signin/browser_state_data_remover.mm:65: // Remove bookmarks
once all browsing data was removed.
On 2017/03/20 15:09:40, sdefresne wrote:
> Maybe change this to "Remove bookmarks and read list data once all browsing
data
> was removed.".

Done.

[Olivier, 2017-03-21 10:10:30]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-03-21 10:10:31]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org,
msarda@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2745313004/#ps160001
(title: "feedback")

[commit-bot: I haz the power, 2017-03-21 10:10:50]

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, 2017-03-21 12:03:19]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490091030898820,
"parent_rev": "951df7be3b2b83f682351f98300354f7a2807be9", "commit_rev":
"85c6bf63ddea1f0a18f2404090acb96f67369ca2"}

[commit-bot: I haz the power, 2017-03-21 12:04:00]

Description was changed from

==========
Remove all ReadingList entries on managed account signout.

Clearing happens after the actual signout, so only local data is deleted.
Also interrupt current distillation on signout as profile may change.

BUG=699590
==========

to

==========
Remove all ReadingList entries on managed account signout.

Clearing happens after the actual signout, so only local data is deleted.
Also interrupt current distillation on signout as profile may change.

BUG=699590

Review-Url: https://codereview.chromium.org/2745313004
Cr-Commit-Position: refs/heads/master@{#458377}
Committed:
https://chromium.googlesource.com/chromium/src/+/85c6bf63ddea1f0a18f2404090ac...
==========

[commit-bot: I haz the power, 2017-03-21 12:04:01]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/85c6bf63ddea1f0a18f2404090ac...