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;
-
  private:
   DISALLOW_COPY_AND_ASSIGN(ContentSettingMIDISysExImageModel);
 };
 
 namespace {
 
 struct ContentSettingsImageDetails {
   ContentSettingsType type;
   gfx::VectorIconId icon_id;
   int blocked_tooltip_id;
@@ skipping 408 matching lines @@
   return gfx::Image(
       gfx::CreateVectorIconWithBadge(icon_id_, 16, icon_color, icon_badge_id_));
 }
 
 ContentSettingImageModel::ContentSettingImageModel()
     : is_visible_(false),
       icon_id_(gfx::VectorIconId::VECTOR_ICON_NONE),
       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;
+}
 
-  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) {
   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