De-dup PrefixSet code in SafeBrowsingDatabaseManager.

De-dup PrefixSet code in SafeBrowsingDatabaseManager.

A lot of SideEffectWhiteListPrefixSet related code was mostly a duplicate
 of the other PrefixSet code in the same file.

Only logical side-effect this has is grouping histograms for all PrefixSets
in the same bucket. This will be addressed as a follow-up in
https://codereview.chromium.org/815863002/ where relevant histograms will be
split once again.

This cleanup makes it easier to progress on issue 440517.

BUG=440517
TBR=jwd

Committed: https://crrev.com/fe11d94a8cb3e3b7d942d1cb3f654c75fe664678
Cr-Commit-Position: refs/heads/master@{#309589}

[gab, 2014-12-09 21:21:32]

gab@chromium.org changed reviewers:
+ mattm@chromium.org

[gab, 2014-12-09 21:21:33]

Hey Matt PTAL, thanks again!!

Cheers,
Gab

https://codereview.chromium.org/790703003/diff/1/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_database.cc (left):

https://codereview.chromium.org/790703003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_database.cc:1425:
builder.GetPrefixSetNoHashes());
GetPrefixSetNoHashes() vs GetPrefixSet() seems to be the only difference between
this method and UpdatePrefixSetUrlStore().

It's not clear to me why this difference is required but for the sake of making
this change a no-op, I added a bool parameter to control this on
UpdatePrefixSetUrlStore().

[gab, 2014-12-10 02:37:42]

Bots are failing to patch because this CL is based on
https://codereview.chromium.org/744183002/ which touches closely related
lines... But this is good for review regardless (and I can easily flip which CL
goes in first).

Thanks!
Gab




On 2014/12/09 21:21:33, gab wrote:
> Hey Matt PTAL, thanks again!!
> 
> Cheers,
> Gab
> 
>
https://codereview.chromium.org/790703003/diff/1/chrome/browser/safe_browsing...
> File chrome/browser/safe_browsing/safe_browsing_database.cc (left):
> 
>
https://codereview.chromium.org/790703003/diff/1/chrome/browser/safe_browsing...
> chrome/browser/safe_browsing/safe_browsing_database.cc:1425:
> builder.GetPrefixSetNoHashes());
> GetPrefixSetNoHashes() vs GetPrefixSet() seems to be the only difference
between
> this method and UpdatePrefixSetUrlStore().
> 
> It's not clear to me why this difference is required but for the sake of
making
> this change a no-op, I added a bool parameter to control this on
> UpdatePrefixSetUrlStore().

[gab, 2014-12-10 19:07:16]

FYI, rebased this on trunk, all green now :-)


On 2014/12/10 02:37:42, gab wrote:
> Bots are failing to patch because this CL is based on
> https://codereview.chromium.org/744183002/ which touches closely related
> lines... But this is good for review regardless (and I can easily flip which
CL
> goes in first).
> 
> Thanks!
> Gab
> 
> 
> 
> 
> On 2014/12/09 21:21:33, gab wrote:
> > Hey Matt PTAL, thanks again!!
> > 
> > Cheers,
> > Gab
> > 
> >
>
https://codereview.chromium.org/790703003/diff/1/chrome/browser/safe_browsing...
> > File chrome/browser/safe_browsing/safe_browsing_database.cc (left):
> > 
> >
>
https://codereview.chromium.org/790703003/diff/1/chrome/browser/safe_browsing...
> > chrome/browser/safe_browsing/safe_browsing_database.cc:1425:
> > builder.GetPrefixSetNoHashes());
> > GetPrefixSetNoHashes() vs GetPrefixSet() seems to be the only difference
> between
> > this method and UpdatePrefixSetUrlStore().
> > 
> > It's not clear to me why this difference is required but for the sake of
> making
> > this change a no-op, I added a bool parameter to control this on
> > UpdatePrefixSetUrlStore().

[mattm, 2014-12-12 19:35:50]

Would like to see the histograms split, otherwise good

[gab, 2014-12-15 21:39:47]

On 2014/12/12 19:35:50, mattm wrote:
> Would like to see the histograms split, otherwise good

Hey Matt, I debated doing this now and opted against it as I don't know that
anyone is actively monitoring these histograms. Having a generic histogram which
buckets multiple stores together is fine for now IMO and we can later split it
if a problem is detected and we need more fine-grained data for analysis.

There is no point having fine-grained data if no one is even looking at the
coarser data in the mean time though...

