Add test for ContentSettingBubbleContents

chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/views/content_setting_bubble_contents.h"
+
+#include <memory>
+
+#include "base/macros.h"
+#include "base/message_loop/message_loop.h"

[msw, 2017/04/11 01:19:52]

nit: probably not needed (transitive include should suffice)

[Charlie Harrison, 2017/04/11 12:20:46]

Done.

+#include "base/strings/string_piece.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_task_scheduler.h"
+#include "base/time/time.h"

[msw, 2017/04/11 01:19:52]

nit: remove this if you can use event generator

[Charlie Harrison, 2017/04/11 12:20:46]

Done.

+#include "chrome/browser/ui/content_settings/content_setting_bubble_model.h"
+#include "chrome/browser/ui/content_settings/content_setting_bubble_model_delegate.h"
+#include "chrome/browser/ui/content_settings/content_setting_image_model.h"

[msw, 2017/04/11 01:19:52]

nit: needed?

[Charlie Harrison, 2017/04/11 12:20:45]

Nope, removed.

+#include "chrome/browser/ui/test/test_confirm_bubble_model.h"

[msw, 2017/04/11 01:19:52]

nit: needed?

[Charlie Harrison, 2017/04/11 12:20:46]

Done.

+#include "chrome/test/base/testing_profile.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/test_browser_thread.h"
+#include "content/public/test/test_web_contents_factory.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/views/bubble/bubble_dialog_delegate.h"
+#include "ui/views/test/views_test_base.h"
+#include "ui/views/view.h"
+#include "ui/views/widget/widget.h"
+#include "ui/views/window/dialog_client_view.h"
+
+using views::Widget;

[msw, 2017/04/11 01:19:52]

nit: remove this, add one more explicit views:: namespace qualifier use.

[Charlie Harrison, 2017/04/11 12:20:46]

Done.

+
+class TestContentSettingBubbleModel : public ContentSettingBubbleModel {

[msw, 2017/04/11 01:19:52]

nit: add a comment, describe the behavior of applying |done_button_text| when
the checkbox is toggled.

[Charlie Harrison, 2017/04/11 12:20:46]

Done.

+ public:
+  TestContentSettingBubbleModel(ContentSettingBubbleModel::Delegate* delegate,
+                                content::WebContents* web_contents,
+                                Profile* profile,
+                                base::string16 done_button_text)
+      : ContentSettingBubbleModel(delegate, web_contents, profile),

[msw, 2017/04/11 01:19:52]

Can you pass null a web_contents and profile?

[Charlie Harrison, 2017/04/11 12:20:46]

Done.

+        done_button_text_(done_button_text) {
+    SetTitle();
+    SetManageText();
+  }
+
+  ~TestContentSettingBubbleModel() override {}
+
+  void SetTitle() override { set_title(base::UTF8ToUTF16("Title")); }
+  void SetManageText() override {
+    set_manage_text(base::UTF8ToUTF16("test"));
+    set_show_manage_text_as_checkbox(true);
+  }
+
+  void OnManageCheckboxChecked(bool is_checked) override {
+    set_done_button_text(done_button_text_);
+  }
+
+ private:
+  base::string16 done_button_text_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestContentSettingBubbleModel);
+};
+
+class ContentSettingBubbleContentsTest
+    : public views::ViewsTestBase,
+      public ContentSettingBubbleModelDelegate {

[msw, 2017/04/11 01:19:52]

nit: make a separate test delegate class instead

[Charlie Harrison, 2017/04/11 12:20:46]

Done. Now the test class is empty!

+ public:
+  ContentSettingBubbleContentsTest()
+      : ui_thread_(content::BrowserThread::UI, message_loop()),

[msw, 2017/04/11 01:19:52]

q: Is this and |scoped_task_scheduler_| actually needed?

[Charlie Harrison, 2017/04/11 12:20:45]

It's needed for the web contents factory, but it's removed now.

+        scoped_task_scheduler_(message_loop()) {}
+
+  void SetUp() override {
+    views::ViewsTestBase::SetUp();
+    web_contents_ = web_contents_factory_.CreateWebContents(&profile_);
+  }
+
+  content::WebContents* web_contents() { return web_contents_; }
+
+  Profile* profile() { return &profile_; }
+
+  // ContentSettingBubbleModelDelegate:
+  void ShowCollectedCookiesDialog(content::WebContents* web_contents) override {
+  }
+  void ShowContentSettingsPage(ContentSettingsType type) override {}
+  void ShowMediaSettingsPage() override {}
+  void ShowLearnMorePage(ContentSettingsType type) override {}
+
+ private:
+  content::TestBrowserThread ui_thread_;
+  base::test::ScopedTaskScheduler scoped_task_scheduler_;
+
+  TestingProfile profile_;

[msw, 2017/04/11 01:19:52]

optional nit: if it's simpler, move the profile and web-contents members to
TestContentSettingBubbleModel and reduce this test subclass to just a using
statement: using ContentSettingBubbleContentsTest = views::ViewsTestBase;

[Charlie Harrison, 2017/04/11 12:20:45]

Not needed, I was able to remove all the web contents stuff :P

+  content::TestWebContentsFactory web_contents_factory_;
+
+  content::WebContents* web_contents_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(ContentSettingBubbleContentsTest);
+};
+
+// TODO(csharrison): Consider moving some functionality to the harness if more

