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

components/autofill/core/browser/legal_message_line.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 "base/i18n/message_formatter.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
+
+namespace autofill {
+namespace {
+
+// Replace "{0}", "{1}", ... in |template_icu| with corresponding strings
+// from |display_texts|. Sets |out_message| to the resulting string, with
+// start position of each replacement in |out_offsets|.
+// Return false on failure. If false is returned then contents of |out_message|
+// and |out_offsets| are undefined.
+bool ReplaceTemplatePlaceholders(
+    const base::string16& template_icu,
+    const std::vector<base::string16>& display_texts,
+    base::string16* out_message,
+    std::vector<size_t>* out_offsets) {
+  // Escape "$" -> "$$" for ReplaceStringPlaceholders().
+  //
+  // Edge cases:
+  // 1. Two or more consecutive $ characters will be incorrectly expanded
+  //    ("$$" -> "$$$$", which ReplaceStringPlaceholders() then turns into
+  //    "$$$").
+  //
+  // 2. "${" will cause false to be returned. "${0}" will expand to "$${0}".
+  //    FormatWithNumberedArgs() turns it into "$$$1", which
+  //    ReplaceStringPlaceholders() then turns into "$$1" without doing the
+  //    parameter replacement. This causes false to be returned because each
+  //    parameter is not used exactly once.
+  //
+  // Both of these cases are noted in the header file, and are unlikely to

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

What header file?

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

Fixed. See legal_message_line.h.

+  // occur in any actual legal message.
+  base::string16 template_icu_escaped;
+  base::ReplaceChars(template_icu, base::ASCIIToUTF16("$"),
+                     base::ASCIIToUTF16("$$"), &template_icu_escaped);
+
+  // Replace "{0}" -> "$1", "{1}" -> "$2", ... to prepare |template_dollars|
+  // for ReplaceStringPlaceholders().
+  base::string16 template_dollars =
+      base::i18n::MessageFormatter::FormatWithNumberedArgs(
+          template_icu_escaped, "$1", "$2", "$3", "$4", "$5", "$6", "$7");
+
+  // FormatWithNumberedArgs() returns an empty string on failure.
+  if (template_dollars.empty() && !template_icu.empty())
+    return false;
+
+  // Replace "$1", "$2", ... with the display text of each parameter.
+  *out_message = base::ReplaceStringPlaceholders(template_dollars,
+                                                 display_texts, out_offsets);
+
+  // Each parameter must be used exactly once. If a parameter is unused or
+  // used more than once then it can't be determined which |offsets| entry
+  // corresponds to which parameter.
+  return out_offsets->size() == display_texts.size();
+}
+
+}  // namespace
+
+// static
+bool LegalMessageLine::Parse(const base::DictionaryValue& legal_message,
+                             std::vector<scoped_ptr<LegalMessageLine>>* out) {
+  const base::ListValue* lines = nullptr;
+  if (legal_message.GetList("line", &lines)) {
+    out->resize(lines->GetSize());
+    for (size_t i = 0; i < lines->GetSize(); ++i) {
+      out->at(i).reset(new LegalMessageLine);

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

Nit: Prefer (*out)[i] to using at() (2 places) (but see below comment)

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

Done.

+      const base::DictionaryValue* single_line;
+      if (!lines->GetDictionary(i, &single_line) ||
+          !out->at(i)->ParseLine(*single_line)) {
+        out->clear();

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

Nit: Instead of using clear() here, do something like this:

  const base::ListValue* line_list;
  if (legal_message.GetList("line", &line_list)) {
    LegalMessageLines lines;
    lines.reserve(line_list->GetSize());
    // Remainder of conditional goes here
    out->swap(lines);
  }
  return true;

This lets you avoid the clear() call before returning false, and allows you to
use reserve()/emplace_back() for a slight efficiency win (which doesn't really
matter in this case).  It's also safer if you add more exit points to the
function in the future, because we can't forget to clear() before exiting.

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

Done.

+        return false;
+      }
+    }
+  }
+
+  return true;
+}
+
+LegalMessageLine::LegalMessageLine() {}
+
+LegalMessageLine::~LegalMessageLine() {}
+
+bool LegalMessageLine::ParseLine(const base::DictionaryValue& line) {
+  // |display_texts| elements are the strings that will be substituted for
+  // "{0}", "{1}", etc. in the template string.
+  std::vector<base::string16> display_texts;
+
+  // Process all the template parameters.
+  const base::ListValue* template_parameters = nullptr;
+  if (line.GetList("template_parameter", &template_parameters)) {
+    display_texts.resize(template_parameters->GetSize());
+    links_.resize(template_parameters->GetSize());

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

Nit: Maybe these should reserve() and the code below emplace_back()?  This would
only work when we haven't called ParseLine() on |this| before, but it seems like
that is, in fact, how this is used.

Perhaps to make this more bulletproof, we could replace ParseLine() with a
private one-arg constructor, so Parse() above wouldn't construct an empty line
and then parse it, it would just construct directly?  If we did this, though,
we'd still need to be able to signal parsing errors, so we'd need to add a
private |valid_| field the caller could read.  I don't know whether on net this
winds up better or worse than the current code.

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

Done.
Going to steer away from the pattern of a private member variable "valid_" to
keep this code relatively simple.

+
+    for (size_t parameter_index = 0;
+         parameter_index < template_parameters->GetSize(); ++parameter_index) {
+      const base::DictionaryValue* single_parameter;
+      std::string url;
+      if (!template_parameters->GetDictionary(parameter_index,
+                                              &single_parameter) ||
+          !single_parameter->GetString("display_text",
+                                       &display_texts[parameter_index]) ||
+          !single_parameter->GetString("url", &url)) {

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

Nit: No {}

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

Done.

+        return false;
+      }
+      links_[parameter_index].url = GURL(url);
+    }
+  }
+
+  // Read the template string. It's a small subset of the ICU message format
+  // syntax.
+  base::string16 template_icu;
+  if (!line.GetString("template", &template_icu))
+    return false;
+
+  // Replace the placeholders in |template_icu| with strings from
+  // |display_texts|, and store the start position of each replacement in
+  // |offsets|.
+  std::vector<size_t> offsets;
+  if (!ReplaceTemplatePlaceholders(template_icu, display_texts, &text_,
+                                   &offsets)) {

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

Nit: No {}

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

Done.

+    return false;
+  }
+
+  // Fill in range values for all links.
+  for (size_t offset_index = 0; offset_index < offsets.size(); ++offset_index) {
+    size_t range_start = offsets[offset_index];
+    links_[offset_index].range = gfx::Range(
+        range_start, range_start + display_texts[offset_index].size());
+  }
+
+  return true;
+}
+
+}  // namespace autofill