Fix Visibility Score MergeFrom() Logic

When this' score is -1 and the incoming score is > 0, the incoming
score is not merged to `this`. Fixed by checking for that case, and
added tests for all cases.

(cherry picked from commit 0a18650)

Bug: 1411063
Change-Id: I9dd381b4237698b020f87f5038f229b964115898
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4201278
Reviewed-by: Sophie Chang <sophiechang@chromium.org>
Auto-Submit: Robert Ogden <robertogden@chromium.org>
Commit-Queue: Robert Ogden <robertogden@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1098349}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4203570
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Cr-Commit-Position: refs/branch-heads/5481@{#803}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}

components/history/core/browser/url_row.cc

@@ -12,20 +12,17 @@
 
 namespace history {
 
-URLRow::URLRow() {
-}
+URLRow::URLRow() = default;
 
-URLRow::URLRow(const GURL& url) : url_(url) {
-}
+URLRow::URLRow(const GURL& url) : url_(url) {}
 
 URLRow::URLRow(const GURL& url, URLID id) : id_(id), url_(url) {}
 
 URLRow::URLRow(const URLRow& other) = default;
 
 URLRow::URLRow(URLRow&& other) noexcept = default;
 
-URLRow::~URLRow() {
-}
+URLRow::~URLRow() = default;
 
 URLRow& URLRow::operator=(const URLRow& other) = default;
 URLRow& URLRow::operator=(URLRow&& other) noexcept = default;
@@ -115,9 +112,10 @@ void VisitContentModelAnnotations::MergeFrom(
     const VisitContentModelAnnotations& other) {
   // To be conservative, we use the lesser of the two visibility scores, but
   // ignore sentinel values (which are negative).
-  if (other.visibility_score >= 0 &&
-      other.visibility_score < visibility_score) {
-    visibility_score = other.visibility_score;
+  if ((this->visibility_score < 0 && other.visibility_score >= 0) ||
+      (this->visibility_score >= 0 && other.visibility_score >= 0 &&
+       other.visibility_score < this->visibility_score)) {
+    this->visibility_score = other.visibility_score;
   }
 
   for (auto& other_category : other.categories) {
@@ -167,11 +165,9 @@ URLResult::URLResult(URLResult&& other) noexcept
       content_annotations_(other.content_annotations_),
       snippet_(std::move(other.snippet_)),
       title_match_positions_(std::move(other.title_match_positions_)),
-      blocked_visit_(other.blocked_visit_) {
-}
+      blocked_visit_(other.blocked_visit_) {}
 
-URLResult::~URLResult() {
-}
+URLResult::~URLResult() = default;
 
 URLResult& URLResult::operator=(const URLResult&) = default;
 

components/history/core/browser/url_row_unittest.cc

@@ -34,6 +34,73 @@ TEST(HistoryUrlRowTest, MergeCategoryIntoVector) {
                   VisitContentModelAnnotations::Category("category3", 30)));
 }
 
+TEST(HistoryUrlRowTest, MergeVisibilityScores) {
+  struct TestCase {
+    std::string label;
+    double starting_score;
+    double merge_score;
+    double want_score;
+  };
+  struct TestCase tests[] = {
+      {
+          // Regression test for http://crbug.com/1411063
+          .label = "Default score is overwritten",
+          .starting_score = -1,
+          .merge_score = 0.5,
+          .want_score = 0.5,
+      },
+      {
+          .label = "Default score is overwritten with 0",
+          .starting_score = -1,
+          .merge_score = 0,
+          .want_score = 0,
+      },
+      {
+          .label = "Set score is not overwritten",
+          .starting_score = 0.5,
+          .merge_score = -1,
+          .want_score = 0.5,
+      },
+      {
+          .label = "Set score of 0 is not overwritten",
+          .starting_score = 0,
+          .merge_score = -1,
+          .want_score = 0,
+      },
+      {
+          .label = "Takes the lowest of two set scores, merge is lower",
+          .starting_score = 0.5,
+          .merge_score = 0,
+          .want_score = 0,
+      },
+      {
+          .label = "Takes the lowest of two set scores, starting is lower",
+          .starting_score = 0,
+          .merge_score = 0.5,
+          .want_score = 0,
+      },
+      {
+          .label = "Both default values is ignored",
+          .starting_score = -1,
+          .merge_score = -1,
+          .want_score = -1,
+      },
+  };
+
+  for (const TestCase& test : tests) {
+    SCOPED_TRACE(test.label);
+    VisitContentModelAnnotations starting;
+    starting.visibility_score = test.starting_score;
+
+    VisitContentModelAnnotations merge_me;
+    merge_me.visibility_score = test.merge_score;
+
+    starting.MergeFrom(merge_me);
+
+    EXPECT_NEAR(starting.visibility_score, test.want_score, 0.001);
+  }
+}
+
 }  // namespace
 
 }  // namespace history