Remove all ReadingList entries on managed account signout.

ios/chrome/browser/reading_list/reading_list_remover_helper.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ios/chrome/browser/reading_list/reading_list_remover_helper.h"
+
+#include "components/reading_list/ios/reading_list_model.h"
+#include "ios/chrome/browser/browser_state/chrome_browser_state.h"
+#include "ios/chrome/browser/reading_list/reading_list_download_service.h"
+#include "ios/chrome/browser/reading_list/reading_list_download_service_factory.h"
+#include "ios/chrome/browser/reading_list/reading_list_model_factory.h"
+
+namespace reading_list {
+
+ReadingListRemoverHelper::ReadingListRemoverHelper(
+    ios::ChromeBrowserState* browser_state)
+    : ReadingListModelObserver(), scoped_observer_(this) {
+  reading_list_model_ =
+      ReadingListModelFactory::GetForBrowserState(browser_state);
+  reading_list_download_service_ =
+      ReadingListDownloadServiceFactory::GetForBrowserState(browser_state);
+}
+
+ReadingListRemoverHelper::~ReadingListRemoverHelper() {}
+
+void ReadingListRemoverHelper::ReadingListModelLoaded(
+    const ReadingListModel* model) {
+  scoped_observer_.Remove(reading_list_model_);
+
+  bool model_cleared = reading_list_model_->DeleteAllEntries();
+  reading_list_download_service_->Clear();
+  if (completion_) {
+    completion_.Run(model_cleared);

[msarda, 2017/03/14 18:46:05]

How about std::move(completion_).Run()?

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

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

+    completion_ = Callback();
+  }
+  reading_list_model_ = nullptr;

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

Here and above: Consider resetting the model before calling the callback (IMHO,
calling the callback should be the last thing that should be done)

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

Done.

+}
+
+void ReadingListRemoverHelper::ReadingListModelBeingDeleted(
+    const ReadingListModel* model) {
+  scoped_observer_.Remove(reading_list_model_);
+  if (completion_) {
+    completion_.Run(false);
+    completion_ = Callback();
+  }
+  reading_list_model_ = nullptr;
+}
+
+void ReadingListRemoverHelper::RemoveAllUserReadingListItemsIOS(
+    Callback completion) {
+  if (!reading_list_model_) {

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

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.

[jif, 2017/03/14 19:00:21]

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]

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).

+    completion.Run(false);
+    return;
+  }
+  completion_ = std::move(completion);

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

Nit: Blank line for comment s that start at the same indent as the previous line
(easy reading in code review tool).

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

Done.

+  // This will call ReadingListModelLoaded if model is already loaded.

[msarda, 2017/03/14 18:46:05]

Optional nit: s/This will call/Calls

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

Done.

+  scoped_observer_.Add(reading_list_model_);
+}
+
+}  // namespace reading_list