DialogBrowserTest implementation to invoke Content settings bubble dialogs.

chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc

Index: chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc
diff --git a/chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc b/chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..913ad14d1c4bfabe5b7bfdaa3384fbd143f1bf06
--- /dev/null
+++ b/chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc
@@ -0,0 +1,180 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.

[tapted, 2017/02/03 00:35:40]

It's unclear what file this is testing (which is understandable - there's a few
involved). But usually it's good to make that link. This is mainly concerned
with the bubble, which is content_setting_bubble_contents, so I'd call this file
content_setting_bubble_contents_browsertest.cc 

[kylix_rd, 2017/02/03 18:55:04]

You make a good point, however I think it is testing both the content and the
invocation for that content. While they do all share the same content framework,
each one will exercise different aspects of it. This also ensures that the
pathway to actually getting that bubble dialog onto the screen is functional in
all cases. I may be mildly redundant in the grand scheme of things, but it is a
pathway that I didn't see getting coverage.

[tapted, 2017/02/03 23:07:02]

Hm - I think you are right that this particular location icon has some missing
coverage for invocation. But there is location_icon_view_interactive_uitest.cc
and page_action_image_view_interactive_uitest.cc for testing the invocation of
other location bar decorations (note they need to be "interactive" tests,
otherwise another test can steal focus away and cause the dialog to dismiss
itself).

If we don't spin a RunLoop, we may be able to get away with a browser_test
instead of an interactive_ui_test, but I don't think we can properly test
invocation/dismissal in this file -- DialogBrowserTest isn't designed for that
:/

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/memory/ptr_util.h"
+#include "base/time/time.h"
+#include "chrome/browser/content_settings/tab_specific_content_settings.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/content_settings/content_setting_image_model.h"
+#include "chrome/browser/ui/location_bar/location_bar.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/test/test_browser_dialog.h"
+#include "chrome/browser/ui/views/content_setting_bubble_contents.h"

[tapted, 2017/02/03 00:35:40]

Then move this include up to the top and put a blank line after

+#include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/ui/views/location_bar/content_setting_image_view.h"
+#include "chrome/browser/ui/views/location_bar/icon_label_bubble_view.h"
+#include "chrome/browser/ui/views/location_bar/location_bar_view.h"

[tapted, 2017/02/03 00:35:40]

hum - this is not cross-platform. So I think we can use a test api for
LocationBarView. With this #include, it's possible to safely downcast from the
LocationBarTesting* to a LocationBarView* and create a test api from it. We
don't need to modify LocationBarTesting

