DialogBrowserTest implementation to invoke Content settings bubble dialogs.

chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h

Index: chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h
diff --git a/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h b/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h
index 58d8413a83a2d4953b1cc9a8afe1f67a287ac097..a9f00a805ef1b864259eebf45ed081e333d62644 100644
--- a/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h
+++ b/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h
@@ -83,6 +83,8 @@ class LocationBarViewMac : public LocationBar,
   ExtensionAction* GetVisiblePageAction(size_t index) override;
   void TestPageActionPressed(size_t index) override;
   bool GetBookmarkStarVisibility() override;
+  int ContentSettingImageModelCount() override;

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

Just a general comment - not sure there's much to do about it until the Cocoa
code is deleted - but this approach (inheriting from a testing interface in
release code) is quite unusual. The preferred approach is to implement a
`TestApi` which is a friend in release, but otherwise implemented (and compiled)
only in test files. (grep the codebase for test_api.h files to see examples - or
you can see a simple one I just made in
https://codereview.chromium.org/2666523002/ ).

There might not be a neat way to jump to this approach until we can assume we
are using a particular implementation (i.e. views only). I guess
LocationBarTesting has set a precedent already, but it feels quite bad.

Just something to think about -- maybe you can scrutinise the places in this CL
where test-only code is being added to the release build to see if there are
ways to avoid it.

[tapted, 2017/02/03 00:38:01]

Note I kinda went back on this in later comments - I think we should be using a
TestApi for the toolkit-views LocationBarView* . (The implementations of these
methods on LocationBarViewMac seem to be unused in the test, so they shouldn't
be added)

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

I was just trying to do a minimum of damage while keeping the the existing model
intact. If this isn't the right approach, I would suggest a different, more
complete, pass is done. Since this is merely there to get the code to compile,
this CL should focus on its primary purpose. Revamping the testing APIs seem a
little out of scope here.

[tapted, 2017/02/03 23:07:02]

We tend not to worry about damage if there's an overall improvement to code
health. But maintaining an existing broken model increases the code debt and
makes it harder to fix in future.

The current model is a violation of
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
. i.e.

"Functions used only for testing should be restricted to test-only scenarios
either by #ifdefing them appropriately (e.g. #if defined(UNIT_TEST)) or by
naming them with a ForTesting suffix. The latter will be checked at presubmit
time to ensure they're only called by test files."


If, while improving code health, there's sufficient damage that it makes code
review hard, then we try to split the CL, and land the cleanup first.

+  ContentSettingImageModel* GetContentSettingImageModel(size_t index) override;
 
   // Set/Get the editable state of the field.
   void SetEditable(bool editable);