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

components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc

 // Copyright 2016 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/physical_web_pages/physical_web_page_suggestions_provider.h"
 
 #include <algorithm>
 #include <memory>
 #include <string>
 #include <utility>
@@ skipping 11 matching lines @@
 using base::DictionaryValue;
 using base::ListValue;
 using base::Value;
 
 namespace ntp_snippets {
 
 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(
     ContentSuggestionsProvider::Observer* observer,
     CategoryFactory* category_factory,
     physical_web::PhysicalWebDataSource* physical_web_data_source)
     : ContentSuggestionsProvider(observer, category_factory),
       category_status_(CategoryStatus::AVAILABLE),
       provided_category_(category_factory->FromKnownCategory(
           KnownCategories::PHYSICAL_WEB_PAGES)),
@@ skipping 10 matching lines @@
 
 CategoryStatus PhysicalWebPageSuggestionsProvider::GetCategoryStatus(
     Category category) {
   return category_status_;
 }
 
 CategoryInfo PhysicalWebPageSuggestionsProvider::GetCategoryInfo(
     Category category) {
   // TODO(vitaliii): Use the proper string once it has been agreed on.
   // TODO(vitaliii): Use a translateable string. (crbug.com/667764)
   // TODO(vitaliii): Implement More action. (crbug.com/667759)

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

Remove this comment now :)

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

Done.

   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,
       l10n_util::GetStringUTF16(IDS_NTP_SUGGESTIONS_SECTION_EMPTY));
 }
 
 void PhysicalWebPageSuggestionsProvider::DismissSuggestion(
     const ContentSuggestion::ID& suggestion_id) {
   // TODO(vitaliii): Implement this and then
   // ClearDismissedSuggestionsForDebugging. (crbug.com/667766)
 }
 
 void PhysicalWebPageSuggestionsProvider::FetchSuggestionImage(
     const ContentSuggestion::ID& suggestion_id,
     const ImageFetchedCallback& callback) {
   // TODO(vitaliii): Implement. (crbug.com/667765)
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(callback, gfx::Image()));
 }
 
 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(
     base::Time begin,
     base::Time end,
     const base::Callback<bool(const GURL& url)>& filter) {
   ClearDismissedSuggestionsForDebugging(provided_category_);
 }
 
 void PhysicalWebPageSuggestionsProvider::ClearCachedSuggestions(
@@ skipping 19 matching lines @@
 void PhysicalWebPageSuggestionsProvider::NotifyStatusChanged(
     CategoryStatus new_status) {
   if (category_status_ == new_status) {
     return;
   }
   category_status_ = new_status;
   observer()->OnCategoryStatusChanged(this, provided_category_, new_status);
 }
 
 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();
 
   std::vector<const DictionaryValue*> page_dictionaries;
   for (const std::unique_ptr<Value>& page_value : *page_values) {
     const DictionaryValue* page_dictionary;
     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(),
             [](const DictionaryValue* left, const DictionaryValue* right) {
               double left_distance, right_distance;
               bool success = left->GetDouble(physical_web::kDistanceEstimateKey,
                                              &left_distance);
               success = right->GetDouble(physical_web::kDistanceEstimateKey,
                                          &right_distance) &&
                         success;
               if (!success) {
                 LOG(DFATAL) << "Distance field is missing.";
               }
               return left_distance < right_distance;
             });
 
   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(
     const DictionaryValue& page) const {
   std::string scanned_url, raw_resolved_url, title, description;
   int scan_timestamp;
   bool success = page.GetString(physical_web::kScannedUrlKey, &scanned_url);
   success = page.GetInteger(physical_web::kScanTimestampKey, &scan_timestamp) &&
             success;
   success = page.GetString(physical_web::kResolvedUrlKey, &raw_resolved_url) &&
@@ skipping 30 matching lines @@
   FetchPhysicalWebPages();
 }
 
 void PhysicalWebPageSuggestionsProvider::OnDistanceChanged(
     const std::string& url,
     double distance_estimate) {
   FetchPhysicalWebPages();
 }
 
 }  // namespace ntp_snippets