Drop diagnostics reference to out-dated database.

Drop diagnostics reference to out-dated database.

In 2010, the Thumbnails database was migrated to Favicons.  The
diagnostics code still refers to Thumbnails.  At some point, Thumbnails
has become a directory containing a leveldb.  Attempting to open a
directory as a SQLite database fails with SQLITE_CANTOPEN, causing the
diagnostics code to generate red herrings.

Rename the "thumbnails" cases to "favicons".  Also add in "Top Sites",
which was missing from the diagnostics code.

BUG=617226
Committed: https://crrev.com/d31f1808e5bd040d54aca2b7a2a6c9767b5860ec
Cr-Commit-Position: refs/heads/master@{#397949}

[Scott Hess - ex-Googler, 2016-06-03 21:15:30]

shess@chromium.org changed reviewers:
+ cpu@chromium.org, mpearson@chromium.org

[Scott Hess - ex-Googler, 2016-06-03 21:15:31]

cpu@ for chrome/browser/diagnostics/OWNERS
mpearson@ for tools/metrics/histograms/OWNERS

https://codereview.chromium.org/2038933002/diff/1/chrome/browser/diagnostics/...
File chrome/browser/diagnostics/diagnostics_metrics.cc (right):

https://codereview.chromium.org/2038933002/diff/1/chrome/browser/diagnostics/...
chrome/browser/diagnostics/diagnostics_metrics.cc:53:
{"SQLiteIntegrityTopSites", "Top Sites database"},
I could see an argument for leaving the THUMBNAILS case in place, and adding new
FAVICONS case, because THUMBNAILS has been generating bad data.  This spot would
leave a stale never-to-be-used value.  I went with the rename because it was
what should have been done in 2010 or whenever the migration started.  AFAICT,
if someone is paying attention to this histogram, they aren't doing much about
it, and even if I added separate histograms, we'd still be seeing data on the
original histogram until it idles out, so the gain would be minimal, IMHO.

[Scott Hess - ex-Googler, 2016-06-03 21:15:37]

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

[commit-bot: I haz the power, 2016-06-03 21:16:34]

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

[commit-bot: I haz the power, 2016-06-03 21:43:46]

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

[commit-bot: I haz the power, 2016-06-03 21:43:47]

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

[Mark P, 2016-06-03 21:54:55]

Hmmm.

It sounds like you're saying that the "SQLite Integrity Thumbnails Test" entry
in the histogram Diagnostics.TestFailures should always report a failure because
it would be trying to load the wrong sort of file.  The counts should be
enormous.  However, if I look at that histogram, the counts are relatively low.

What am I misunderstanding?

Or is the bucket labeled "SQLite Integrity Thumbnails Test" actually already
testing the favicons integrity, not the thumbnail integrity?

--mark

[Scott Hess - ex-Googler, 2016-06-03 21:56:03]

Update diagnostic test count.

[Scott Hess - ex-Googler, 2016-06-03 21:57:57]

Oops, missed the by-name histograms that were in there.

[Scott Hess - ex-Googler, 2016-06-03 21:58:16]

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

[commit-bot: I haz the power, 2016-06-03 21:59:05]

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

[Scott Hess - ex-Googler, 2016-06-03 22:03:23]

On 2016/06/03 21:54:55, Mark P wrote:
> It sounds like you're saying that the "SQLite Integrity Thumbnails Test" entry
> in the histogram Diagnostics.TestFailures should always report a failure
because
> it would be trying to load the wrong sort of file.  The counts should be
> enormous.  However, if I look at that histogram, the counts are relatively
low.

I was suprised it was so low, too.  It should always report a failure if the
Thumbnails is present and is a directory - but I'm not entirely certain where
the Thumbnails directory is coming from, so it's possible that not everyone has
it.  Pre-patch I get a failure on that test every time I run with --diagnostics.

> Or is the bucket labeled "SQLite Integrity Thumbnails Test" actually already
> testing the favicons integrity, not the thumbnail integrity?

It's testing kThumbnailsFile which is "Thumbnails", so I don't think it's
accidentally testing the right thing.

It may be that the other tests in the bucket are simply swamping this test.  I
don't fully see how that could be it, I think it's more likely that most people
don't have Thumbnails/ in the first place.

[Mark P, 2016-06-03 22:06:40]

On 2016/06/03 22:03:23, Scott Hess wrote:
> On 2016/06/03 21:54:55, Mark P wrote:
> > It sounds like you're saying that the "SQLite Integrity Thumbnails Test"
entry
> > in the histogram Diagnostics.TestFailures should always report a failure
> because
> > it would be trying to load the wrong sort of file.  The counts should be
> > enormous.  However, if I look at that histogram, the counts are relatively
> low.
> 
> I was suprised it was so low, too.  It should always report a failure if the
> Thumbnails is present and is a directory - but I'm not entirely certain where
> the Thumbnails directory is coming from, so it's possible that not everyone
has
> it.  Pre-patch I get a failure on that test every time I run with
--diagnostics.
> 
> > Or is the bucket labeled "SQLite Integrity Thumbnails Test" actually already
> > testing the favicons integrity, not the thumbnail integrity?
> 
> It's testing kThumbnailsFile which is "Thumbnails", so I don't think it's
> accidentally testing the right thing.
> 
> It may be that the other tests in the bucket are simply swamping this test.  I
> don't fully see how that could be it, I think it's more likely that most
people
> don't have Thumbnails/ in the first place.

Given that the old bucket is correctly labeled (even if we don't fully
understand
why the value is so low), please don't reuse (rename) the old bucket.  Add a new
histogram bucket for favicons.

--mark

[Mark P, 2016-06-03 22:06:54]

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

https://codereview.chromium.org/2038933002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:8572: +  <owner>Please list the metric's
owners. Add more owner tags as needed.</owner>
Please fill in all these owner fields.

[Scott Hess - ex-Googler, 2016-06-03 22:16:09]

On 2016/06/03 22:03:23, Scott Hess wrote:
> On 2016/06/03 21:54:55, Mark P wrote:
> > It sounds like you're saying that the "SQLite Integrity Thumbnails Test"
entry
> > in the histogram Diagnostics.TestFailures should always report a failure
> because
> > it would be trying to load the wrong sort of file.  The counts should be
> > enormous.  However, if I look at that histogram, the counts are relatively
> low.
> 
> I was suprised it was so low, too.  It should always report a failure if the
> Thumbnails is present and is a directory - but I'm not entirely certain where
> the Thumbnails directory is coming from, so it's possible that not everyone
has
> it.  Pre-patch I get a failure on that test every time I run with
--diagnostics.

In the "file not found" case, --diagnostics gives:
> [PASS] Thumbnails database
>        File not found (but that is OK)
which I believe is TEST_OK and will be counted as a success.

I have a Thumbnails/ directory in my Chromium profile, and in my canary-channel
profile, but not in my beta-channel profile.  On another development machine,
it's not in my Chromium profile, but is in each of my canary-channel and
beta-channel profiles.  All examples of trivial size, so I'm inclined to think
they are empty.  My best guess is chrome/browser/search/suggestions/, based on
suggestions_service_factory.cc.

I don't _think_ that any old code attempting sql::Connection::Delete() would be
able to delete a directory, so I don't _think_ that old code could accidentally
keep stomping on someone trying to put a leveldb in there.

[Scott Hess - ex-Googler, 2016-06-03 22:30:41]

On 2016/06/03 22:06:40, Mark P wrote:
> Given that the old bucket is correctly labeled (even if we don't fully
> understand
> why the value is so low), please don't reuse (rename) the old bucket.  Add a
new
> histogram bucket for favicons.

OK, I'll obsolete the old bucket and make the new ones new.

[Scott Hess - ex-Googler, 2016-06-03 22:46:58]

Restore thumbnails as obsolete, add favicons as new.

[Scott Hess - ex-Googler, 2016-06-03 22:50:25]

Own a bunch of diagnostics histograms

[Scott Hess - ex-Googler, 2016-06-03 22:51:19]

Scott Hess wrote:
> Restore thumbnails as obsolete, add favicons as new.
> Own a bunch of diagnostics histograms

OK, I think your points are addressed.

[Scott Hess - ex-Googler, 2016-06-03 22:51:35]

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

https://codereview.chromium.org/2038933002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:8572: +  <owner>Please list the metric's
owners. Add more owner tags as needed.</owner>
On 2016/06/03 22:06:54, Mark P wrote:
> Please fill in all these owner fields.

:-(.  I'll take on the SQLite ones (except for NSS, I think those are ChromeOS),
and the overall ran-the-tests ones.  I don't have enough understanding of what
this code is intended to do to confidently take overall ownership.

[Scott Hess - ex-Googler, 2016-06-03 22:51:57]

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

[commit-bot: I haz the power, 2016-06-03 22:52:33]

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

[cpu_(ooo_6.6-7.5), 2016-06-03 23:43:40]

diagnostics/* lgtm

[commit-bot: I haz the power, 2016-06-04 00:17:25]

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

[commit-bot: I haz the power, 2016-06-04 00:17:26]

Dry run: This issue passed the CQ dry run.

[Mark P, 2016-06-04 04:11:19]

minor nit, otherwise histograms.xml lgtm

--mark

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

https://codereview.chromium.org/2038933002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:8389: +    Deprecated 05/2016 in Issue
617226, has not been valid for many years.
nit: , -> ;

ditto below

[Scott Hess - ex-Googler, 2016-06-04 21:30:51]

thanks!

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

https://codereview.chromium.org/2038933002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:8389: +    Deprecated 05/2016 in Issue
617226, has not been valid for many years.
On 2016/06/04 04:11:19, Mark P wrote:
> nit: , -> ;
> 
> ditto below

Done.

[Scott Hess - ex-Googler, 2016-06-04 21:31:03]

comma to semicolon

[Scott Hess - ex-Googler, 2016-06-04 21:31:53]

The CQ bit was checked by shess@chromium.org

[Scott Hess - ex-Googler, 2016-06-04 21:31:54]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2038933002/#ps100001
(title: "comma to semicolon")

[commit-bot: I haz the power, 2016-06-04 21:32:03]

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

[commit-bot: I haz the power, 2016-06-05 20:12:09]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-05 20:13:34]

Description was changed from

==========
Drop diagnostics reference to out-dated database.

In 2010, the Thumbnails database was migrated to Favicons.  The
diagnostics code still refers to Thumbnails.  At some point, Thumbnails
has become a directory containing a leveldb.  Attempting to open a
directory as a SQLite database fails with SQLITE_CANTOPEN, causing the
diagnostics code to generate red herrings.

Rename the "thumbnails" cases to "favicons".  Also add in "Top Sites",
which was missing from the diagnostics code.

BUG=617226
==========

to

==========
Drop diagnostics reference to out-dated database.

In 2010, the Thumbnails database was migrated to Favicons.  The
diagnostics code still refers to Thumbnails.  At some point, Thumbnails
has become a directory containing a leveldb.  Attempting to open a
directory as a SQLite database fails with SQLITE_CANTOPEN, causing the
diagnostics code to generate red herrings.

Rename the "thumbnails" cases to "favicons".  Also add in "Top Sites",
which was missing from the diagnostics code.

BUG=617226

Committed: https://crrev.com/d31f1808e5bd040d54aca2b7a2a6c9767b5860ec
Cr-Commit-Position: refs/heads/master@{#397949}
==========

[commit-bot: I haz the power, 2016-06-05 20:13:35]

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