Remove CONTENT_SETTINGS_NUM_TYPES (part 2)

Remove CONTENT_SETTINGS_NUM_TYPES (part 2)

To make it possible to make ContentSettingsTypes no longer an enum we need
to remove the occurrences of iterations over it. This CL removes occurences
from chrome/browser/ui/content_settings and does some cleanup. Rather than
switching on ContentSettingsType, we instead create a list of content settings
types that have bubbles and iterate over that.

The list of nib paths in content_setting_bubble_cocoa.mm has been updated to
include CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC/CAMERA to match the list of
bubble types.

location_bar_view.cc has been changed not to iterate over entire enum anymore
because it only needed to iterate over the list of types that have bubbles.

BUG=526932
Committed: https://crrev.com/3432a1f970e76002552e599cd1148774a3310393
Cr-Commit-Position: refs/heads/master@{#347888}

[raymes, 2015-09-03 04:52:27]

raymes@chromium.org changed reviewers:
+ tapted@chromium.org

[raymes, 2015-09-03 05:40:29]

The CQ bit was checked by raymes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-03 05:40:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312053006/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312053006/1

[commit-bot: I haz the power, 2015-09-03 06:21:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-03 06:21:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tapted, 2015-09-03 06:36:38]

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm
(right):

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:255:
{CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC, @"ContentBlockedMedia"},
_MIC and _CAMERA were previously NOTREACHED() - is this change intentional?
(mention in CL description?)

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:283:
DCHECK_NE(@"", nibPath);
DCHECK_NE(0u, [nibPath length]);

(otherwise it's a pointer comparison, but might still fail if the optimizer
gives the two @"" constants the same storage)

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1286:
base::LazyInstance<std::set<ContentSettingsType>> g_supported_bubble_types =
either:

anonymous namespace + base::LazyInstance<std::set<ContentSettingsType>>::Leaky
(otherwise it registers an atexit handler to destroy it)

or (probably better)

in GetSupportedBubbleTypes, do

CR_DEFINE_STATIC_LOCAL(
    const std::set<ContentSettingsType>,
    supported_bubble_types,
    (kSupportedBubbleTypes,
         kSupportedBubbleTypes + arraysize(kSupportedBubbleTypes)));
return supported_bubble_types;

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1307:
DCHECK(ContainsKey(GetSupportedBubbleTypes(), content_type));
I've never really understood ContainsKey.... if you don't want stl_util.h, you
can just do

DCHECK(GetSupportedBubbleTypes().count(content_type));

but I guess not everyone sees set::count as being an alias for (non-existent)
set::contains

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:104: static
bool HasBubble(ContentSettingsType type);
HasBubbleForType?

(HasBubble sounds too much like IsBubbleShowing)

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:288:
ContentSettingBubbleModel::GetSupportedBubbleTypes()) {
This is now iterating over fewer things right? (was that a bug? worth
mentioning?)

[raymes, 2015-09-07 03:25:46]

The CQ bit was checked by raymes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-07 03:25:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312053006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312053006/20001

[commit-bot: I haz the power, 2015-09-07 04:04:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-07 04:04:36]

Dry run: This issue passed the CQ dry run.

[raymes, 2015-09-07 04:29:28]

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm
(right):

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:255:
{CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC, @"ContentBlockedMedia"},
On 2015/09/03 06:36:38, tapted wrote:
> _MIC and _CAMERA were previously NOTREACHED() - is this change intentional?
> (mention in CL description?)

This is intentional to match the list of bubble types.

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:283:
DCHECK_NE(@"", nibPath);
On 2015/09/03 06:36:38, tapted wrote:
> DCHECK_NE(0u, [nibPath length]);
> 
> (otherwise it's a pointer comparison, but might still fail if the optimizer
> gives the two @"" constants the same storage)

Done.

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1286:
base::LazyInstance<std::set<ContentSettingsType>> g_supported_bubble_types =
Ah you're right - I had this before but changed it but now I can do what you
suggested :)

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1307:
DCHECK(ContainsKey(GetSupportedBubbleTypes(), content_type));
I agree with you - I used to do that but I've had other people tell me to do the
opposite :( I'll leave it as is for now.

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:104: static
bool HasBubble(ContentSettingsType type);
I removed this and changed the callers to just check the set returned above :)

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1312053006/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:288:
ContentSettingBubbleModel::GetSupportedBubbleTypes()) {
On 2015/09/03 06:36:38, tapted wrote:
> This is now iterating over fewer things right? (was that a bug? worth
> mentioning?)

This stuff is all only used for bubbles. The scary part is that it was working
properly when iterating over all content settings types =\

[tapted, 2015-09-07 05:39:21]

https://codereview.chromium.org/1312053006/diff/20001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_unittest.mm
(right):

https://codereview.chromium.org/1312053006/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_unittest.mm:124:
CreateBubbleController(CONTENT_SETTINGS_TYPE_MEDIASTREAM);
Should this iterate over _MIC and _CAMERA as well so there's coverage?
Alternatively, in the Init test, what about something like

size_t expected_menu_size = 0;
if (MEDIASTREAM || _MIC || _CAMERA)
  expected_menu_size = 2;
EXPECT_EQ(expected_menu_isze, [controller mediaMenus]->size());

[raymes, 2015-09-07 05:58:46]

The CQ bit was checked by raymes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-07 05:58:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312053006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312053006/40001

[raymes, 2015-09-07 06:21:59]

On 2015/09/07 05:39:21, tapted wrote:
>
https://codereview.chromium.org/1312053006/diff/20001/chrome/browser/ui/cocoa...
> File
>
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_unittest.mm
> (right):
> 
>
https://codereview.chromium.org/1312053006/diff/20001/chrome/browser/ui/cocoa...
>
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_unittest.mm:124:
> CreateBubbleController(CONTENT_SETTINGS_TYPE_MEDIASTREAM);
> Should this iterate over _MIC and _CAMERA as well so there's coverage?
> Alternatively, in the Init test, what about something like
> 
> size_t expected_menu_size = 0;
> if (MEDIASTREAM || _MIC || _CAMERA)
>   expected_menu_size = 2;
> EXPECT_EQ(expected_menu_isze, [controller mediaMenus]->size());

I ended up removing the _MIC and _CAMERA entries from the list of bubble types
because they are unused too :)

