Replace deprecated calls to GetMetadata in Physical Web Zine provider

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 <set>
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/grit/components_scaled_resources.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/pref_util.h"
+#include "components/physical_web/data_source/physical_web_data_source.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "grit/components_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/image/image.h"
 #include "url/gurl.h"
 
-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) {
-  std::string raw_resolved_url;
-  if (!page_dictionary.GetString(physical_web::kResolvedUrlKey,
-                                 &raw_resolved_url)) {
-    LOG(DFATAL) << physical_web::kResolvedUrlKey << " field is missing.";
-  }
-  return raw_resolved_url;
+std::string GetPageId(const physical_web::Metadata& page_metadata) {
+  return page_metadata.resolved_url.spec();

[vitaliii, 2017/01/18 08:03:34]

This looks so much better now, thank you for the new data interface!

[mattreynolds, 2017/01/18 22:26:12]

No problem!

 }
 
 }  // namespace
 
 PhysicalWebPageSuggestionsProvider::PhysicalWebPageSuggestionsProvider(
     ContentSuggestionsProvider::Observer* observer,
     physical_web::PhysicalWebDataSource* physical_web_data_source,
     PrefService* pref_service)
     : ContentSuggestionsProvider(observer),
       category_status_(CategoryStatus::AVAILABLE),
@@ skipping 72 matching lines @@
 
 void PhysicalWebPageSuggestionsProvider::ClearCachedSuggestions(
     Category category) {
   // Ignored
 }
 
 void PhysicalWebPageSuggestionsProvider::GetDismissedSuggestionsForDebugging(
     Category category,
     const DismissedSuggestionsCallback& callback) {
   DCHECK_EQ(provided_category_, category);
-  std::unique_ptr<ListValue> page_values =
-      physical_web_data_source_->GetMetadata();
+  std::unique_ptr<physical_web::MetadataList> page_metadata_list =
+      physical_web_data_source_->GetMetadataList();
   const std::set<std::string> dismissed_ids = ReadDismissedIDsFromPrefs();
   std::vector<ContentSuggestion> suggestions;
-  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.";
-      continue;
-    }
-
-    if (dismissed_ids.count(GetPageId(*page_dictionary))) {
-      suggestions.push_back(ConvertPhysicalWebPage(*page_dictionary));
+  for (const auto& page_metadata : *page_metadata_list) {
+    if (dismissed_ids.count(GetPageId(page_metadata))) {
+      suggestions.push_back(ConvertPhysicalWebPage(page_metadata));
     }
   }
 
   callback.Run(std::move(suggestions));
 }
 
 void PhysicalWebPageSuggestionsProvider::ClearDismissedSuggestionsForDebugging(
     Category category) {
   DCHECK_EQ(provided_category_, category);
   StoreDismissedIDsToPrefs(std::set<std::string>());
@@ skipping 23 matching lines @@
   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::unique_ptr<physical_web::MetadataList> page_metadata_list =
+      physical_web_data_source_->GetMetadataList();
 
   // These is to filter out dismissed suggestions and at the same time prune the
   // dismissed IDs list removing nonavailable pages (this is need since some
   // OnLost() calls may have been missed).
   const std::set<std::string> old_dismissed_ids = ReadDismissedIDsFromPrefs();
   std::set<std::string> new_dismissed_ids;
-  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.";
-      continue;
-    }
-
-    const std::string page_id = GetPageId(*page_dictionary);
+  physical_web::MetadataList filtered_metadata_list;
+  for (const auto& page_metadata : *page_metadata_list) {
+    const std::string page_id = GetPageId(page_metadata);
     if (!excluded_ids.count(page_id) && !old_dismissed_ids.count(page_id)) {
-      page_dictionaries.push_back(page_dictionary);
+      filtered_metadata_list.push_back(page_metadata);
     }
 
     if (old_dismissed_ids.count(page_id)) {
       new_dismissed_ids.insert(page_id);
     }
   }
 
   if (old_dismissed_ids.size() != new_dismissed_ids.size()) {
     StoreDismissedIDsToPrefs(new_dismissed_ids);
   }
 
