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..9d38226fed8846b415f4cb9b9405b1b9ee0094c3 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;
-
  private:
   DISALLOW_COPY_AND_ASSIGN(ContentSettingMIDISysExImageModel);
 };
@@ -521,39 +520,72 @@ ContentSettingImageModel::ContentSettingImageModel()
       icon_badge_id_(gfx::VectorIconId::VECTOR_ICON_NONE),
       explanatory_string_id_(0) {}
 
+// The ordering of the models here influences the order in which icons are
+// shown in the omnibox.
+static constexpr ContentSettingsType content_types[] = {

[tapted, 2017/02/07 01:23:31]

nit: Might be your C roots showing again ;). It's less common to use `static` as
a storage class specifier in Chromium (trivia: early C++ standards deprecated
it, but then they undeprecated it - I think there are cases for templates where
you can't have an anonymous namespace, but still want local linkage).

this should go up in the anonymous namespace, below kImageDetails[] (and be
named kContentTypeIconOrder or similar)

Edit: but, see comment below

[Peter Kasting, 2017/02/07 01:34:06]

Don't generalize too hard -- see e.g.
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/pF7nAmUQ2uw
.  Maybe you meant to speak of not using "static" to affect linkage, which is
true -- we try to use anonymous namespace for that.  But "static" for e.g.
static storage duration in a function is common.

In this case, the struct is not at function-scope, so I'd agree with you and
make it non-static in an anonymous namespace.

+    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,
+    CONTENT_SETTINGS_TYPE_DEFAULT,

[tapted, 2017/02/07 01:23:31]

This at least needs a comment, but.. although I used "DEFAULT" in my test code,
I'm not sure we can do this here for release code. I didn't fully dive in, but I
don't think there's a corresponding ContentSettingsType for
ContentSettingSubresourceFilterImageModel (aka "Deceptive Content").

And there's a comment in content_settings_types.h "WARNING: This enum is going
to be removed soon." - so I don't think we can just add a DECPETIVE_CONTENT enum
value (it's also used for histograms/UMA, so there are other implications with
doing that).

I _think_ the new way to identify ContentSettingsTypes is
"content_settings::ResourceIdentifier" (typedef to std::string), but that may
confuse things here.


So, my suggestion for this would be something like the following:

// Deceptive content doesn't have a corresponding ContentSettingsType, so
// provide a new, unique value to identify it in the icon order.
constexpr int kDeceptiveContentModelId = CONTENT_SETTINGS_TYPE_DEFAULT - 1;
constexpr int kContentTypeIconOrder[] = {
  CONTENT_SETTINGS_TYPE_COOKIES
..
  CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC, // Note: also does camera.
  kDeceptiveContentModelId,
...
};


but we may want an OWNER (e.g. raymes@) to run an eye over this

Using an existing enum value, such as _DEFAULT is an option too, so long as it's
clearly commented (but I'm leaning towards kDeceptiveContentModelId)

[kylix_rd, 2017/02/07 18:25:21]

I started down this path, but it's going to require some ugly casting between
ContentSettingType and int. At least the compiler isn't treating the enum as an
int here. 
 

+    CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS,
+    CONTENT_SETTINGS_TYPE_MIDI_SYSEX,
+};
+
 // 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 content_type : content_types) {

[tapted, 2017/02/07 01:23:31]

(with the above, my preference would be auto -> int to be clear we are losing
some type safety)

+    switch (content_type) {
+      case CONTENT_SETTINGS_TYPE_GEOLOCATION:
+        result.push_back(
+            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 CONTENT_SETTINGS_TYPE_DEFAULT:
+        result.push_back(
+            base::MakeUnique<ContentSettingSubresourceFilterImageModel>());
+        break;
+      case CONTENT_SETTINGS_TYPE_MIDI_SYSEX:
+        result.push_back(base::MakeUnique<ContentSettingMIDISysExImageModel>());
+        break;
+      default:
+        result.push_back(

[tapted, 2017/02/07 01:23:31]

Comment here, e.g.

// All other content settings types use ContentSettingBlockedImageModel.

[kylix_rd, 2017/02/07 18:25:21]

Done.

+            base::MakeUnique<ContentSettingBlockedImageModel>(content_type));
+        break;
+    }
+  }
   return result;
 }
 
+// static
+size_t ContentSettingImageModel::GetContentSettingImageModelIndexForTesting(

[tapted, 2017/02/07 01:23:31]

I would actually just expose kContentTypeIconOrder in the .h -- (e.g. as a data
member, and go back to static const -heh). Then I think this can be implmented
in the test file (and doesn't need `ForTesting`).

[kylix_rd, 2017/02/07 18:25:21]

Hmmm... I think keeping it internal and private is better here. This static
method shouldn't get linked in except for the tests since it's not referenced in
the release code.

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

Although this is likely true, I've never seen it used as justification for
putting a method in the release build rather than test code. I think the "don't
call ForTesting methods" is enforced via a python presubmit, so I've never
really trusted it.

I don't see much wrong with exposing kContentTypeIconOrder over exposing
GetContentSettingImageModelIndexForTesting(). kContentTypeIconOrder is a
constant, so it's hard to abuse.

I won't push on this, but an OWNER might (and I am not an OWNER here ;)

+    ContentSettingsType content_type) {
+  size_t result = 0;
+  if (content_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA)
+    content_type = CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC;
+  for (size_t i = 0; i < ARRAYSIZE(content_types); ++i) {
+    if (content_type == content_types[i])
+      return result;
+    ++result;
+  }
+  NOTREACHED();
+  return ARRAYSIZE(content_types);

[tapted, 2017/02/07 01:23:31]

ARRAYSIZE -> arraysize (I'm not sure where ARRAYSIZE is defined (libusb? or
maybe a windows header)), same above

[kylix_rd, 2017/02/07 18:25:21]

Done.

+}
+
 #if defined(OS_MACOSX)
 bool ContentSettingImageModel::UpdateFromWebContentsAndCheckIfIconChanged(
     content::WebContents* web_contents) {