+#include "chrome/test/base/in_process_browser_test.h"
+#include "components/content_settings/core/common/content_settings_types.h"
+#include "ui/events/event.h"
+#include "ui/events/event_constants.h"
+#include "ui/views/view.h"
+#include "ui/views/widget/widget.h"
+#include "url/gurl.h"
+
+class ContentSettingBubbleDialogTest : public DialogBrowserTest {
+ public:
+  ContentSettingBubbleDialogTest() {}
+
+  ContentSettingImageModel* GetImageModel(ContentSettingsType content_type,
+                                          LocationBarTesting* location_bar) {
+    for (int i = 0; i < location_bar->ContentSettingImageModelCount(); i++) {
+      ContentSettingImageModel* image_model =
+          location_bar->GetContentSettingImageModel(i);
+      if (image_model->GetContentType() == content_type)
+        return image_model;
+    }
+    return nullptr;
+  }
+
+  void ShowDialogBubble(ContentSettingsType content_type) {

[tapted, 2017/02/03 00:35:40]

This is too big for an inline method - even in a test. Perhaps a helper method
in an anonymous namespace like


// Comment.
ShowContentSettingBubble(Browser* browser, ContentSettingsType type)


(note that a review from pkasting will typically ask for *all* methods in tests
to be declared out-of-line unless they are accessors that meet the style guide
notes for release code)

+    content::WebContents* web_contents =
+        browser()->tab_strip_model()->GetActiveWebContents();
+    TabSpecificContentSettings* content_settings =
+        TabSpecificContentSettings::FromWebContents(web_contents);
+    switch (content_type) {
+      case CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC:
+      case CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA:
+        content_settings->OnMediaStreamPermissionSet(
+            GURL::EmptyGURL(),
+            content_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC
+                ? TabSpecificContentSettings::MICROPHONE_ACCESSED
+                : TabSpecificContentSettings::CAMERA_ACCESSED,
+            std::string(), std::string(), std::string(), std::string());
+        break;
+        case CONTENT_SETTINGS_TYPE_GEOLOCATION:

[tapted, 2017/02/03 00:35:39]

nit: indent is off

[kylix_rd, 2017/02/03 18:55:04]

It's odd that 'git cl format' didn't catch this one.

+          content_settings->OnGeolocationPermissionSet(
+              GURL::EmptyGURL(), false);
+          break;
+      default:
+        content_settings->OnContentBlocked(content_type);

[tapted, 2017/02/03 00:35:40]

comment about this?

+    }
+    BrowserView* browser_view =
+        BrowserView::GetBrowserViewForBrowser(browser());
+    LocationBarView* location_bar_view = browser_view->GetLocationBarView();

[tapted, 2017/02/03 00:35:40]

huh, actually we have a LocationBarView* here too -- definitely no need for
`LocationBarTesting` - LocationBarViewTestApi is the way to access
LocationBarView internals

[kylix_rd, 2017/02/03 18:55:03]

Yes, I should have commented this code.

This block is needed in order to work around some silly C++ visibility issues.
While LocationBarView is descended from LocationBar, it moves overridden
virtuals to private (there should be a compiler warning that indicates a
descendant is lowering member visibility). Doing the up-cast will reveal those
virtuals. This is a classic case where lowering visibility in a descendant is
kind of silly because it doesn't *really* hide anything. This doesn't apply for
pure-virtual classes used as 'interfaces', where you want to require the caller
to only use the 'interface' to interact with the instance.

In the end, another approach would be for the LocationBarView to befriend the
ContentSettingBubbleDialogTest class which would then expose
GetLocationBarForTesting(), obviating the need for this block. Both techniques
work. Which to use is just a matter of preference and style.

[tapted, 2017/02/03 23:07:02]

hehe - I agree with you on this point - I don't think overrides should ever have
more restricted visibility, since there are always ways around it. But not
everyone agrees on this, and there's no style guideline about it. We used to
have implicit_cast<> in base/something.h that made it clear "I'm upcasting to
get around visibility weirdness" but it got removed.
Generally, it's nicer to `friend` a TestApi class only. There are many
`TODO(..): This class has too many friends` littered around the codebase, since
they proliferate easily without a TestApi.
If we are using LocationBarTesting, I think we should use

browser()->window()->GetLocationBar()->GetLocationBarForTesting();

to jump straight to LocationBarTesting.

Ideally... we don't #include location_bar_view.h -- that class isn't used on
MAC. Then we ensure the LocationBarTesting API is sufficient for testing without
access to the LocationBarView* internals (except I don't think we should be
adding more to LocationBarTesting, since it's modifying release code).

If we are testing LocationBarView* only, then we should use
LocationBarViewTestApi (and not mix two different testing APIs).

But to get this test running on Mac, we need to avoid LocationBarView* entirely.

+    LocationBar* location_bar = location_bar_view;
+    LocationBarTesting* location_bar_testing =
+        location_bar->GetLocationBarForTesting();
+    location_bar_view->Update(web_contents);

[tapted, 2017/02/03 00:35:39]

this needs more comments/commentary. E.g. is this the line that reveals the
dialog?

+    ContentSettingImageModel* image_model =
+        GetImageModel(content_type, location_bar_testing);
+    EXPECT_NE(image_model, nullptr);
+    ContentSettingImageView* image_view =
+        location_bar_view->GetContentSettingImageViewFromImageModel(
+            image_model);
+    EXPECT_NE(image_view, nullptr);
+    image_view->OnActivate(ui::MouseEvent(

[tapted, 2017/02/03 00:35:40]

Why's this necessary?

We typically can't rely on activation to be reliable in browser_tests because
there are many tests running in parallel on the test machine -- another test may
pop a dialog and deactivate.

[kylix_rd, 2017/02/03 18:55:04]

This actually invokes the dialog... In, essentially, the same manner as if the
user had clicked or otherwise activated the glyph/text in the omnibox. I'll add
a comment to that effect.

+        ui::ET_MOUSE_RELEASED, gfx::Point(0, 0), gfx::Point(0, 0),
+        base::TimeTicks::Now(), ui::EF_LEFT_MOUSE_BUTTON, 0));
+    EXPECT_NE(image_view->bubble_view(), nullptr);
+    EXPECT_TRUE(image_view->bubble_view()->visible());
+    views::Widget* widget = image_view->bubble_view()->GetWidget();
+    EXPECT_TRUE(widget->IsVisible());
+  }
+
+  void ShowDialog(const std::string& name) override {
+    const struct _content_settings_values {

[tapted, 2017/02/03 00:35:39]

the name `_content_settings_values` can just be omitted I think

[kylix_rd, 2017/02/03 18:55:04]

My "C" roots are showing ;)

+      std::string name;
+      ContentSettingsType content_type;
+    } content_settings_values[] = {
+        {std::string("cookies"), CONTENT_SETTINGS_TYPE_COOKIES},

[tapted, 2017/02/03 00:35:40]

string has an implicit constructor, so the `std::string(` shouldn't be needed

[kylix_rd, 2017/02/03 18:55:04]

I thought so as well, C++11 complains without the cast.

[tapted, 2017/02/03 23:07:02]

och boo ;). I'd suggest making the |name| member a const char* then. It will get
implicitly converted when operator== is applied. I think then you can also
declare this as `constexpr struct`, which is the preferred way of declaring
consts (when it works).

+        {std::string("images"), CONTENT_SETTINGS_TYPE_IMAGES},
+        {std::string("javascript"), CONTENT_SETTINGS_TYPE_JAVASCRIPT},
+        {std::string("plugins"), CONTENT_SETTINGS_TYPE_PLUGINS},
+        {std::string("popups"), CONTENT_SETTINGS_TYPE_POPUPS},
+        {std::string("geolocation"), CONTENT_SETTINGS_TYPE_GEOLOCATION},
+        {std::string("ppapi_broker"), CONTENT_SETTINGS_TYPE_PPAPI_BROKER},
+        {std::string("mixed_script"), CONTENT_SETTINGS_TYPE_MIXEDSCRIPT},
+        {std::string("mediastream_mic"), CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC},
+        {std::string("mediastream_camera"),
+         CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA},
+        {std::string("protocol_handlers"),
+         CONTENT_SETTINGS_TYPE_PROTOCOL_HANDLERS},
+        {std::string("automatic_downloads"),
+         CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS},
+        {std::string("midi_sysex"), CONTENT_SETTINGS_TYPE_MIDI_SYSEX},
+    };
+    for (size_t i = 0; i < sizeof(content_settings_values) /

[tapted, 2017/02/03 00:35:39]

there is arraysize in base/macros.h for this, but I think you can just do


for (const auto& case : content_settings_values) {
  if (name == case.name) {
    ShowDialogBubble(case.content_type);
    return;
  }
}

[kylix_rd, 2017/02/03 18:55:04]

Right... Thanks for the reminder.

+                               sizeof(content_settings_values[0]);
+         ++i) {
+      if (name == content_settings_values[i].name) {
+        ShowDialogBubble(content_settings_values[i].content_type);
+        return;
+      }
+    }
+    EXPECT_TRUE(false);

[tapted, 2017/02/03 00:35:40]

NOTREACHED()

[kylix_rd, 2017/02/03 18:55:04]

Won't NOTREACHED() terminated the whole test process while this will only fail
the current test? Although, if it does land here, there are probably larger
issues afoot.

[Peter Kasting, 2017/02/03 22:20:41]

Right, don't use NOTREACHED() in tests for the same reason you want to avoid
DCHECK -- it kills the process and messes up cleanup.

I believe the idiom you want here is ADD_FAILURE().

+  }
+};
+
+IN_PROC_BROWSER_TEST_F(ContentSettingBubbleDialogTest, InvokeDialog_cookies) {
+  RunDialog();
+}
+
+IN_PROC_BROWSER_TEST_F(ContentSettingBubbleDialogTest, InvokeDialog_images) {
+  RunDialog();
+}
+
+IN_PROC_BROWSER_TEST_F(ContentSettingBubbleDialogTest,
+                       InvokeDialog_javascript) {
+  RunDialog();
+}
+
+IN_PROC_BROWSER_TEST_F(ContentSettingBubbleDialogTest, InvokeDialog_plugins) {
+  RunDialog();
+}
+
+IN_PROC_BROWSER_TEST_F(ContentSettingBubbleDialogTest, InvokeDialog_popups) {
+  RunDialog();
+}
+
+IN_PROC_BROWSER_TEST_F(ContentSettingBubbleDialogTest,
+                       InvokeDialog_geolocation) {
+  RunDialog();
+}
+
+IN_PROC_BROWSER_TEST_F(ContentSettingBubbleDialogTest,
+                       InvokeDialog_ppapi_broker) {
+  RunDialog();
+}
+
+IN_PROC_BROWSER_TEST_F(ContentSettingBubbleDialogTest,
+                       InvokeDialog_mixed_script) {
+  RunDialog();
+}
+
+IN_PROC_BROWSER_TEST_F(ContentSettingBubbleDialogTest,
+                       InvokeDialog_mediastream_mic) {
+  RunDialog();
+}
+
+IN_PROC_BROWSER_TEST_F(ContentSettingBubbleDialogTest,
+                       InvokeDialog_mediastream_camera) {
+  RunDialog();
+}
+
+IN_PROC_BROWSER_TEST_F(ContentSettingBubbleDialogTest,
+                       InvokeDialog_protocol_handlers) {
+  RunDialog();
+}
+
+IN_PROC_BROWSER_TEST_F(ContentSettingBubbleDialogTest,
+                       InvokeDialog_automatic_downloads) {
+  RunDialog();
+}
+
+IN_PROC_BROWSER_TEST_F(ContentSettingBubbleDialogTest,
+                       InvokeDialog_midi_sysex) {
+  RunDialog();
+}