[commit-bot: I haz the power, 2015-09-07 06:28:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-07 06:28:17]

Dry run: This issue passed the CQ dry run.

[tapted, 2015-09-07 06:41:53]

lgtm

[raymes, 2015-09-07 06:48:45]

raymes@chromium.org changed reviewers:
+ msw@chromium.org

[raymes, 2015-09-07 06:48:46]

+msw for non-cocoa chrome/browser/ui/* OWNERS

Thanks!

[msw, 2015-09-08 18:40:46]

lgtm with nits.

https://codereview.chromium.org/1312053006/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/1312053006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:107: for
(ContentSettingsType type :
nit: auto

https://codereview.chromium.org/1312053006/diff/40001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

https://codereview.chromium.org/1312053006/diff/40001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:100: //
Returns a vector of the Supported bubble types.
nit: s/vector/set/ and un-capitalize "supported"

https://codereview.chromium.org/1312053006/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1312053006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:287: for
(ContentSettingsType type :
nit: auto

[raymes, 2015-09-09 01:02:08]

raymes@chromium.org changed reviewers:
+ bauerb@google.com

[raymes, 2015-09-09 01:02:09]

+bauerb for the discussion around "auto" below

Thanks msw! There is still the question around "auto" usage but I won't block
landing this on that. If necessary I will make the change in a subsequent CL.

https://codereview.chromium.org/1312053006/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/1312053006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:107: for
(ContentSettingsType type :
Hmm I've had +bauerb@ ask me to do the exact opposite in cases like this. 

I read up on the style guide and skimmed the discussions around it:
http://chromium-cpp.appspot.com/ My feeling is that in this case it doesn't make
the code more readable to use auto (and potentially makes it less readable).
What do you think?

https://codereview.chromium.org/1312053006/diff/40001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

https://codereview.chromium.org/1312053006/diff/40001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:100: //
Returns a vector of the Supported bubble types.
Oops, thanks. Done.

[raymes, 2015-09-09 01:02:26]

raymes@chromium.org changed reviewers:
+ bauerb@chromium.org
- bauerb@google.com

[raymes, 2015-09-09 01:03:10]

The CQ bit was checked by raymes@chromium.org

[raymes, 2015-09-09 01:03:10]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1312053006/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-09-09 01:03:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312053006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312053006/60001

[msw, 2015-09-09 01:10:03]

still lgtm

https://codereview.chromium.org/1312053006/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/1312053006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:107: for
(ContentSettingsType type :
On 2015/09/09 01:02:09, raymes wrote:
> Hmm I've had +bauerb@ ask me to do the exact opposite in cases like this. 
> 
> I read up on the style guide and skimmed the discussions around it:
> http://chromium-cpp.appspot.com/ My feeling is that in this case it doesn't
make
> the code more readable to use auto (and potentially makes it less readable).
> What do you think?

I'd use auto in fairly obvious scenarios when it reduces lines of code, but it's
a nit, fine either way and subject to author/reviewer preferences.

[commit-bot: I haz the power, 2015-09-09 09:18:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-09 09:19:08]

Patchset 4 (id:??) landed as
https://crrev.com/3432a1f970e76002552e599cd1148774a3310393
Cr-Commit-Position: refs/heads/master@{#347888}