Remove old SBInterstitial counter

Remove old SBInterstitial counter

The first version of the Safe Browsing interstitial used a SBInterstitial counter. It was subsequently replaced by two histograms: SB2.InterstitialAction and SB2.InterstitialActionDetails. This deprecates the SBInterstitial counter.

BUG=346120
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287921

[felt, 2014-08-04 01:56:21]

mattm, mpearson, PTAL.

I've never seen the histogram suffix thing before, so please let me know if
deleting it will break something. I don't see any evidence of suffixes actually
appearing in UMA logs for anything under the name SBInterstitial or
SB2.InterstitialAction, so I don't understand what the suffixes were being used
for.

[Mark P, 2014-08-04 17:24:16]

On 2014/08/04 01:56:21, felt wrote:
> mattm, mpearson, PTAL.
> 
> I've never seen the histogram suffix thing before, so please let me know if
> deleting it will break something. I don't see any evidence of suffixes
actually
> appearing in UMA logs for anything under the name SBInterstitial or
> SB2.InterstitialAction, so I don't understand what the suffixes were being
used
> for.

Yes, it seems safe to delete the suffix list thing.  It appears to not be used.

As for the user actions within in the code, if you're deleting them, can you
please modify actions.xml to mark all those actions as obsolete?

thanks,
mark

[chromium-reviews, 2014-08-04 17:29:50]

If the suffix list documents real histograms that were emitted (and that we
have data for from old versions), that block should be marked as obsolete
instead. (If such histograms were never emitted, then deleting it is OK I
guess.)


On Mon, Aug 4, 2014 at 1:24 PM, <mpearson@chromium.org> wrote:

> On 2014/08/04 01:56:21, felt wrote:
>
>> mattm, mpearson, PTAL.
>>
>
>  I've never seen the histogram suffix thing before, so please let me know
>> if
>> deleting it will break something. I don't see any evidence of suffixes
>>
> actually
>
>> appearing in UMA logs for anything under the name SBInterstitial or
>> SB2.InterstitialAction, so I don't understand what the suffixes were being
>>
> used
>
>> for.
>>
>
> Yes, it seems safe to delete the suffix list thing.  It appears to not be
> used.
>
> As for the user actions within in the code, if you're deleting them, can
> you
> please modify actions.xml to mark all those actions as obsolete?
>
> thanks,
> mark
>
>
> https://codereview.chromium.org/439683002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark P, 2014-08-04 17:45:12]

Alexei is right.  I spoke too soon.  The histograms that the suffix list applies
to still exist and are not even marked as obsolete.  Admittedly, no histograms
with either of those suffices are being emitted to nowadays.  Nonetheless, it
ought to remain to aid interpreting older data if necessary.

--mark

[mattm, 2014-08-04 20:26:07]

https://codereview.chromium.org/439683002/diff/20001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (left):

https://codereview.chromium.org/439683002/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:710:
content::RecordComputedAction(action);
This is not technically a histogram. (It's under the "counts" section of the uma
dashboard. The "SBInterstitial" also showing up in histogram_suffixes is
coincidental.) It's not only relevant to the v1 interstitial, it's just old.

Probably still want to get rid of it since the more useful actual histograms
SB2.InterstitialAction / SB2.InterstitialActionDetails also exist, but it
doesn't quite match the CL description.

[felt, 2014-08-05 04:33:48]

On 2014/08/04 20:26:07, mattm wrote:
>
https://codereview.chromium.org/439683002/diff/20001/chrome/browser/safe_brow...
> File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (left):
> 
>
https://codereview.chromium.org/439683002/diff/20001/chrome/browser/safe_brow...
> chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:710:
> content::RecordComputedAction(action);
> This is not technically a histogram. (It's under the "counts" section of the
uma
> dashboard. The "SBInterstitial" also showing up in histogram_suffixes is
> coincidental.) It's not only relevant to the v1 interstitial, it's just old.
> 
> Probably still want to get rid of it since the more useful actual histograms
> SB2.InterstitialAction / SB2.InterstitialActionDetails also exist, but it
> doesn't quite match the CL description.

Thanks for the explanation. I didn't understand what was going on here. Changes
since you all last looked:

* I left the SBInterstitial suffix list alone, since it will be useful in
understanding old histogram entries.
* I deprecated the SBInterstitial* counters.

[mattm, 2014-08-05 06:56:21]

https://codereview.chromium.org/439683002/diff/40001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/439683002/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:9943: <action
name="SBInterstitialMultipleDontProceed_V1">
missed this one

https://codereview.chromium.org/439683002/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/439683002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:52189: +  <suffix name="V2"
label="version 2+ (new interstitial)"/>
The suffix hasn't been recorded for a long time (since
http://src.chromium.org/viewvc/chrome?view=rev&revision=167056) so I think this
is inaccurate

[Mark P, 2014-08-05 16:03:43]

https://codereview.chromium.org/439683002/diff/40001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/439683002/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:9868: <obsolete>Replaced by
SB2.InterstitialAction</obsolete>
Can you make these
Replaced by the histogram SB2.InterstitialAction
?

[felt, 2014-08-06 19:51:07]

https://codereview.chromium.org/439683002/diff/40001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/439683002/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:9868: <obsolete>Replaced by
SB2.InterstitialAction</obsolete>
On 2014/08/05 16:03:42, Mark P wrote:
> Can you make these
> Replaced by the histogram SB2.InterstitialAction
> ?

Done.

https://codereview.chromium.org/439683002/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:9943: <action
name="SBInterstitialMultipleDontProceed_V1">
On 2014/08/05 06:56:20, mattm wrote:
> missed this one

Done.

https://codereview.chromium.org/439683002/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/439683002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:52189: +  <suffix name="V2"
label="version 2+ (new interstitial)"/>
On 2014/08/05 06:56:20, mattm wrote:
> The suffix hasn't been recorded for a long time (since
> http://src.chromium.org/viewvc/chrome?view=rev&revision=167056) so I think
this
> is inaccurate

Done.

[Mark P, 2014-08-06 20:08:07]

lgtm
one comment below

https://codereview.chromium.org/439683002/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/439683002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:52189: <suffix name="V2" label="version
2 (new interstitial)"/>
I think mattm@ was implying you should give this a better description because
it's been so long deprecated.  I suggest removing the " (new interstitial)".

[felt, 2014-08-06 20:19:35]

https://codereview.chromium.org/439683002/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/439683002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:52189: <suffix name="V2" label="version
2 (new interstitial)"/>
On 2014/08/06 20:08:07, Mark P wrote:
> I think mattm@ was implying you should give this a better description because
> it's been so long deprecated.  I suggest removing the " (new interstitial)".

Done.

[mattm, 2014-08-06 20:44:46]

lgtm

[felt, 2014-08-06 20:47:22]

The CQ bit was checked by felt@chromium.org

[commit-bot: I haz the power, 2014-08-06 20:49:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/felt@chromium.org/439683002/80001

[commit-bot: I haz the power, 2014-08-07 00:56:24]

Change committed as 287921