Autofill: Add legal message footer to save credit card bubble.

chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc

Index: chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc
diff --git a/chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc b/chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc
index 2e43699e3f435ffe870588f220a00da460322e94..ca2eddbb85f9b42814c0d067e6016919ec5445fc 100644
--- a/chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc
+++ b/chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc
@@ -4,6 +4,9 @@
 
 #include "chrome/browser/ui/autofill/save_card_bubble_controller_impl.h"
 
+#include "base/i18n/message_formatter.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/autofill/save_card_bubble_view.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -42,6 +45,119 @@ void SaveCardBubbleControllerImpl::SetCallback(
   save_card_callback_ = save_card_callback;
 }
 
+bool SaveCardBubbleControllerImpl::SetLegalMessage(

[bondd, 2015/11/07 02:29:38]

I'll add some unit tests for SetLegalMessage() unless you think it's not worth
the trouble.

[Evan Stade, 2015/11/07 03:03:19]

this function needs unit tests

+    const base::ListValue& lines) {
+  ClearLegalMessage();
+  base::string16 message;
+  size_t next_range_index = 0;

[Evan Stade, 2015/11/07 03:03:19]

|next_range_index| is not intuitive at all and requires a thorough reading of
the code to understand; this is a good example of why I don't like added
complexity (looping) without a good reason for it

if you need to keep the proto as is, you should change this for loop body to a
single function call:

ParseAndAppendToLegalMessage();

this will disallow or at least discourage cross-iteration variables like
next_range_index which are just confusing and I don't think strictly necessary 
(e.g. if you have a local legal_message_links that you build up before appending
everything to legal_message_links_ you won't need this)

[bondd, 2015/11/11 01:53:36]

Done.

+
+  // Process all lines of the message. See comment in header file for example
+  // of valid |lines| data.
+  for (size_t line_index = 0; line_index < lines.GetSize(); ++line_index) {
+    const base::DictionaryValue* single_line;
+    if (!lines.GetDictionary(line_index, &single_line))
+      return false;
+
+    // |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 for the current message line.
+    const base::ListValue* template_parameters = nullptr;
+    if (single_line->GetList("template_parameter", &template_parameters)) {
+      for (size_t parameter_index = 0;
+           parameter_index < template_parameters->GetSize();
+           ++parameter_index) {
+        // Get a single element of the "template_parameter" list.
+        const base::DictionaryValue* single_parameter;
+        if (!template_parameters->GetDictionary(parameter_index,
+                                                &single_parameter))
+          return false;
+
+        // Read and store the "display_text" string.
+        base::string16 text;

[Evan Stade, 2015/11/07 03:03:19]

display_texts.push_back(base::string16());
if (!single_parameter->GetString("display_text", &display_texts.back()))
  return false;
// Save a line \o/

[bondd, 2015/11/11 01:53:36]

Done.

+        if (!single_parameter->GetString("display_text", &text))
+          return false;
+        display_texts.push_back(text);
+
+        // Read and store the "url" string.
+        LegalMessageLink link;

[Evan Stade, 2015/11/07 03:03:19]

ditto

[bondd, 2015/11/11 01:53:36]

Done.

+        if (!single_parameter->GetString("url", &link.url))
+          return false;
+        legal_message_links_.push_back(link);
+      }
+    }
+
+    // Read the template string. It's a small subset of the ICU message format
+    // syntax.
+    base::string16 template_icu;
+    if (!single_line->GetString("template", &template_icu))

[Evan Stade, 2015/11/07 03:03:19]

this function is pretty long and hard to read, can you split into helper
functions? In particular it seems the template parsing/modification part could
be separated cleanly.

[bondd, 2015/11/11 01:53:36]

Done. I think the new level of splitting makes sense; all of the Value reading
for a message line is in one function (less jumping back and forth between
functions to see what's happening) and each function fits on ~1 screen. Let me
know if you'd like me to split it further.

+      return false;
+
+    // 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
+    // occur in any actual legal message.
+    base::ReplaceChars(template_icu, base::ASCIIToUTF16("$"),
+                       base::ASCIIToUTF16("$$"), &template_icu);
+
+    // Replace "{0}" -> "$1", "{1}" -> "$2", ... to prepare |template_dollars|
+    // for ReplaceStringPlaceholders().
+    base::string16 template_dollars =
+        base::i18n::MessageFormatter::FormatWithNumberedArgs(
+            template_icu, "$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.
+    std::vector<size_t> offsets;
+    base::string16 message_line =
+        ReplaceStringPlaceholders(template_dollars, display_texts, &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.
+    if (offsets.size() != display_texts.size())
+      return false;
+
+    // Fill in |range| values for all links in this line.
+    for (size_t offset_index = 0; offset_index < offsets.size();
+         ++offset_index) {
+      size_t range_start = message.size() + offsets[offset_index];
+      legal_message_links_[next_range_index++].range = gfx::Range(
+          range_start, range_start + display_texts[offset_index].size());
+    }
+
+    // |message_line| is now the final line that will be displayed. Append it to
+    // |message|, along with a newline if there will be another line after this
+    // one.
+    message.append(message_line);
+    if (line_index + 1 < lines.GetSize())
+      message.push_back('\n');
+  }
+
+  legal_message_ = message;

[Evan Stade, 2015/11/07 03:03:19]

so legal_message_ will be empty on failure but legal_message_links_ will be
half-filled? seems inconsistent

[bondd, 2015/11/11 01:53:36]

Done. Everything is now empty on failure.

+  return true;
+}
+
+void SaveCardBubbleControllerImpl::ClearLegalMessage() {
+  legal_message_.clear();
+  legal_message_links_.clear();
+}
+
 void SaveCardBubbleControllerImpl::ShowBubble() {
   DCHECK(!save_card_callback_.is_null());
 
@@ -84,9 +200,19 @@ void SaveCardBubbleControllerImpl::OnCancelButton() {
 }
 
 void SaveCardBubbleControllerImpl::OnLearnMoreClicked() {
-  web_contents()->OpenURL(content::OpenURLParams(
-      GURL(kHelpURL), content::Referrer(), NEW_FOREGROUND_TAB,
-      ui::PAGE_TRANSITION_LINK, false));
+  OpenUrl(kHelpURL);
+}
+
+void SaveCardBubbleControllerImpl::OnLegalMessageLinkClicked(
+    const gfx::Range& link_range) {
+  for (size_t i = 0; i < legal_message_links_.size(); ++i) {
+    if (legal_message_links_[i].range == link_range) {
+      OpenUrl(legal_message_links_[i].url);
+      return;
+    }
+  }
+  // link_range was not found.
+  NOTREACHED();
 }
 
 void SaveCardBubbleControllerImpl::OnBubbleClosed() {
@@ -94,12 +220,31 @@ void SaveCardBubbleControllerImpl::OnBubbleClosed() {
   UpdateIcon();
 }
 
+const base::string16& SaveCardBubbleControllerImpl::GetLegalMessage() const {
+  return legal_message_;
+}
+
+size_t SaveCardBubbleControllerImpl::GetLegalMessageNumLinks() const {
+  return legal_message_links_.size();
+}
+
+const gfx::Range& SaveCardBubbleControllerImpl::GetLegalMessageLinkRange(
+    size_t index) const {
+  return legal_message_links_[index].range;
+}
+
 void SaveCardBubbleControllerImpl::UpdateIcon() {
   Browser* browser = chrome::FindBrowserWithWebContents(web_contents());
   LocationBar* location_bar = browser->window()->GetLocationBar();
   location_bar->UpdateSaveCreditCardIcon();
 }
 
+void SaveCardBubbleControllerImpl::OpenUrl(const std::string& url) {
+  web_contents()->OpenURL(
+      content::OpenURLParams(GURL(url), content::Referrer(), NEW_FOREGROUND_TAB,
+                             ui::PAGE_TRANSITION_LINK, false));
+}
+
 void SaveCardBubbleControllerImpl::DidNavigateMainFrame(
     const content::LoadCommittedDetails& details,
     const content::FrameNavigateParams& params) {