Add explanation text and legal message footer to upload bubble.

chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm

Index: chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm
diff --git a/chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm b/chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm
index 497c7a72b55ccd277e58332b0c78a1c1e99847f3..728e714241265ae091ffe3fcc7e8c92da87c9161 100644
--- a/chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm
+++ b/chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm
@@ -2,7 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/json/json_reader.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/values.h"
 #include "chrome/browser/ui/autofill/save_card_bubble_controller.h"
 #import "chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.h"
 #import "chrome/browser/ui/cocoa/browser_window_controller.h"
@@ -20,11 +22,42 @@ namespace {
 class TestSaveCardBubbleController : public SaveCardBubbleController {
  public:
   TestSaveCardBubbleController() {
-    lines_.reset(new LegalMessageLines());
+    std::string message_json =
+        "{"
+        "  \"line\" : ["
+        "    {"
+        "      \"template\": \"Please check out our {0}.\","
+        "      \"template_parameter\": ["
+        "        {"
+        "          \"display_text\": \"terms of service\","
+        "          \"url\": \"http://help.example.com/legal_message\""
+        "        }"
+        "      ]"
+        "    },"
+        "    {"
+        "      \"template\": \"We also have a {0} and {1}.\","
+        "      \"template_parameter\": ["
+        "        {"
+        "          \"display_text\": \"mission statement\","
+        "          \"url\": \"http://www.example.com/our_mission\""
+        "        },"
+        "        {"
+        "          \"display_text\": \"privacy policy\","
+        "          \"url\": \"http://help.example.com/privacy_policy\""
+        "        }"
+        "      ]"
+        "    }"
+        "  ]"
+        "}";
+    scoped_ptr<base::Value> value(base::JSONReader::Read(message_json));
+    base::DictionaryValue* legal_message;
+    value->GetAsDictionary(&legal_message);

[groby-ooo-7-16, 2016/03/03 00:40:23]

ASSERT_TRUE?

[Justin Donnelly, 2016/03/03 18:45:10]

Done. (Required moving this code to a void function.)

+    LegalMessageLine::Parse(*legal_message, &lines_);
 
     on_save_button_was_called_ = false;
     on_cancel_button_was_called_ = false;
     on_learn_more_was_called_ = false;
+    on_legal_message_was_called_ = false;
     on_bubble_closed_was_called_ = false;
   }
 
@@ -42,25 +75,32 @@ class TestSaveCardBubbleController : public SaveCardBubbleController {
   void OnSaveButton() override { on_save_button_was_called_ = true; }
   void OnCancelButton() override { on_cancel_button_was_called_ = true; }

[groby-ooo-7-16, 2016/03/03 00:40:23]

It's not the CL to change this, but this looks like it would've been better off
as a mock.

[Justin Donnelly, 2016/03/03 18:45:10]

I don't disagree, but some reviewers are enthusiastically anti-mock so I tend to
use the manual approach when the conditions I care about are relatively simple.
If this gets any more complicated, I'll definitely switch.

[groby-ooo-7-16, 2016/03/07 19:54:58]

Argl. If it's more complicated, the likelihood it should be a mock is smaller :)
We should restrict gmock to fairly trivial objects.

But that's probably a discussion for chromium-dev when we're bored ;)

[Justin Donnelly, 2016/03/07 21:21:02]

I'm not sure there's anything in all of software engineering on which there's
less consensus than mocks. :) I look forward to the discussion.

   void OnLearnMoreClicked() override { on_learn_more_was_called_ = true; }
-  void OnLegalMessageLinkClicked(const GURL& url) override {}
+  void OnLegalMessageLinkClicked(const GURL& url) override {
+    on_legal_message_was_called_ = true;
+    legal_message_url_ = url.spec();
+  }
   void OnBubbleClosed() override { on_bubble_closed_was_called_ = true; }
 
   const LegalMessageLines& GetLegalMessageLines() const override {
-    return *lines_;
+    return lines_;
   }
 
   // Testing state.
   bool on_save_button_was_called() { return on_save_button_was_called_; }
   bool on_cancel_button_was_called() { return on_cancel_button_was_called_; }
   bool on_learn_more_was_called() { return on_learn_more_was_called_; }
+  bool on_legal_message_was_called() { return on_legal_message_was_called_; }
+  std::string legal_message_url() { return legal_message_url_; }
   bool on_bubble_closed_was_called() { return on_bubble_closed_was_called_; }
 
  private:
-  scoped_ptr<LegalMessageLines> lines_;
+  LegalMessageLines lines_;
 
   bool on_save_button_was_called_;
   bool on_cancel_button_was_called_;
   bool on_learn_more_was_called_;
+  bool on_legal_message_was_called_;
+  std::string legal_message_url_;
   bool on_bubble_closed_was_called_;
 };
 
