Improve the comments around content settings histograms.

Improve the comments around content settings histograms.

1. Explicitly mention that new histogram entries must be added to the end.
   This is necessary to keep consistent numbering of the histograms.
   However, it is not necessary for the ordering to be the same as in
   ContentSettingsType - in fact, the requirement to replace removed
   settings with placeholders means that there won't be perfect
   correspondence.

2. Refer to |kHistogramOrder| from content_settings_types.h, since
   ContentSettingsTypeHistogram does not exist anymore.

3. Remove the #ifdef for PROTECTED_MEDIA_IDENTIFIER in |kHistogramOrder|.
   PROTECTED_MEDIA_IDENTIFIER is not defined on some platforms, but that
   is true for many content settings; this is captured in
   WebsiteSettingsRegistry, and should not be specified in histograms.

BUG=None

[msramek, 2016-07-08 10:45:30]

Patchset #1 (id:1) has been deleted

[msramek, 2016-07-08 10:46:41]

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

[msramek, 2016-07-08 10:46:42]

Hi Raymes,

please have a look!

Thanks,
Martin

[raymes, 2016-07-12 07:27:36]

lgtm - Thanks Martin!

It was suggested separately that we just hardcode the constants which are used
in histograms.xml. What do you think about that suggestion?

[msramek, 2016-07-12 14:18:50]

On 2016/07/12 07:27:36, raymes wrote:
> lgtm - Thanks Martin!
> 
> It was suggested separately that we just hardcode the constants which are used
> in histograms.xml. What do you think about that suggestion?

You mean hardcoding the result of ContentSettingTypeToHistogramValue() into the
registration of that ContentSettingsType? Sounds good to me.

We would define "int last_histogram_id". With every new ContentSettingsType, we
would increment that counter and attach the new value to that new
ContentSettingsType. Removing a ContentSettingsType requires no action, its ID
is simply disused. |last_histogram_id| itself is equal to |num_values| which was
the out-parameter ContentSettingTypeToHistogramValue().

This will also require less attention from code reviewers, because you'll notice
the numbers when you register a new content setting. The only disadvantage that
I see is that the number will have to be used in WebsiteSettingsRegistry as well
as ContentSettingsRegistry, so it's not clear where to define it.

...Did I understand you correctly, and does this plan sound good? If so, I can
do that in this CL.

[raymes, 2016-07-13 01:44:03]

On 2016/07/12 14:18:50, msramek wrote:
> On 2016/07/12 07:27:36, raymes wrote:
> > lgtm - Thanks Martin!
> > 
> > It was suggested separately that we just hardcode the constants which are
used
> > in histograms.xml. What do you think about that suggestion?
> 
> You mean hardcoding the result of ContentSettingTypeToHistogramValue() into
the
> registration of that ContentSettingsType? Sounds good to me.
> 
> We would define "int last_histogram_id". With every new ContentSettingsType,
we
> would increment that counter and attach the new value to that new
> ContentSettingsType. Removing a ContentSettingsType requires no action, its ID
> is simply disused. |last_histogram_id| itself is equal to |num_values| which
was
> the out-parameter ContentSettingTypeToHistogramValue().
> 
> This will also require less attention from code reviewers, because you'll
notice
> the numbers when you register a new content setting. The only disadvantage
that
> I see is that the number will have to be used in WebsiteSettingsRegistry as
well
> as ContentSettingsRegistry, so it's not clear where to define it.
> 
> ...Did I understand you correctly, and does this plan sound good? If so, I can
> do that in this CL.

I was describing something simpler. Instead of having the array and relying on
ordering, just hardcoding the histogram values, e.g:
ContentSettingHistogramValue kHistogramValue[] = {
  { CONTENT_SETTINGS_TYPE_COOKIES, 0 },
  { CONTENT_SETTINGS_TYPE_IMAGES, 1 },
  ...
};

The integer would match whatever is in histograms.xml. The benefit here is that
adding or removing things from the array won't break the histogram. WDYT?

[msramek, 2017-03-13 10:17:59]

Closing in favor of https://codereview.chromium.org/2728303002/.