-  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::sort(filtered_metadata_list.begin(), filtered_metadata_list.end(),
+            [](const physical_web::Metadata& left,
+               const physical_web::Metadata& right) {
+              return left.distance_estimate < right.distance_estimate;

[vitaliii, 2017/01/18 08:03:34]

Since now it is known that distance_estimate may be <= 0, could you compare them
such that <= 0 ones are last? Also adding a test would be nice.

You could achieve this by e.g.:
bool is_left_estimated = left.distance_estimate > 0;
bool is_right_estimated = right.distance_estimate > 0;
if (is_left_estimated != is_right_estimated) return is_left_estimated;
return left.distance_estimate < right.distance_estimate;

[vitaliii, 2017/01/18 11:50:50]

I am actually addressing this in https://codereview.chromium.org/2640773002/,
since you do not have to land yours before BP.
If it lands first, feel free to ignore the comment above.

[mattreynolds, 2017/01/18 22:26:12]

Thanks for the heads up. Looks like yours has landed so I've rebased on top of
it.

             });
 
   std::vector<ContentSuggestion> suggestions;
-  for (const DictionaryValue* page_dictionary : page_dictionaries) {
+  for (const auto& page_metadata : filtered_metadata_list) {
     if (static_cast<int>(suggestions.size()) == max_quantity) {
       break;
     }
-    suggestions.push_back(ConvertPhysicalWebPage(*page_dictionary));
+    suggestions.push_back(ConvertPhysicalWebPage(page_metadata));
   }
 
   return suggestions;
 }
 
 ContentSuggestion PhysicalWebPageSuggestionsProvider::ConvertPhysicalWebPage(
-    const DictionaryValue& page) const {
-  std::string scanned_url, raw_resolved_url, title, description;
-  bool success = page.GetString(physical_web::kScannedUrlKey, &scanned_url);
-  success = page.GetString(physical_web::kResolvedUrlKey, &raw_resolved_url) &&
-            success;
-  success = page.GetString(physical_web::kTitleKey, &title) && success;
-  success =
-      page.GetString(physical_web::kDescriptionKey, &description) && success;
-  if (!success) {
-    LOG(DFATAL) << "Expected field is missing.";
-  }
-
-  const GURL resolved_url(raw_resolved_url);
+    const physical_web::Metadata& page) const {
   ContentSuggestion suggestion(provided_category_, GetPageId(page),
-                               resolved_url);
-  DCHECK(base::IsStringUTF8(title));
-  suggestion.set_title(base::UTF8ToUTF16(title));
-  suggestion.set_publisher_name(base::UTF8ToUTF16(resolved_url.host()));
-  DCHECK(base::IsStringUTF8(description));
-  suggestion.set_snippet_text(base::UTF8ToUTF16(description));
+                               page.resolved_url);
+  DCHECK(base::IsStringUTF8(page.title));
+  suggestion.set_title(base::UTF8ToUTF16(page.title));
+  suggestion.set_publisher_name(base::UTF8ToUTF16(page.resolved_url.host()));
+  DCHECK(base::IsStringUTF8(page.description));
+  suggestion.set_snippet_text(base::UTF8ToUTF16(page.description));
   return suggestion;
 }
 
 // PhysicalWebListener implementation.
 void PhysicalWebPageSuggestionsProvider::OnFound(const GURL& url) {
   FetchPhysicalWebPages();
 }
 
 void PhysicalWebPageSuggestionsProvider::OnLost(const GURL& url) {
   InvalidateSuggestion(url.spec());
@@ skipping 26 matching lines @@
 }
 
 void PhysicalWebPageSuggestionsProvider::StoreDismissedIDsToPrefs(
     const std::set<std::string>& dismissed_ids) {
   prefs::StoreDismissedIDsToPrefs(pref_service_,
                                   prefs::kDismissedPhysicalWebPageSuggestions,
                                   dismissed_ids);
 }
 
 }  // namespace ntp_snippets