[NTP::PhysicalWeb] Filter our pages with same group_id.

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 147cfdb4aec14661853ca1cdda1e41a0a49e00d0..0d0ea87618d33cbc60bcd913f12df368f3b7144b 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
@@ -46,6 +46,81 @@ std::string GetPageId(const DictionaryValue& page_dictionary) {
   return raw_resolved_url;
 }
 
+bool CompareByDistanceClosestFirst(const DictionaryValue* left,

[Marc Treib, 2017/01/18 11:24:01]

nit: "ClosestFirst" IMO isn't necessary to state, I'd assume that by default
(standard numerical ordering)

[vitaliii, 2017/01/18 11:48:15]

Done.

+                                   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.";
+  }
+
+  // When there is no estimate, the value is <= 0, so we need to treat this case
+  // manually.

[Marc Treib, 2017/01/18 11:24:01]

nit: Treat how, exactly?
Or maybe actually just assign infinity to the corresponding double variable?

[vitaliii, 2017/01/18 11:48:16]

Do you how to get infinity for double?

I rewrote the comment, it may do now.

[Marc Treib, 2017/01/18 11:54:47]

std::numeric_limits<double>::infinity();
(or there's also some c-style constant for it, but I forget)

[vitaliii, 2017/01/18 14:02:27]

Acknowledged.

+  bool is_left_estimated = left_distance > 0;
+  bool is_right_estimated = right_distance > 0;
+
+  if (is_left_estimated != is_right_estimated)
+    return is_left_estimated;
+  return left_distance < right_distance;
+}
+
+void FilterOutByGroupId(
+    std::vector<const DictionaryValue*>* page_dictionaries) {
+  // |std::unique| only removes duplicates that immediately follow each other.
+  // Thus, first, we have to sort by group_id and distance and only then remove
+  // duplicates.
+  std::sort(page_dictionaries->begin(), page_dictionaries->end(),
+            [](const DictionaryValue* left, const DictionaryValue* right) {
+              std::string left_group_id, right_group_id;
+              bool success =
+                  left->GetString(physical_web::kGroupIdKey, &left_group_id);
+              success = right->GetString(physical_web::kGroupIdKey,
+                                         &right_group_id) &&
+                        success;
+              if (!success) {
+                LOG(DFATAL) << "Group id field is missing.";

[Marc Treib, 2017/01/18 11:24:01]

So the field must always be there, even if it's empty?

[vitaliii, 2017/01/18 11:48:16]

As experiments show, yes.

This won't be a problem after https://codereview.chromium.org/2643453002/. 

+              }
+
+              if (left_group_id != right_group_id) {
+                return left_group_id < right_group_id;
+              }
+
+              // We want closest pages first, so in case of same group_id we
+              // sort by distance.
+              return CompareByDistanceClosestFirst(left, right);
+            });
+
+  // Empty group_id must be treated as unique, so we do not apply std::unique to

[Marc Treib, 2017/01/18 11:24:01]

nit: *Each* empty group id must be...

[vitaliii, 2017/01/18 11:48:16]

Done.

+  // them at all.
+  std::vector<const DictionaryValue*>::iterator nonempty_group_id_begin =

[Marc Treib, 2017/01/18 11:24:01]

optional: You could use auto here

[vitaliii, 2017/01/18 11:48:16]

Done.

+      page_dictionaries->begin();
+  for (; nonempty_group_id_begin != page_dictionaries->end();

[Marc Treib, 2017/01/18 11:24:01]

Since you're already using std algorithms quite heavily: This could probably be
written as an std::find_if.

[vitaliii, 2017/01/18 11:48:16]

Done.

Good observation.

+       ++nonempty_group_id_begin) {
+    std::string group_id;
+
+    (*nonempty_group_id_begin)->GetString(physical_web::kGroupIdKey, &group_id);
+    if (!group_id.empty()) {
+      break;
+    }
+  }
+
+  std::vector<const DictionaryValue*>::iterator new_end = std::unique(

[Marc Treib, 2017/01/18 11:24:01]

optional: auto

[vitaliii, 2017/01/18 11:48:15]

Done.

+      nonempty_group_id_begin, page_dictionaries->end(),
+      [](const DictionaryValue* left, const DictionaryValue* right) {
+        std::string left_group_id, right_group_id;
+
+        left->GetString(physical_web::kGroupIdKey, &left_group_id);
+        right->GetString(physical_web::kGroupIdKey, &right_group_id);
+        return left_group_id == right_group_id;
+      });
+
+  page_dictionaries->erase(new_end, page_dictionaries->end());
+}
+
 }  // namespace
 
 PhysicalWebPageSuggestionsProvider::PhysicalWebPageSuggestionsProvider(
@@ -222,19 +297,10 @@ PhysicalWebPageSuggestionsProvider::GetMostRecentPhysicalWebPagesWithFilter(
     StoreDismissedIDsToPrefs(new_dismissed_ids);
   }
 
+  FilterOutByGroupId(&page_dictionaries);
+
   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;
-            });
+            CompareByDistanceClosestFirst);
 
   std::vector<ContentSuggestion> suggestions;
   for (const DictionaryValue* page_dictionary : page_dictionaries) {