DialogBrowserTest implementation to invoke Content settings bubble dialogs.

chrome/browser/ui/content_settings/content_setting_image_model.cc

Index: chrome/browser/ui/content_settings/content_setting_image_model.cc
diff --git a/chrome/browser/ui/content_settings/content_setting_image_model.cc b/chrome/browser/ui/content_settings/content_setting_image_model.cc
index 1bef3be8a1f4d1faa43c6f0c1379345e4de734e6..a170f6ceb9ecc8f36b69724c405f88fe0297d294 100644
--- a/chrome/browser/ui/content_settings/content_setting_image_model.cc
+++ b/chrome/browser/ui/content_settings/content_setting_image_model.cc
@@ -92,7 +92,6 @@ class ContentSettingMIDISysExImageModel
   ContentSettingMIDISysExImageModel();
 
   void UpdateFromWebContents(WebContents* web_contents) override;
-

[Peter Kasting, 2017/02/08 01:00:03]

Nit: Don't remove this

[kylix_rd, 2017/02/10 22:47:48]

Done.

  private:
   DISALLOW_COPY_AND_ASSIGN(ContentSettingMIDISysExImageModel);
 };
@@ -127,6 +126,27 @@ const ContentSettingsImageDetails kImageDetails[] = {
      IDS_BLOCKED_DOWNLOADS_EXPLANATION, IDS_ALLOWED_DOWNLOAD_TITLE},
 };
 
+// The ordering of the models here influences the order in which icons are
+// shown in the omnibox.
+constexpr ContentSettingsType kDeceptiveContentModelId =
+    static_cast<ContentSettingsType>(
+        static_cast<int>(CONTENT_SETTINGS_TYPE_DEFAULT) - 1);

[Peter Kasting, 2017/02/08 01:00:03]

This seems dangerous, because it's easy to reorder the original enum so DEFAULT
is not -1 or something, but rather has a valid value just before it.

It would probably be better to have the enum explicitly declare this signal
value as -2.  It's weird to have this in the enum, but DEFAULT is already weird,
and if clients are going to be passing values as ContentSettingsTypes that have
meaning, they should really be explicitly listed in the original enum and
updated there.