@@ -100,6 +140,7 @@ TEST_F(SaveCardBubbleViewTest, SaveShouldClose) {
   EXPECT_TRUE(bubble_controller_->on_save_button_was_called());
   EXPECT_FALSE(bubble_controller_->on_cancel_button_was_called());
   EXPECT_FALSE(bubble_controller_->on_learn_more_was_called());
+  EXPECT_FALSE(bubble_controller_->on_legal_message_was_called());
 
   EXPECT_TRUE(bubble_controller_->on_bubble_closed_was_called());
 }
@@ -110,6 +151,7 @@ TEST_F(SaveCardBubbleViewTest, CancelShouldClose) {
   EXPECT_FALSE(bubble_controller_->on_save_button_was_called());
   EXPECT_TRUE(bubble_controller_->on_cancel_button_was_called());
   EXPECT_FALSE(bubble_controller_->on_learn_more_was_called());
+  EXPECT_FALSE(bubble_controller_->on_legal_message_was_called());
 
   EXPECT_TRUE(bubble_controller_->on_bubble_closed_was_called());
 }
@@ -122,6 +164,29 @@ TEST_F(SaveCardBubbleViewTest, LearnMoreShouldNotClose) {
   EXPECT_FALSE(bubble_controller_->on_save_button_was_called());
   EXPECT_FALSE(bubble_controller_->on_cancel_button_was_called());
   EXPECT_TRUE(bubble_controller_->on_learn_more_was_called());
+  EXPECT_FALSE(bubble_controller_->on_legal_message_was_called());
+
+  EXPECT_FALSE(bubble_controller_->on_bubble_closed_was_called());
+}
+
+TEST_F(SaveCardBubbleViewTest, LegalMessageShouldNotClose) {
+  NSString* legalText = @"We also have a mission statement and privacy policy.";
+  base::scoped_nsobject<NSTextView> textView(
+      [[NSTextView alloc] initWithFrame:NSZeroRect]);
+  base::scoped_nsobject<NSAttributedString> attributedMessage(
+      [[NSAttributedString alloc] initWithString:legalText attributes:@{}]);
+  [[textView textStorage] setAttributedString:attributedMessage];
+
+  NSObject* link = nil;
+  [bridge_->view_controller_ textView:textView clickedOnLink:link atIndex:40];
+
+  EXPECT_FALSE(bubble_controller_->on_save_button_was_called());
+  EXPECT_FALSE(bubble_controller_->on_cancel_button_was_called());
+  EXPECT_FALSE(bubble_controller_->on_learn_more_was_called());

[groby-ooo-7-16, 2016/03/03 00:40:23]

Do these EXPECT_FALSE statements matter?

[Justin Donnelly, 2016/03/03 18:45:10]

I think so. Effective unit tests IMO check both that the expected outcome
occurred and that unexpected conditions don't arise.

[groby-ooo-7-16, 2016/03/07 19:54:58]

Meh... There's an infinite number of unexpected conditions.  Can't test for them
all.

[Justin Donnelly, 2016/03/07 21:21:02]

Granted, you can't check them all. But I don't think you should check for none,
either, and I don't think I'm alone in that. Besides, this way, what to check
becomes a subjective question, ideal for bikeshedding. ;)

+  EXPECT_TRUE(bubble_controller_->on_legal_message_was_called());
+
+  std::string url("http://help.example.com/privacy_policy");
+  EXPECT_EQ(url, bubble_controller_->legal_message_url());
 
   EXPECT_FALSE(bubble_controller_->on_bubble_closed_was_called());
 }