[msw, 2017/04/11 01:19:52]

nit: remove this todo, it's understood.

[Charlie Harrison, 2017/04/11 12:20:45]

Done.

+// tests need to be written.
+//
+// Tests that clicking the checkbox successfully changes the text within the
+// done button, and that the size is properly updated.
+TEST_F(ContentSettingBubbleContentsTest, CheckboxClicked_ResizesDoneButton) {
+  // Set up the root widget.

[msw, 2017/04/11 01:19:53]

nit: "Set up the root widget, needed to show the bubble."

[Charlie Harrison, 2017/04/11 12:20:46]

Done.

+  views::Widget* widget = views::Widget::CreateWindowWithContextAndBounds(
+      nullptr, GetContext(), gfx::Rect(350, 0, 100, 100));
+  views::View* view = new views::View();

[msw, 2017/04/11 01:19:52]

nit: pass widget->GetContentsView() to the ctor instead.

[Charlie Harrison, 2017/04/11 12:20:46]

Done.

+  widget->GetContentsView()->AddChildView(view);
+  view->SetBounds(0, 0, 100, 100);
+  widget->Show();
+
+  base::string16 done_button_text = base::UTF8ToUTF16("01234567890123456789");
+
+  TestContentSettingBubbleModel* bubble_model =
+      new TestContentSettingBubbleModel(this, web_contents(), profile(),
+                                        done_button_text);
+  ContentSettingBubbleContents* bubble_view = new ContentSettingBubbleContents(
+      bubble_model, web_contents(), view, views::BubbleBorder::TOP_RIGHT);
+  views::Widget* bubble_widget =
+      views::BubbleDialogDelegateView::CreateBubble(bubble_view);
+  bubble_widget->Show();

[msw, 2017/04/11 01:19:52]

optional nit: you can inline the widget like this:
views::BubbleDialogDelegateView::CreateBubble(bubble_view)->Show();

[Charlie Harrison, 2017/04/11 12:20:45]

Done.

+
+  views::LabelButton* button = bubble_view->GetDialogClientView()->ok_button();
+  EXPECT_NE(done_button_text, button->GetText());
+  gfx::Size before_size = button->GetPreferredSize();

[msw, 2017/04/11 01:19:52]

nit: use the actual width instead of the preferred.

[Charlie Harrison, 2017/04/11 12:20:45]

Done.

+
+  // Check the checkbox. This entails finding it in the view hierarchy via some

[msw, 2017/04/11 01:19:52]

You can optionally add a views::Checkbox*
ContentSettingBubbleContents::manage_checkbox_for_testing() { return
manage_checkbox_; } instead

[Charlie Harrison, 2017/04/11 12:20:45]

I think I prefer this way. Adding getters for any view we might want to pull out
of the contents seems not sustainable.

+  // iteration.
+  auto children = bubble_view->GetChildrenInZOrder();
+  for (auto* child : children) {
+    if (base::StringPiece(child->GetClassName()) == "Checkbox") {

[msw, 2017/04/11 01:19:52]

q: is stringpiece actually needed?

[Charlie Harrison, 2017/04/11 12:20:46]

IMO it is nicer style than using strncmp or something like that.

+      views::Checkbox* b = static_cast<views::Checkbox*>(child);
+      ui::MouseEvent e(ui::ET_MOUSE_RELEASED, gfx::Point(0, 0), gfx::Point(),

[msw, 2017/04/11 01:19:52]

nit: use an EventGenerator instead

[Charlie Harrison, 2017/04/11 12:20:46]

Done.

+                       base::TimeTicks::Now(), ui::EF_LEFT_MOUSE_BUTTON,
+                       ui::EF_LEFT_MOUSE_BUTTON);
+      EXPECT_FALSE(b->checked());
+      b->OnMouseReleased(e);
+      EXPECT_TRUE(b->checked());
+      break;
+    }
+  }
+
+  // Text should change, and the preferred width should change to match it. The

[msw, 2017/04/11 01:19:53]

optional nit: just say "The done button's text and size should change." and
remove the preferred size check.

[Charlie Harrison, 2017/04/11 12:20:45]

Done.

+  // actual width should change to match the preferred width.
+  EXPECT_EQ(done_button_text, button->GetText());
+  gfx::Size after_size = button->GetPreferredSize();
+  EXPECT_GT(after_size.width(), before_size.width());
+  EXPECT_EQ(after_size.width(), button->width());
+
+  widget->Close();
+}