DialogBrowserTest implementation to invoke Content settings bubble dialogs.

chrome/browser/ui/content_settings/content_setting_image_model.cc

 // Copyright (c) 2012 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/content_settings/content_setting_image_model.h"
 
 #include "base/feature_list.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "build/build_config.h"
@@ skipping 74 matching lines @@
 
   void UpdateFromWebContents(WebContents* web_contents) override;
 };
 
 class ContentSettingMIDISysExImageModel
     : public ContentSettingSimpleImageModel {
  public:
   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);
 };
 
 namespace {
 
 struct ContentSettingsImageDetails {
   ContentSettingsType type;
   gfx::VectorIconId icon_id;
   int blocked_tooltip_id;
@@ skipping 14 matching lines @@
      IDS_BLOCKED_POPUPS_TOOLTIP, IDS_BLOCKED_POPUPS_EXPLANATORY_TEXT, 0},
     {CONTENT_SETTINGS_TYPE_MIXEDSCRIPT, gfx::VectorIconId::MIXED_CONTENT,
      IDS_BLOCKED_DISPLAYING_INSECURE_CONTENT, 0, 0},
     {CONTENT_SETTINGS_TYPE_PPAPI_BROKER, gfx::VectorIconId::EXTENSION,
      IDS_BLOCKED_PPAPI_BROKER_TITLE, 0, IDS_ALLOWED_PPAPI_BROKER_TITLE},
     {CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS,
      gfx::VectorIconId::FILE_DOWNLOAD, IDS_BLOCKED_DOWNLOAD_TITLE,
      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)
       return &image_details;
   }
   return nullptr;
 }
 
 }  // namespace
 
@@ skipping 379 matching lines @@
     : is_visible_(false),
       icon_id_(gfx::VectorIconId::VECTOR_ICON_NONE),
       icon_badge_id_(gfx::VectorIconId::VECTOR_ICON_NONE),
       explanatory_string_id_(0) {}
 
 // static
 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;
+}
 
-  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) {
   gfx::VectorIconId old_icon = icon_id_;
   gfx::VectorIconId old_badge_icon = icon_badge_id_;
   UpdateFromWebContents(web_contents);
   return old_icon != icon_id_ && old_badge_icon != icon_badge_id_;
 }
 #endif