[NTP::PhysicalWeb] Implement Fetch for "More" button.

components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc

Index: components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
diff --git a/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc b/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
index f6e8084cc6de942a12a0df650b0d057015b87f46..4fb6dc38591faee46eba9ac002011de8872f6fa3 100644
--- a/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
+++ b/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
@@ -29,6 +29,15 @@ namespace {
 
 const size_t kMaxSuggestionsCount = 10;
 
+std::string GetPageId(const DictionaryValue* page_dictionary) {

[Marc Treib, 2016/12/05 15:55:19]

If it can't be null, reference rather than pointer.

[vitaliii, 2016/12/06 07:30:20]

Done.

+  std::string raw_resolved_url;
+  if (!page_dictionary->GetString(physical_web::kResolvedUrlKey,
+                                  &raw_resolved_url)) {
+    LOG(DFATAL) << "kResolvedUrlKey field is missing.";

[Marc Treib, 2016/12/05 15:55:19]

Use the actual value rather than "kResolvedUrlKey"?

[vitaliii, 2016/12/06 07:30:20]

Done.

+  }
+  return GURL(raw_resolved_url).spec();

[Marc Treib, 2016/12/05 15:55:19]

This converts to GURL and back to string. That does *some* normalization (e.g.
add a trailing slash in some cases). If that's intended, please add a comment.

[vitaliii, 2016/12/06 07:30:20]

Done.

+}
+
 }  // namespace
 
 PhysicalWebPageSuggestionsProvider::PhysicalWebPageSuggestionsProvider(
@@ -63,7 +72,7 @@ CategoryInfo PhysicalWebPageSuggestionsProvider::GetCategoryInfo(
   return CategoryInfo(
       base::ASCIIToUTF16("Physical web pages"),
       ContentSuggestionsCardLayout::FULL_CARD,
-      /*has_more_action=*/false,
+      /*has_more_action=*/true,
       /*has_reload_action=*/false,
       /*has_view_all_action=*/false,
       /*show_if_empty=*/false,
@@ -88,14 +97,12 @@ void PhysicalWebPageSuggestionsProvider::Fetch(
     const Category& category,
     const std::set<std::string>& known_suggestion_ids,
     const FetchDoneCallback& callback) {
-  LOG(DFATAL)
-      << "PhysicalWebPageSuggestionsProvider has no |Fetch| functionality!";
+  DCHECK_EQ(category, provided_category_);
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
-      base::Bind(callback, Status(StatusCode::PERMANENT_ERROR,
-                                  "PhysicalWebPageSuggestionsProvider "
-                                  "has no |Fetch| functionality!"),
-                 base::Passed(std::vector<ContentSuggestion>())));
+      base::Bind(callback, Status(StatusCode::SUCCESS),
+                 base::Passed(GetMostRecentPhysicalWebPagesWithFilter(
+                     kMaxSuggestionsCount, known_suggestion_ids))));
 }
 
 void PhysicalWebPageSuggestionsProvider::ClearHistory(
@@ -136,6 +143,16 @@ void PhysicalWebPageSuggestionsProvider::NotifyStatusChanged(
 
 void PhysicalWebPageSuggestionsProvider::FetchPhysicalWebPages() {
   NotifyStatusChanged(CategoryStatus::AVAILABLE);

[Marc Treib, 2016/12/05 15:55:19]

Not related to this CL, but isn't the status always AVAILABLE anyway?

[vitaliii, 2016/12/06 07:30:20]

It is, but we may start setting it to something else.

[Marc Treib, 2016/12/06 10:01:34]

That's not a good reason to keep unnecessary code there.
DCHECK instead?

[vitaliii, 2016/12/06 12:45:52]

Done.

+  observer()->OnNewSuggestions(this, provided_category_,
+                               GetMostRecentPhysicalWebPagesWithFilter(
+                                   kMaxSuggestionsCount,
+                                   /*excluded_ids=*/std::set<std::string>()));
+}
+
+std::vector<ContentSuggestion>
+PhysicalWebPageSuggestionsProvider::GetMostRecentPhysicalWebPagesWithFilter(
+    int max_quantity,
+    const std::set<std::string>& excluded_ids) {
   std::unique_ptr<ListValue> page_values =
       physical_web_data_source_->GetMetadata();
 
@@ -145,7 +162,9 @@ void PhysicalWebPageSuggestionsProvider::FetchPhysicalWebPages() {
     if (!page_value->GetAsDictionary(&page_dictionary)) {
       LOG(DFATAL) << "Physical Web page is not a dictionary.";
     }
-    page_dictionaries.push_back(page_dictionary);
+    if (!excluded_ids.count(GetPageId(page_dictionary))) {
+      page_dictionaries.push_back(page_dictionary);
+    }
   }
 
   std::sort(page_dictionaries.begin(), page_dictionaries.end(),
@@ -165,13 +184,12 @@ void PhysicalWebPageSuggestionsProvider::FetchPhysicalWebPages() {
   std::vector<ContentSuggestion> suggestions;
   for (const DictionaryValue* page_dictionary : page_dictionaries) {
     suggestions.push_back(ConvertPhysicalWebPage(*page_dictionary));
-    if (suggestions.size() == kMaxSuggestionsCount) {
+    if (static_cast<int>(suggestions.size()) == max_quantity) {

[Marc Treib, 2016/12/05 15:55:19]

This assumes max_quantity >= 1. Calling this with 0 isn't totally unreasonable,
so maybe handle this case, or DCHECK?

[vitaliii, 2016/12/06 07:30:20]

Done.

       break;
     }
   }
 
-  observer()->OnNewSuggestions(this, provided_category_,
-                               std::move(suggestions));
+  return suggestions;
 }
 
 ContentSuggestion PhysicalWebPageSuggestionsProvider::ConvertPhysicalWebPage(