Add logic for fetching the Reading List entries

components/ntp_snippets/reading_list/reading_list_suggestions_provider.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 "components/ntp_snippets/reading_list/reading_list_suggestions_provider.h"
 
 #include <vector>
 
 #include "base/bind.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/ntp_snippets/category.h"
 #include "components/reading_list/core/reading_list_entry.h"
 #include "components/reading_list/core/reading_list_model.h"
 #include "components/strings/grit/components_strings.h"
+#include "components/url_formatter/url_formatter.h"
 #include "ui/base/l10n/l10n_util.h"
 
+namespace {

[Marc Treib, 2017/03/28 13:25:15]

nit: Move into the ntp_snippets namespace?

[gambard, 2017/03/28 15:10:06]

Done.

+const int kMaxEntries = 3;
+}
+
 namespace ntp_snippets {
 
 ReadingListSuggestionsProvider::ReadingListSuggestionsProvider(
     ContentSuggestionsProvider::Observer* observer,
     ReadingListModel* reading_list_model)
     : ContentSuggestionsProvider(observer),
       category_status_(CategoryStatus::AVAILABLE_LOADING),
       provided_category_(
           Category::FromKnownCategory(KnownCategories::READING_LIST)),
       reading_list_model_(reading_list_model) {
   observer->OnCategoryStatusChanged(this, provided_category_, category_status_);
   reading_list_model->AddObserver(this);
-  if (reading_list_model_->loaded()) {

[Marc Treib, 2017/03/28 13:25:15]

Is this not needed anymore?

[gambard, 2017/03/28 15:10:06]

No, if the model is loaded when an observer is added, ReadingListModelLoaded()
is called on the observer. I have added a comment to specify it.

-    FetchReadingListInternal();
-  }
 }
 
 ReadingListSuggestionsProvider::~ReadingListSuggestionsProvider() {
-  reading_list_model_->RemoveObserver(this);
+  if (reading_list_model_)

[Marc Treib, 2017/03/28 13:25:15]

nit: braces please

optional: ScopedObserver might make this simpler

[gambard, 2017/03/28 15:10:06]

Thanks, I did not know about ScopedObserver!

+    reading_list_model_->RemoveObserver(this);
 }
 
 CategoryStatus ReadingListSuggestionsProvider::GetCategoryStatus(
     Category category) {
   DCHECK_EQ(category, provided_category_);
   return category_status_;
 }
 
 CategoryInfo ReadingListSuggestionsProvider::GetCategoryInfo(
     Category category) {
@@ skipping 54 matching lines @@
     Category category) {
   // TODO(crbug.com/702241): Implement this method.
 }
 
 void ReadingListSuggestionsProvider::ReadingListModelLoaded(
     const ReadingListModel* model) {
   DCHECK(model == reading_list_model_);
   FetchReadingListInternal();
 }
 
+void ReadingListSuggestionsProvider::ReadingListModelBeingDeleted(
+    const ReadingListModel* model) {
+  DCHECK(model == reading_list_model_);
+  reading_list_model_->RemoveObserver(this);
+  reading_list_model_ = nullptr;
+}
+
 void ReadingListSuggestionsProvider::FetchReadingListInternal() {
-  // TODO(crbug.com/702241): Implement this method.
+  if (!reading_list_model_)
+    return;
+
+  DCHECK(reading_list_model_->loaded());
+  std::vector<const ReadingListEntry*> entries;
+  for (const auto& url : reading_list_model_->Keys()) {

[Marc Treib, 2017/03/28 13:25:15]

s/auto/actual type/ ? IMO it's not clear what the type is otherwise.

[gambard, 2017/03/28 15:10:06]

Done.

+    const ReadingListEntry* entry = reading_list_model_->GetEntryByURL(url);
+    if (!entry->IsRead()) {

[Marc Treib, 2017/03/28 13:25:16]

optional: "if (entry->IsRead()) { continue; }" ?

[gambard, 2017/03/28 15:10:06]

Done.

+      bool added = false;
+      for (auto it = entries.begin(); it != entries.end(); ++it) {
+        if ((*it)->UpdateTime() < entry->UpdateTime()) {
+          entries.insert(it, entry);
+          added = true;
+          break;
+        }
+      }
+      if (!added && entries.size() < kMaxEntries) {
+        entries.push_back(entry);
+      }
+      if (entries.size() > kMaxEntries) {
+        entries.pop_back();
+      }

[Marc Treib, 2017/03/28 13:25:16]

Hm, IMO it's not really obvious what this is supposed to do, and that it
actually does it ;)
Some comments might help, or alternatively, maybe a simpler solution: Add all
(unread) entries, std::partial_sort them, and resize as necessary?

[gambard, 2017/03/28 15:10:07]

Done.

+    }
+  }
+
+  std::vector<ContentSuggestion> suggestions;
+  for (const ReadingListEntry* entry : entries) {
+    ContentSuggestion suggestion(provided_category_, entry->URL().spec(),
+                                 entry->URL());
+
+    if (entry->Title().size() > 0) {

[Marc Treib, 2017/03/28 13:25:16]

nit: !entry->Title().empty()

[gambard, 2017/03/28 15:10:07]

Done.

+      suggestion.set_title(base::UTF8ToUTF16(entry->Title()));
+    } else {
+      suggestion.set_title(url_formatter::FormatUrl(entry->URL()));
+    }
+    suggestion.set_snippet_text(
+        url_formatter::FormatUrl(entry->URL().GetOrigin()));

[Marc Treib, 2017/03/28 13:25:16]

If the URL is already used as the title, then this seems redundant. Maybe just
leave it empty in that case?

[gambard, 2017/03/28 15:10:07]

The question was to do this test in the provider or a the view level. Maybe it
makes more sense to have it a the view level.

[Marc Treib, 2017/03/28 15:31:29]

How would you check it there? After formatting, I don't know if a substring
check or something like that would be sufficient.

[gambard, 2017/03/28 15:42:49]

Sorry, I missread your comment. The URL is used in the title only if the title
is empty. If the page as a title, it is displayed as title.
In every case the domain name of the url is displayed as secondary text. So you
have:
[Title or full url]
[domain]

[Marc Treib, 2017/03/28 15:51:45]

So if the page doesn't have a title, you'd see:
www.publisher.com/article
www.publisher.com
which seems redundant to me. But not my decision I guess :)

But now that I look at this, snippet_text is almost certainly the wrong field to
set here. I think publisher_name would fit better.

[gambard, 2017/03/29 07:23:24]

Yes, you would see both. I looks better in the UI than in plain text here :)
Done.

+    suggestions.emplace_back(std::move(suggestion));
+  }
+
+  NotifyStatusChanged(CategoryStatus::AVAILABLE);
+  observer()->OnNewSuggestions(this, provided_category_,
+                               std::move(suggestions));
+}
+
+void ReadingListSuggestionsProvider::NotifyStatusChanged(
+    CategoryStatus new_status) {
+  if (category_status_ == new_status) {
+    return;
+  }
+  category_status_ = new_status;
+  observer()->OnCategoryStatusChanged(this, provided_category_, new_status);
 }
 
 }  // namespace ntp_snippets