+constexpr ContentSettingsType kContentTypeIconOrder[] = {
+    CONTENT_SETTINGS_TYPE_COOKIES,
+    CONTENT_SETTINGS_TYPE_IMAGES,
+    CONTENT_SETTINGS_TYPE_JAVASCRIPT,
+    CONTENT_SETTINGS_TYPE_PPAPI_BROKER,
+    CONTENT_SETTINGS_TYPE_PLUGINS,
+    CONTENT_SETTINGS_TYPE_POPUPS,
+    CONTENT_SETTINGS_TYPE_GEOLOCATION,
+    CONTENT_SETTINGS_TYPE_MIXEDSCRIPT,
+    CONTENT_SETTINGS_TYPE_PROTOCOL_HANDLERS,
+    CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC,  // Note: also does camera.
+    kDeceptiveContentModelId,
+    CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS,
+    CONTENT_SETTINGS_TYPE_MIDI_SYSEX,
+};
+
 const ContentSettingsImageDetails* GetImageDetails(ContentSettingsType type) {
   for (const ContentSettingsImageDetails& image_details : kImageDetails) {
     if (image_details.type == type)
@@ -526,34 +546,50 @@ std::vector<std::unique_ptr<ContentSettingImageModel>>
 ContentSettingImageModel::GenerateContentSettingImageModels() {
   std::vector<std::unique_ptr<ContentSettingImageModel>> result;
 
-  // The ordering of the models here influences the order in which icons are
-  // shown in the omnibox.
-  result.push_back(base::MakeUnique<ContentSettingBlockedImageModel>(
-      CONTENT_SETTINGS_TYPE_COOKIES));
-  result.push_back(base::MakeUnique<ContentSettingBlockedImageModel>(
-      CONTENT_SETTINGS_TYPE_IMAGES));
-  result.push_back(base::MakeUnique<ContentSettingBlockedImageModel>(
-      CONTENT_SETTINGS_TYPE_JAVASCRIPT));
-  result.push_back(base::MakeUnique<ContentSettingBlockedImageModel>(
-      CONTENT_SETTINGS_TYPE_PPAPI_BROKER));
-  result.push_back(base::MakeUnique<ContentSettingBlockedImageModel>(
-      CONTENT_SETTINGS_TYPE_PLUGINS));
-  result.push_back(base::MakeUnique<ContentSettingBlockedImageModel>(
-      CONTENT_SETTINGS_TYPE_POPUPS));
-  result.push_back(base::MakeUnique<ContentSettingGeolocationImageModel>());
-  result.push_back(base::MakeUnique<ContentSettingBlockedImageModel>(
-      CONTENT_SETTINGS_TYPE_MIXEDSCRIPT));
-  result.push_back(base::MakeUnique<ContentSettingRPHImageModel>());
-  result.push_back(base::MakeUnique<ContentSettingMediaImageModel>());
-  result.push_back(
-      base::MakeUnique<ContentSettingSubresourceFilterImageModel>());
-  result.push_back(base::MakeUnique<ContentSettingBlockedImageModel>(
-      CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS));
-  result.push_back(base::MakeUnique<ContentSettingMIDISysExImageModel>());
-
+  for (auto icon : kContentTypeIconOrder) {
+    switch (icon) {
+      case CONTENT_SETTINGS_TYPE_GEOLOCATION:
+        result.push_back(

[Peter Kasting, 2017/02/08 01:00:03]

Nit: Might make sense to do:

    std::unique_ptr<ContentSettingImageModel> model;
    switch (icon) {
      case XXX:
        model = base::MakeUnique<...>;
      ...
    }
    result.push_back(std::move(model));

Due to better wrapping this might end up slightly shorter, and either way it
duplicates the least logic.

[kylix_rd, 2017/02/10 22:47:48]

Done.

+            base::MakeUnique<ContentSettingGeolocationImageModel>());
+        break;
+      case CONTENT_SETTINGS_TYPE_PROTOCOL_HANDLERS:
+        result.push_back(base::MakeUnique<ContentSettingRPHImageModel>());
+        break;
+      case CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC:
+        result.push_back(base::MakeUnique<ContentSettingMediaImageModel>());
+        break;
+      case kDeceptiveContentModelId:
+        result.push_back(
+            base::MakeUnique<ContentSettingSubresourceFilterImageModel>());
+        break;
+      case CONTENT_SETTINGS_TYPE_MIDI_SYSEX:
+        result.push_back(base::MakeUnique<ContentSettingMIDISysExImageModel>());
+        break;
+      default:
+        // All other content settins types use ContentSettingBlockedImageModel

[tapted, 2017/02/08 00:31:49]

settins -> settings (and full stop at end)

+        result.push_back(
+            base::MakeUnique<ContentSettingBlockedImageModel>(icon));

[tapted, 2017/02/08 00:31:49]

my preference would be a static cast here. Casting "out of range" values to an
enum can result in undefined behavior (see item (8) of
http://en.cppreference.com/w/cpp/language/static_cast)

I would also suggest adding

DCHECK(icon >= CONTENT_SETTINGS_TYPE_COOKIES && icon <=
CONTENT_SETTINGS_TYPE_MIDI_SYSEX);

[Peter Kasting, 2017/02/08 01:00:03]

Casting what?  |icon| is already of the type the ContentSettingBlockedImageModel
expects.
Note: If you do this, do it as two DCHECK_GE/LEs

[tapted, 2017/02/08 01:20:19]

An option would be to declare kContentTypeIconOrder[] as int[], that avoids the
static_cast from a calculated int above, but would require a static cast here.
(the ContentSettingsType enum is used for UMA and is on the chopping block
according to comments in the header, so adding a new enum value to handle
"deceptive content" may have other downsides.)

[Peter Kasting, 2017/02/08 01:26:19]

I think we're OK if the new value is -2 -- that won't need any histogram
changes.

I think adding the value to the enum is probably a better route than using an
int.

[kylix_rd, 2017/02/10 22:47:48]

Done. Added value to the enum.

+        break;
+    }
+  }
   return result;
 }
 
+// static
+size_t ContentSettingImageModel::GetContentSettingImageModelIndexForTesting(
+    ContentSettingsType content_type) {
+  size_t result = 0;

[Peter Kasting, 2017/02/08 01:00:03]

Why does this temp exist?  Why not just return |i| directly in the loop below?

[kylix_rd, 2017/02/10 22:47:48]

Done.

+  if (content_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA)
+    content_type = CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC;
+  for (size_t i = 0; i < arraysize(kContentTypeIconOrder); ++i) {
+    if (content_type == kContentTypeIconOrder[i])
+      return result;
+    ++result;
+  }
+  NOTREACHED();
+  return arraysize(kContentTypeIconOrder);
+}
+
 #if defined(OS_MACOSX)
 bool ContentSettingImageModel::UpdateFromWebContentsAndCheckIfIconChanged(
     content::WebContents* web_contents) {