DialogBrowserTest implementation to invoke Content settings bubble dialogs.

chrome/browser/ui/content_settings/content_setting_image_model.h

Index: chrome/browser/ui/content_settings/content_setting_image_model.h
diff --git a/chrome/browser/ui/content_settings/content_setting_image_model.h b/chrome/browser/ui/content_settings/content_setting_image_model.h
index ded59c1681d84d78ee254c8a83433136f2e908d3..8db43357afda75df66aa7f02594cc892dd9fdb4c 100644
--- a/chrome/browser/ui/content_settings/content_setting_image_model.h
+++ b/chrome/browser/ui/content_settings/content_setting_image_model.h
@@ -64,6 +64,10 @@ class ContentSettingImageModel {
   int explanatory_string_id() const { return explanatory_string_id_; }
   const base::string16& get_tooltip() const { return tooltip_; }
 
+  // For testing only. If this image model has a content type, it is returned
+  // here. Otherwise, CONTENT_SETTINGS_TYPE_DEFAULT is returned.
+  virtual ContentSettingsType GetContentType();

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

This is adding a lot of plumbing. (and we generally shouldn't rely on a comment
saying `ForTestingOnly`, instead we do GetContentTypeForTesting() -- there are
presubmits that check for that being used outside of test code -- but it doesn't
work for virtual methods.  there is also a performance cost converting an
accessor into a virtual method).

I see that it's tricky with all the subclasses though. But this is
cross-platform code, so there's no reason to avoid a TestApi. And I think at
least we should try to remove the virtual method. I suggest

namespace test {
class ContentSettingImageModelTestApi;
}

class ContentSettingImageModel {
..
protected:
  set_last_content_type_for_test(ContentSettingsType content_type) {
    last_content_type_for_test_ = content_type;
  }
..

private:
  friend class test::ContentSettingImageModelTestApi;
..
  ContentSettingsType last_content_type_for_test_ = /*something invalid */
};


in content_setting_bubble_dialog_browsertest.cc (for now - we can move it later
if other tests want it)

namespace test {
class ContentSettingImageModelTestApi {
 public:
  explicit ContentSettingImageModelTestApi(ContentSettingImageModel* model) :
model_(model) {}

  Foo::ContentType last_content_type() { return
model_->last_content_type_for_test_; }

 private:
  ContentSettingImageModel* model_;
};
}  // namespace test


(then remove the ContentSettingMediaImageModel bits and have other subclasses
set last_content_type_for_test_ in their constructor)

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

This seems a little overly complicated for merely getting one value. I would
argue that where this code is exercised (in the UI), the performance hit for the
added virtual method is negligible. I understand the sentiment here, but I don't
think the overhead is enough to warrant all this complexity. In plenty of other
cases, yes, but these classes are fairly simple.

+
  protected:
   ContentSettingImageModel();
 
@@ -102,13 +106,12 @@ class ContentSettingSimpleImageModel : public ContentSettingImageModel {
   bool ShouldRunAnimation(content::WebContents* web_contents) override;
   void SetAnimationHasRun(content::WebContents* web_contents) override;
 
+  ContentSettingsType GetContentType() override;
+
   // Factory method. Used only for testing.
   static std::unique_ptr<ContentSettingImageModel>
   CreateForContentTypeForTesting(ContentSettingsType content_type);
 
- protected:
-  ContentSettingsType content_type() { return content_type_; }
-
  private:
   ContentSettingsType content_type_;