Anyways, I'll do this as a follow-up (a suffixed histogram helper method will be
easier to write after PrefixSetIds are introduced in
https://codereview.chromium.org/794273002/) if you feel strongly about it, but
IMO it's not worth it just yet.

Cheers,
Gab

[mattm, 2014-12-16 03:26:01]

On 2014/12/15 21:39:47, gab wrote:
> On 2014/12/12 19:35:50, mattm wrote:
> > Would like to see the histograms split, otherwise good
> 
> Hey Matt, I debated doing this now and opted against it as I don't know that
> anyone is actively monitoring these histograms. Having a generic histogram
which
> buckets multiple stores together is fine for now IMO and we can later split it
> if a problem is detected and we need more fine-grained data for analysis.
> 
> There is no point having fine-grained data if no one is even looking at the
> coarser data in the mean time though...
> 
> Anyways, I'll do this as a follow-up (a suffixed histogram helper method will
be
> easier to write after PrefixSetIds are introduced in
> https://codereview.chromium.org/794273002/) if you feel strongly about it, but
> IMO it's not worth it just yet.
> 
> Cheers,
> Gab


Having multiple things mixed in the same histogram basically makes it unusable
in my experience.. there are(or were, haven't checked recently) some other
safebrowsing histograms that already are that way and the one time I wanted to
look at them it was pointless.
If these are actually not used it would be better to mark them deprecated and
remove entirely, but you'd have to ask Scott about that. Looks like he is back
now.

[gab, 2014-12-16 14:21:26]

gab@chromium.org changed reviewers:
+ shess@chromium.org

[gab, 2014-12-16 14:45:43]

@Scott: see question for you below.


On 2014/12/16 03:26:01, mattm wrote:
> On 2014/12/15 21:39:47, gab wrote:
> > On 2014/12/12 19:35:50, mattm wrote:
> > > Would like to see the histograms split, otherwise good
> > 
> > Hey Matt, I debated doing this now and opted against it as I don't know that
> > anyone is actively monitoring these histograms. Having a generic histogram
> which
> > buckets multiple stores together is fine for now IMO and we can later split
it
> > if a problem is detected and we need more fine-grained data for analysis.
> > 
> > There is no point having fine-grained data if no one is even looking at the
> > coarser data in the mean time though...
> > 
> > Anyways, I'll do this as a follow-up (a suffixed histogram helper method
will
> be
> > easier to write after PrefixSetIds are introduced in
> > https://codereview.chromium.org/794273002/) if you feel strongly about it,
but
> > IMO it's not worth it just yet.
> > 
> > Cheers,
> > Gab
> 
> 
> Having multiple things mixed in the same histogram basically makes it unusable
> in my experience.. there are(or were, haven't checked recently) some other
> safebrowsing histograms that already are that way and the one time I wanted to
> look at them it was pointless.
> If these are actually not used it would be better to mark them deprecated and
> remove entirely, but you'd have to ask Scott about that. Looks like he is back
> now.

Yes, there are many histograms that bucket multiple stores in the same histogram
(and even more after r303978 as the UwS list and malware list now use the same
helpers for PrefixSets).

e.g.:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf...

We could de-dup these if desired, but only if we're actually going to use them.

@Scott do you think it's worth it or should we just remove most histograms in
SafeBrowsingDatabase?

Here's a display of the histograms under discussion:
https://uma.googleplex.com/p/chrome/histograms/?dayCount=1&endDate=12-15-2014...



@Matt: In either case I'd do this in a follow-up to
https://codereview.chromium.org/794273002/ as having PrefixSetIds will make
writing such reporting helpers easier if we decide to go with it (and this CL is
a precursor to that one).


Thanks!
Gab

[Scott Hess - ex-Googler, 2014-12-16 23:34:32]

Mostly fine with this.  The main reason things are the way they are is because I
didn't want to end up passing a bunch of config info up and down the stack. 
Instead I aimed to provide reasonable components and embedded the config into
the code.  So I'd be somewhat -1 to unifying code at the expense of having to
add too much state, unless someone figures out a sane approach to using
encapsulation to manage it.  Like if this level could just have a vector of
stores which get visited to process everything.

https://codereview.chromium.org/790703003/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/790703003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:1343: }
Is there any reason to expect that add_full_hashes will be non-empty in the case
where store_full_hashes_in_prefix_set is false?  If that can't happen, could you
just drop the flag entirely?

https://codereview.chromium.org/790703003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:1374:
UMA_HISTOGRAM_COUNTS("SB2.DatabaseKilobytes",
I'm not happy with this part.  I understand your point about having a bunch of
useless histograms nobody looks at - but this is one that actually matters,
because it directly reflects the footprint of databases on the user's disk, and
the different databases are different sizes.  For instance, if someone decided
to build a unified storage, aggregate stats wouldn't be useful for estimating
total size or component size.  I don't spend time looking at it, but
periodically I want to know without having to wait a few release cycles...

The cases where I think distinct values are (or would be, as the case may be)
valuable:
  SB2.AddPrefixes (example of a unified one, BTW)
  SB2.SubPrefixes (full hashes might make sense, too, though w/size maybe no
more is needed)
  SB2.*DatabaseKilobytes
  SB2.DatabaseUpdateKilobytes
  SB2.*PrefixSetKilobytes
  SB2.DatabaseFailure (manually distinct, anyhow useful to see if there are
specific failures)
AFAICT, other things are mostly useful in a "Is anyone outlandish?" fashion, but
it's not clear how anyone could be uniquely outlandish.  If generation or write
times are slow, they'll likely be slow for all databases and driven by size.

I would fully support "someone" going in and retrofitting histogram suffixes,
similar to how sql/ does things.  In that world you'd have like
SB2.DatabaseKilobytes.Browse, and histograms.xml would use a histogram_suffixes
stanza to list the variants.  Not sure if it's really worth it, or not.  I do
think that most histograms in this case happen seldom enough that a dynamic
lookup helper like sql/connection.cc wouldn't be excessive overhead.

[gab, 2014-12-18 21:14:44]

Thanks for your comments, see replies below.

Most importantly, I'm proposing https://codereview.chromium.org/815863002/ as a
follow-up to fix the histograms damage suffered in this CL (which still needs to
go in first because it's a precursor to
https://codereview.chromium.org/794273002/ which is used in the proposed CL..!)

https://codereview.chromium.org/790703003/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/790703003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:1343: }
On 2014/12/16 23:34:32, Scott Hess wrote:
> Is there any reason to expect that add_full_hashes will be non-empty in the
case
> where store_full_hashes_in_prefix_set is false?  If that can't happen, could
you
> just drop the flag entirely?

I don't know, but the previous code made that assumption and I don't think this
CL is the right place to change this; happy to add a TODO if you think this
should be looked into.

https://codereview.chromium.org/790703003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:1374:
UMA_HISTOGRAM_COUNTS("SB2.DatabaseKilobytes",
On 2014/12/16 23:34:32, Scott Hess wrote:
> I'm not happy with this part.  I understand your point about having a bunch of
> useless histograms nobody looks at - but this is one that actually matters,
> because it directly reflects the footprint of databases on the user's disk,
and
> the different databases are different sizes.  For instance, if someone decided
> to build a unified storage, aggregate stats wouldn't be useful for estimating
> total size or component size.  I don't spend time looking at it, but
> periodically I want to know without having to wait a few release cycles...
> 
> The cases where I think distinct values are (or would be, as the case may be)
> valuable:
>   SB2.AddPrefixes (example of a unified one, BTW)
>   SB2.SubPrefixes (full hashes might make sense, too, though w/size maybe no
> more is needed)
>   SB2.*DatabaseKilobytes
>   SB2.DatabaseUpdateKilobytes
>   SB2.*PrefixSetKilobytes
>   SB2.DatabaseFailure (manually distinct, anyhow useful to see if there are
> specific failures)
> AFAICT, other things are mostly useful in a "Is anyone outlandish?" fashion,
but
> it's not clear how anyone could be uniquely outlandish.  If generation or
write
> times are slow, they'll likely be slow for all databases and driven by size.
> 
> I would fully support "someone" going in and retrofitting histogram suffixes,
> similar to how sql/ does things.  In that world you'd have like
> SB2.DatabaseKilobytes.Browse, and histograms.xml would use a
histogram_suffixes
> stanza to list the variants.  Not sure if it's really worth it, or not.  I do
> think that most histograms in this case happen seldom enough that a dynamic
> lookup helper like sql/connection.cc wouldn't be excessive overhead.

Ok so how about this:

I'll get the histogram suffixes going by recovering the granularity lost in this
CL for SB2.DatabaseKilobytes and SB2.PrefixSetKilobytes in
https://codereview.chromium.org/815863002/ (done as a separate CL because it
uses IDs introduced in https://codereview.chromium.org/794273002/ which the
current CL is a precursor to..).

(I'm not going to spend more time retrofitting all of the currently unified
histograms but am happy to fix what I'm making worse in this CL -- this CL is
already a tangent of a tangent of what I'm really working on and the histograms
CL is thus a 3*tangent..!)

Also, FWIW, in M40 the generic PrefixSet methods are also used by the new
UnwantedSoftwarePrefixSet so they are currently semi-unified, but the above
proposal fixes this as well.

I could split timing/IO PrefixSet histograms in the same way, but it seems we
agree that unifying those is fine (makes for less histograms to look at and they
can always be expanded if things look wrong and more granularity is required).

[Scott Hess - ex-Googler, 2014-12-19 21:25:48]

LGTM.  [I don't think there was anything pending from Matt.]

Do you want to be in OWNERS?  I'm kind of thinking of moving on, and I'm only in
safe_browsing/OWNERS because one day I got tired of people complaining about
SQLite.

https://codereview.chromium.org/790703003/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/790703003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:1343: }
On 2014/12/18 21:14:44, gab wrote:
> On 2014/12/16 23:34:32, Scott Hess wrote:
> > Is there any reason to expect that add_full_hashes will be non-empty in the
> case
> > where store_full_hashes_in_prefix_set is false?  If that can't happen, could
> you
> > just drop the flag entirely?
> 
> I don't know, but the previous code made that assumption and I don't think
this
> CL is the right place to change this; happy to add a TODO if you think this
> should be looked into.

The previous code wasn't attempting to generalize this function to cover
multiple cases.  The callers are passing a constant for this parameter, so
basically it's a config parameter, which is kind of a code smell.  Stores which
don't want full hashes should just not have full hashes in the first place.

[I guess a TODO will work.]

https://codereview.chromium.org/790703003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:1374:
UMA_HISTOGRAM_COUNTS("SB2.DatabaseKilobytes",
On 2014/12/18 21:14:44, gab wrote:
> On 2014/12/16 23:34:32, Scott Hess wrote:
> > I'm not happy with this part.  I understand your point about having a bunch
of
> > useless histograms nobody looks at - but this is one that actually matters,
> > because it directly reflects the footprint of databases on the user's disk,
> and
> > the different databases are different sizes.  For instance, if someone
decided
> > to build a unified storage, aggregate stats wouldn't be useful for
estimating
> > total size or component size.  I don't spend time looking at it, but
> > periodically I want to know without having to wait a few release cycles...
> > 
> > The cases where I think distinct values are (or would be, as the case may
be)
> > valuable:
> >   SB2.AddPrefixes (example of a unified one, BTW)
> >   SB2.SubPrefixes (full hashes might make sense, too, though w/size maybe no
> > more is needed)
> >   SB2.*DatabaseKilobytes
> >   SB2.DatabaseUpdateKilobytes
> >   SB2.*PrefixSetKilobytes
> >   SB2.DatabaseFailure (manually distinct, anyhow useful to see if there are
> > specific failures)
> > AFAICT, other things are mostly useful in a "Is anyone outlandish?" fashion,
> but
> > it's not clear how anyone could be uniquely outlandish.  If generation or
> write
> > times are slow, they'll likely be slow for all databases and driven by size.
> > 
> > I would fully support "someone" going in and retrofitting histogram
suffixes,
> > similar to how sql/ does things.  In that world you'd have like
> > SB2.DatabaseKilobytes.Browse, and histograms.xml would use a
> histogram_suffixes
> > stanza to list the variants.  Not sure if it's really worth it, or not.  I
do
> > think that most histograms in this case happen seldom enough that a dynamic
> > lookup helper like sql/connection.cc wouldn't be excessive overhead.
> 
> Ok so how about this:
> 
> I'll get the histogram suffixes going by recovering the granularity lost in
this
> CL for SB2.DatabaseKilobytes and SB2.PrefixSetKilobytes in
> https://codereview.chromium.org/815863002/ (done as a separate CL because it
> uses IDs introduced in https://codereview.chromium.org/794273002/ which the
> current CL is a precursor to..).
> 
> (I'm not going to spend more time retrofitting all of the currently unified
> histograms but am happy to fix what I'm making worse in this CL -- this CL is
> already a tangent of a tangent of what I'm really working on and the
histograms
> CL is thus a 3*tangent..!)
> 
> Also, FWIW, in M40 the generic PrefixSet methods are also used by the new
> UnwantedSoftwarePrefixSet so they are currently semi-unified, but the above
> proposal fixes this as well.
> 
> I could split timing/IO PrefixSet histograms in the same way, but it seems we
> agree that unifying those is fine (makes for less histograms to look at and
they
> can always be expanded if things look wrong and more granularity is required).

Acknowledged.

[gab, 2014-12-22 16:11:24]

Thanks, TODO added.

https://codereview.chromium.org/790703003/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/790703003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:1343: }
On 2014/12/19 21:25:47, Scott Hess wrote:
> On 2014/12/18 21:14:44, gab wrote:
> > On 2014/12/16 23:34:32, Scott Hess wrote:
> > > Is there any reason to expect that add_full_hashes will be non-empty in
the
> > case
> > > where store_full_hashes_in_prefix_set is false?  If that can't happen,
could
> > you
> > > just drop the flag entirely?
> > 
> > I don't know, but the previous code made that assumption and I don't think
> this
> > CL is the right place to change this; happy to add a TODO if you think this
> > should be looked into.
> 
> The previous code wasn't attempting to generalize this function to cover
> multiple cases.  The callers are passing a constant for this parameter, so
> basically it's a config parameter, which is kind of a code smell.  Stores
which
> don't want full hashes should just not have full hashes in the first place.

Agreed.

> 
> [I guess a TODO will work.]

Done.

[gab, 2014-12-22 16:28:25]

On 2014/12/19 21:25:48, Scott Hess wrote:
> LGTM.  [I don't think there was anything pending from Matt.]
> 
> Do you want to be in OWNERS?  I'm kind of thinking of moving on, and I'm only
in
> safe_browsing/OWNERS because one day I got tired of people complaining about
> SQLite.

I would be interested yes, if Matt agrees.

I'm beginning to be fairly familiar with the database (how everything is hooked
together; haven't much explored the on-disk formats much yet) and the logic
behind the SafeBrowsingResourceThrottle which allows safe browsing to inspect
every network requests and act accordingly.

I would still have to lean on Matt for the UI (interstitials) and
chrome/renderer side of things (and would still of course keep him in the loop
for any big changes, whether it involves UI/renderer or not, at least
initially).

My upcoming projects are all up the safe browsing alley (we can chat offline if
you care about details).

The only nit I think is that I'm thinking of stepping out of safe browsing work
for a little while as I'll join the M42 performance milestone efforts full-time
in January. But at the same time I'm thinking of mentoring somebody to pick up
the safe browsing projects I have in mind so it would indeed help that I become
an owner here.

Cheers,
Gab

[gab, 2014-12-22 16:31:03]

gab@chromium.org changed reviewers:
+ jwd@chromium.org

[gab, 2014-12-22 16:31:03]

+jwd for histograms, thanks!

[gab, 2014-12-22 18:14:48]

gab@chromium.org changed reviewers:
+ isherman@chromium.org, jar@chromium.org, mpearson@chromium.org
- jwd@chromium.org

[gab, 2014-12-22 18:14:49]

@jar/isherman/mpearson: looks like jwd@ is out for the holidays, is one of you
in today? If so, could you please take a look at this histograms.xml change?

Thanks!
Gab

[gab, 2014-12-23 19:08:42]

gab@chromium.org changed reviewers:
+ jwd@chromium.org
- isherman@chromium.org, jar@chromium.org, mpearson@chromium.org

[gab, 2014-12-23 19:11:55]

On 2014/12/22 18:14:49, gab wrote:
> @jar/isherman/mpearson: looks like jwd@ is out for the holidays, is one of you
> in today? If so, could you please take a look at this histograms.xml change?
> 

Looks like everyone is away... TBR jwd@ (others to CC), already planning a
follow-up CL @ https://codereview.chromium.org/815863002/ and will be happy to
respond to any post-commit comments here on that CL.

Cheers!
Gab

[gab, 2014-12-23 19:12:03]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-12-23 19:12:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/790703003/80001

[commit-bot: I haz the power, 2014-12-23 21:37:53]

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

[commit-bot: I haz the power, 2014-12-23 21:37:54]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[gab, 2014-12-23 21:44:51]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-12-23 21:45:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/790703003/80001

[commit-bot: I haz the power, 2014-12-24 00:22:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-12-24 00:22:48]

Patchset 5 (id:??) landed as
https://crrev.com/fe11d94a8cb3e3b7d942d1cb3f654c75fe664678
Cr-Commit-Position: refs/heads/master@{#309589}

[Mark P, 2015-01-02 19:56:19]

mpearson@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2015-01-02 19:56:20]

Please follow up with these histograms.xml comments in
https://codereview.chromium.org/815863002/

thanks,
mark

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

https://codereview.chromium.org/790703003/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:29769: -    Replaced by
SB2.BrowseDatabaseKilobytes.
This doesn't seem right to me.  You're taking a histogram name that had
previously been used and then deprecated (marked as obsolete), then using it
again for something else.  I would encourage you to use an unused name (even if
you only plan to emit histograms with suffixes to this).

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

https://codereview.chromium.org/790703003/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:29770: +    The size of one of the
browsing SafeBrowsing database file on disk in
nit: "browsing SafeBrowsing" seems redundant to me.

https://codereview.chromium.org/790703003/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:29771: +    kilobytes, after an update
has occurred.
(under the you-touched-it-you-must-improve-it rule)
Be more precise about when this histogram is emitted.

Ditto all the (non-deprecated) histograms below that you touch in this
changelist.
(Though I'm especially concerned here because I don't know exactly what this
caveat "after an update has occurred" means in turns of restriction what times
it would otherwise be omitted.)

[gab, 2015-01-05 18:24:38]

Thanks, comments addressed on https://codereview.chromium.org/815863002/

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

https://codereview.chromium.org/790703003/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:29769: -    Replaced by
SB2.BrowseDatabaseKilobytes.
On 2015/01/02 19:56:20, Mark P wrote:
> This doesn't seem right to me.  You're taking a histogram name that had
> previously been used and then deprecated (marked as obsolete), then using it
> again for something else.  I would encourage you to use an unused name (even
if
> you only plan to emit histograms with suffixes to this).
> 

Done, introduced SB2.DatabaseSizeKilobytes for this.

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

https://codereview.chromium.org/790703003/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:29770: +    The size of one of the
browsing SafeBrowsing database file on disk in
On 2015/01/02 19:56:20, Mark P wrote:
> nit: "browsing SafeBrowsing" seems redundant to me.

Oops, definitely a typo.

https://codereview.chromium.org/790703003/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:29771: +    kilobytes, after an update
has occurred.
On 2015/01/02 19:56:20, Mark P wrote:
> (under the you-touched-it-you-must-improve-it rule)
> Be more precise about when this histogram is emitted.
> 
> Ditto all the (non-deprecated) histograms below that you touch in this
> changelist.
> (Though I'm especially concerned here because I don't know exactly what this
> caveat "after an update has occurred" means in turns of restriction what times
> it would otherwise be omitted.)

Added more details about when an update occurs.

As for the histograms below I was mostly fixing the fact that they now are
reported for many files rather than a single one. I do not think this calls for
digging into and adding more details to each of these histograms.

[Ilya Sherman, 2015-01-06 00:21:16]

isherman@chromium.org changed reviewers:
+ isherman@chromium.org

[Ilya Sherman, 2015-01-06 00:21:17]

One more nit:

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

https://codereview.chromium.org/790703003/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:30357: -</histogram>
Please mark this obsolete rather than removing it entirely.

[gab, 2015-01-06 13:37:30]

@Ilya: please see reply inline.

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

https://codereview.chromium.org/790703003/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:30357: -</histogram>
On 2015/01/06 00:21:17, Ilya Sherman wrote:
> Please mark this obsolete rather than removing it entirely.

It was never ever reported, instead the code was reporting
"SB2.SideEffectFreePrefixSetWrite" which I added (as obsolete) above.

Is that fine? Or does some histograms infra depend on this histogram being
marked as obsolete despite having never received any reports for it?

[Ilya Sherman, 2015-01-06 22:49:02]

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

https://codereview.chromium.org/790703003/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:30357: -</histogram>
On 2015/01/06 13:37:30, gab wrote:
> On 2015/01/06 00:21:17, Ilya Sherman wrote:
> > Please mark this obsolete rather than removing it entirely.
> 
> It was never ever reported, instead the code was reporting
> "SB2.SideEffectFreePrefixSetWrite" which I added (as obsolete) above.
> 
> Is that fine? Or does some histograms infra depend on this histogram being
> marked as obsolete despite having never received any reports for it?

I see -- in that case, there is indeed no reason to mark it as <obsolete>. 
Thanks for the clarification.