Add card details and legal message to Android save credit card infobar.

components/autofill/core/browser/legal_message_line_unittest.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.

[gone, 2016/01/12 19:04:23]

nit: 2016

[please use gerrit instead, 2016/01/12 22:58:56]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/autofill/core/browser/legal_message_line.h"
+
+#include <utility>
+
+#include "base/json/json_reader.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace autofill {
+
+class LegalMessageLineTest : public ::testing::Test {
+ public:
+  LegalMessageLineTest() {}
+  ~LegalMessageLineTest() override {}
+
+  const LegalMessageLines& actual_lines() const { return actual_lines_; }
+
+  void SetLegalMessage(const std::string& message_json) {
+    scoped_ptr<base::Value> value(base::JSONReader::Read(message_json));
+    ASSERT_TRUE(value);
+    base::DictionaryValue* dictionary = nullptr;
+    ASSERT_TRUE(value->GetAsDictionary(&dictionary));

[Peter Kasting, 2016/01/12 23:09:37]

Nit: This one could probably be EXPECT_TRUE

[please use gerrit instead, 2016/01/13 21:22:09]

Done.

+    ASSERT_TRUE(dictionary);
+    LegalMessageLine::Parse(*dictionary, &actual_lines_);
+  }
+
+  // Returns true if lines are the same.

[Peter Kasting, 2016/01/12 23:09:37]

Nit: Maybe "Returns whether |lines| consists solely of |line|." (This uses my
param renames suggested below)

[please use gerrit instead, 2016/01/13 21:22:09]

Done.

+  static bool CompareLegalMessages(const LegalMessageLine& single_line,

[Peter Kasting, 2016/01/12 23:09:37]

Nit: I think it might be possible to declare some file-scoped non-member
operator==() methods in the place of these comparison functions?  The goal here
would be to use EXPECT_EQ() below for better error messages.  I think that would
also require defining a method to convert a LegalMessageLine to a string (which
hopefully could also be defined as a non-member in this file? Not sure), but
maybe it's worth it?

[please use gerrit instead, 2016/01/13 21:22:09]

Unfortunately does not work:

 error: invalid operands to binary expression ('const
autofill::LegalMessageLine' and 'element_type' (aka 'const
autofill::LegalMessageLine'))

[Peter Kasting, 2016/01/13 22:10:25]

I can't diagnose this without seeing the code you tried to write.  I wouldn't
give up here; see below.

[please use gerrit instead, 2016/01/14 02:39:39]

Parameterized. Done.

+                                   const LegalMessageLines& b) {

[Peter Kasting, 2016/01/12 23:09:37]

Nit: |b| -> |lines| (and maybe |single_line| -> |line|)

[please use gerrit instead, 2016/01/13 21:22:09]

Done.

+    return b.size() == 1 && CompareLegalMessageLines(single_line, *b[0]);
+  }
+
+  // Returns true if messages are the same.
+  static bool CompareLegalMessages(const LegalMessageLines& a,
+                                   const LegalMessageLines& b) {
+    if (a.size() != b.size())
+      return false;
+    for (size_t i = 0; i < a.size(); ++i) {
+      if (!CompareLegalMessageLines(*a[i], *b[i]))
+        return false;
+    }
+    return true;
+  }
+
+ private:
+  // Returns true if lines are the same.
+  static bool CompareLegalMessageLines(const LegalMessageLine& a,
+                                       const LegalMessageLine& b) {

[Peter Kasting, 2016/01/12 23:09:37]

Do we even need this method?  It seems like just checking "a == b" in the caller
ought to work?

[please use gerrit instead, 2016/01/13 21:22:09]

Unfortunately does not work:

 error: invalid operands to binary expression ('const
autofill::LegalMessageLine' and 'element_type' (aka 'const
autofill::LegalMessageLine'))

This error message makes little sense to me: I should be able to compare them.

[Peter Kasting, 2016/01/13 22:10:25]

Both this and the above are because you don't have an appropriate operator==()
defined.

The Google style guide now encourages operator== for cases like this:

"Don't go out of your way to avoid defining operator overloads. For example,
prefer to define == ... rather than Equals()..."

I think it ought to be possible to define == as a non-member, per
http://en.cppreference.com/w/cpp/language/operators :

bool operator==(const LegalMessageLine& lhs, const LegalMessageLine& rhs);

[please use gerrit instead, 2016/01/14 02:39:39]

Done.

+    if (a.text() != b.text())
+      return false;
+    if (a.links().size() != b.links().size())
+      return false;
+    for (size_t i = 0; i < a.links().size(); ++i) {
+      if (a.links()[i].range != b.links()[i].range)
+        return false;
+      if (a.links()[i].url != b.links()[i].url)
+        return false;
+    }
+    return true;
+  }
+
+  LegalMessageLines actual_lines_;
+
+  DISALLOW_COPY_AND_ASSIGN(LegalMessageLineTest);
+};
+
+TEST_F(LegalMessageLineTest, NoParameters) {

[Peter Kasting, 2016/01/12 23:09:37]

Nit: It seems kinda like you could condense this file by just having an array of
(input string, expected line) pairs and iterating through them.  This might
require implementing my next suggestion.

[please use gerrit instead, 2016/01/13 21:22:09]

I've tried both array of test cases and parameterized test. Both of these
approaches made the test more complex due to LegalMessageLines being
std::vector<scoped_ptr<LegalMessageLine>> instead of plain old
std::vector<LegalMessageLine>. Inability to copy LegalMessageLine also does not
help. Let's leave the test structure as is.

[Peter Kasting, 2016/01/13 22:10:25]

Why did you go with vector<scoped_ptr<LegalMessageLine>>, anyway?  It seems like
vector<LegalMessageLine> is the more logical type.  If the only reason is to
avoid adding copy/assignment support, you should definitely go ahead and make
the class copyable and eliminate the scoped_ptrs.

That in turn might make this change feasible.

[please use gerrit instead, 2016/01/14 02:39:39]

Made LegalMessageLine copyable and my life is so much easier now.

+  SetLegalMessage(
+      "{"
+      "  \"line\" : [ {"
+      "     \"template\": \"This is the entire message.\""
+      "  } ]"
+      "}");
+
+  LegalMessageLine expected_line;
+  expected_line.text_ = base::ASCIIToUTF16("This is the entire message.");

[Peter Kasting, 2016/01/12 23:09:37]

Nit: Given the contents of this file, it seems like it might be worth adding a
2-arg constructor that allows you to pass in the text and links.  If you want
you could make it test-only.

[please use gerrit instead, 2016/01/13 21:22:09]

Done.

+  EXPECT_TRUE(CompareLegalMessages(expected_line, actual_lines()));
+}
+
+TEST_F(LegalMessageLineTest, SingleParameter) {
+  SetLegalMessage(
+      "{"
+      "  \"line\" : [ {"
+      "     \"template\": \"Panda {0}.\","
+      "     \"template_parameter\": [ {"
+      "        \"display_text\": \"bears are fuzzy\","
+      "        \"url\": \"http://www.example.com\""
+      "     } ]"
+      "  } ]"
+      "}");
+
+  LegalMessageLine expected_line;
+  expected_line.text_ = base::ASCIIToUTF16("Panda bears are fuzzy.");
+  expected_line.links_ = {
+      {{6, 21}, GURL("http://www.example.com")},
+  };
+  EXPECT_TRUE(CompareLegalMessages(expected_line, actual_lines()));
+}
+
+TEST_F(LegalMessageLineTest, MissingUrl) {
+  SetLegalMessage(
+      "{"
+      "  \"line\" : [ {"
+      "     \"template\": \"Panda {0}.\","
+      "     \"template_parameter\": [ {"
+      "        \"display_text\": \"bear\""
+      "     } ]"
+      "  } ]"
+      "}");
+  // Legal message is invalid so actual_lines() should return no lines.
+  EXPECT_TRUE(
+      CompareLegalMessages(LegalMessageLines(), actual_lines()));
+}
+
+TEST_F(LegalMessageLineTest, MissingDisplayText) {
+  SetLegalMessage(
+      "{"
+      "  \"line\" : [ {"
+      "    \"template\": \"Panda {0}.\","
+      "    \"template_parameter\": [ {"
+      "      \"url\": \"http://www.example.com\""
+      "     } ]"
+      "  } ]"
+      "}");
+  // Legal message is invalid so actual_lines() should return no lines.
+  EXPECT_TRUE(
+      CompareLegalMessages(LegalMessageLines(), actual_lines()));
+}
+
+TEST_F(LegalMessageLineTest, EscapeCharacters) {
+  SetLegalMessage(
+      "{"
+      "  \"line\" : [ {"
+      "    \"template\": \"Panda '{'{0}'}' '{1}' don't $1.\","
+      "    \"template_parameter\": [ {"
+      "      \"display_text\": \"bears\","
+      "      \"url\": \"http://www.example.com\""
+      "     } ]"
+      "  } ]"
+      "}");
+
+  LegalMessageLine expected_line;
+  expected_line.text_ = base::ASCIIToUTF16("Panda {bears} {1} don't $1.");
+  expected_line.links_ = {
+      {{7, 12}, GURL("http://www.example.com")},
+  };
+  EXPECT_TRUE(CompareLegalMessages(expected_line, actual_lines()));
+}
+
+TEST_F(LegalMessageLineTest, ConsecutiveDollarSigns) {
+  SetLegalMessage(
+      "{"
+      "  \"line\" : [ {"
+      "     \"template\": \"$$\""
+      "  } ]"
+      "}");
+
+  // Consecutive dollar signs do not expand correctly (see comment in
+  // ReplaceTemplatePlaceholders() in save_card_bubble_controller_impl.cc).
+  // If this is fixed and this test starts to fail, please update the
+  // "Caveats" section of the SaveCardBubbleControllerImpl::SetLegalMessage()
+  // header file comment.
+  LegalMessageLine expected_line;
+  expected_line.text_ = base::ASCIIToUTF16("$$$");
+
+  EXPECT_TRUE(CompareLegalMessages(expected_line, actual_lines()));
+}
+
+TEST_F(LegalMessageLineTest, DollarAndParenthesis) {
+  // "${" does not expand correctly (see comment in
+  // ReplaceTemplatePlaceholders() in save_card_bubble_controller_impl.cc).
+  // If this is fixed and this test starts to fail, please update the
+  // "Caveats" section of the SaveCardBubbleControllerImpl::SetLegalMessage()
+  // header file comment.
+  SetLegalMessage(
+      "{"
+      "  \"line\" : [ {"
+      "    \"template\": \"${0}\","
+      "    \"template_parameter\": [ {"
+      "      \"display_text\": \"bears\","
+      "      \"url\": \"http://www.example.com\""
+      "    } ]"
+      "  } ]"
+      "}");
+  // Legal message is invalid so actual_lines() should return no lines.
+  EXPECT_TRUE(
+      CompareLegalMessages(LegalMessageLines(), actual_lines()));
+}
+
+TEST_F(LegalMessageLineTest, MultipleParameters) {
+  SetLegalMessage(
+      "{"
+      "  \"line\" : [ {"
+      "    \"template\": \"Panda {0} like {2} eat {1}.\","
+      "    \"template_parameter\": [ {"
+      "      \"display_text\": \"bears\","
+      "      \"url\": \"http://www.example.com/0\""
+      "    }, {"
+      "      \"display_text\": \"bamboo\","
+      "      \"url\": \"http://www.example.com/1\""
+      "    }, {"
+      "      \"display_text\": \"to\","
+      "      \"url\": \"http://www.example.com/2\""
+      "    } ]"
+      "  } ]"
+      "}");
+
+  LegalMessageLine expected_line;
+  expected_line.text_ = base::ASCIIToUTF16("Panda bears like to eat bamboo.");
+  expected_line.links_ = {
+      {{6, 11}, GURL("http://www.example.com/0")},
+      {{24, 30}, GURL("http://www.example.com/1")},
+      {{17, 19}, GURL("http://www.example.com/2")},
+  };
+  EXPECT_TRUE(CompareLegalMessages(expected_line, actual_lines()));
+}
+
+TEST_F(LegalMessageLineTest, MultipleLineElements) {
+  SetLegalMessage(
+      "{"
+      "  \"line\" : [ {"
+      "    \"template\": \"Panda {0}\","
+      "    \"template_parameter\": [ {"
+      "      \"display_text\": \"bears\","
+      "      \"url\": \"http://www.example.com/line_0_param_0\""
+      "    } ]"
+      "  }, {"
+      "    \"template\": \"like {1} eat {0}.\","
+      "    \"template_parameter\": [ {"
+      "      \"display_text\": \"bamboo\","
+      "      \"url\": \"http://www.example.com/line_1_param_0\""
+      "    }, {"
+      "      \"display_text\": \"to\","
+      "      \"url\": \"http://www.example.com/line_1_param_1\""
+      "    } ]"
+      "  }, {"
+      "    \"template\": \"The {0}.\","
+      "    \"template_parameter\": [ {"
+      "      \"display_text\": \"end\","
+      "      \"url\": \"http://www.example.com/line_2_param_0\""
+      "    } ]"
+      "  } ]"
+      "}");
+
+  LegalMessageLines expected(3);
+
+  // Line 0.
+  expected[0].reset(new LegalMessageLine);
+  expected[0]->text_ = base::ASCIIToUTF16("Panda bears");
+  expected[0]->links_ = {
+      {{6, 11}, GURL("http://www.example.com/line_0_param_0")},
+  };
+
+  // Line 1.
+  expected[1].reset(new LegalMessageLine);
+  expected[1]->text_ = base::ASCIIToUTF16("like to eat bamboo.");
+  expected[1]->links_ = {
+      {{12, 18}, GURL("http://www.example.com/line_1_param_0")},
+      {{5, 7}, GURL("http://www.example.com/line_1_param_1")},
+  };
+
+  // Line 2.
+  expected[2].reset(new LegalMessageLine);
+  expected[2]->text_ = base::ASCIIToUTF16("The end.");
+  expected[2]->links_ = {
+      {{4, 7}, GURL("http://www.example.com/line_2_param_0")},
+  };
+
+  EXPECT_TRUE(CompareLegalMessages(expected, actual_lines()));
+}
+
+TEST_F(LegalMessageLineTest, EmbeddedNewlines) {
+  SetLegalMessage(
+      "{"
+      "  \"line\" : [ {"
+      "    \"template\": \"Panda {0}\nlike {2} eat {1}.\nThe {3}.\","
+      "    \"template_parameter\": [ {"
+      "      \"display_text\": \"bears\","
+      "      \"url\": \"http://www.example.com/0\""
+      "    }, {"
+      "      \"display_text\": \"bamboo\","
+      "      \"url\": \"http://www.example.com/1\""
+      "    }, {"
+      "      \"display_text\": \"to\","
+      "      \"url\": \"http://www.example.com/2\""
+      "    }, {"
+      "      \"display_text\": \"end\","
+      "      \"url\": \"http://www.example.com/3\""
+      "    } ]"
+      "  } ]"
+      "}");
+
+  LegalMessageLine expected_line;
+  expected_line.text_ =
+      base::ASCIIToUTF16("Panda bears\nlike to eat bamboo.\nThe end.");
+  expected_line.links_ = {
+      {{6, 11}, GURL("http://www.example.com/0")},
+      {{24, 30}, GURL("http://www.example.com/1")},
+      {{17, 19}, GURL("http://www.example.com/2")},
+      {{36, 39}, GURL("http://www.example.com/3")},
+  };
+  EXPECT_TRUE(CompareLegalMessages(expected_line, actual_lines()));
+}
+
+TEST_F(LegalMessageLineTest, MaximumPlaceholders) {
+  SetLegalMessage(
+      "{"
+      "  \"line\" : [ {"
+      "    \"template\": \"a{0} b{1} c{2} d{3} e{4} f{5} g{6}\","
+      "    \"template_parameter\": [ {"
+      "      \"display_text\": \"A\","
+      "      \"url\": \"http://www.example.com/0\""
+      "    }, {"
+      "      \"display_text\": \"B\","
+      "      \"url\": \"http://www.example.com/1\""
+      "    }, {"
+      "      \"display_text\": \"C\","
+      "      \"url\": \"http://www.example.com/2\""
+      "    }, {"
+      "      \"display_text\": \"D\","
+      "      \"url\": \"http://www.example.com/3\""
+      "    }, {"
+      "      \"display_text\": \"E\","
+      "      \"url\": \"http://www.example.com/4\""
+      "    }, {"
+      "      \"display_text\": \"F\","
+      "      \"url\": \"http://www.example.com/5\""
+      "    }, {"
+      "      \"display_text\": \"G\","
+      "      \"url\": \"http://www.example.com/6\""
+      "    } ]"
+      "  } ]"
+      "}");
+
+  LegalMessageLine expected_line;
+  expected_line.text_ = base::ASCIIToUTF16("aA bB cC dD eE fF gG");
+  expected_line.links_ = {
+      {{1, 2}, GURL("http://www.example.com/0")},
+      {{4, 5}, GURL("http://www.example.com/1")},
+      {{7, 8}, GURL("http://www.example.com/2")},
+      {{10, 11}, GURL("http://www.example.com/3")},
+      {{13, 14}, GURL("http://www.example.com/4")},
+      {{16, 17}, GURL("http://www.example.com/5")},
+      {{19, 20}, GURL("http://www.example.com/6")},
+  };
+  EXPECT_TRUE(CompareLegalMessages(expected_line, actual_lines()));
+}
+
+}  // namespace autofill