PrintPreview: Added new histograms for print preview.

PrintPreview: Added new histograms for print preview.

Added code to track the following stats:

- How often users cancel print preview.
- How often users use the advanced settings link.
- How often users print to PDF.
- How often print preview fails.
- How many times users adjust (re-render) the preview before printing.
- Number of printers.

BUG=80917
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86307

[Lei Zhang, 2011-05-13 22:56:20]

+jar

jar - can you look at the comment on print_preview_handler.cc:105?

http://codereview.chromium.org/7016039/diff/1/chrome/browser/ui/webui/print_p...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7016039/diff/1/chrome/browser/ui/webui/print_p...
chrome/browser/ui/webui/print_preview_handler.cc:102: int i = 0;
This is part of the for loop below. Don't separate it from the for loop.

http://codereview.chromium.org/7016039/diff/1/chrome/browser/ui/webui/print_p...
chrome/browser/ui/webui/print_preview_handler.cc:105:
UMA_HISTOGRAM_COUNTS("PrintPreview.TotalNumberOfPrinters",
Can you just name this "NumberOfPrinters" ?

http://codereview.chromium.org/7016039/diff/1/chrome/browser/ui/webui/print_p...
chrome/browser/ui/webui/print_preview_handler.cc:105:
UMA_HISTOGRAM_COUNTS("PrintPreview.TotalNumberOfPrinters",
jar: does this record "a user has N printers"? Or will it increment the counter
by N every time - thus if this gets called M times, it'll report the user has M
x N printers.

http://codereview.chromium.org/7016039/diff/1/chrome/browser/ui/webui/print_p...
chrome/browser/ui/webui/print_preview_handler.cc:311:
"PrintPreview.RegeneratePreviewRequestCount.BeforePrint",
I don't think we need Count in the name.

http://codereview.chromium.org/7016039/diff/1/chrome/browser/ui/webui/print_p...
chrome/browser/ui/webui/print_preview_handler.cc:394:
manage_printers_dialog_request_count_++;
nit: can we be consistent about ++i vs i++?

http://codereview.chromium.org/7016039/diff/1/chrome/browser/ui/webui/print_p...
chrome/browser/ui/webui/print_preview_handler.cc:404: UMA_HISTOGRAM_COUNTS(
Do you need to check and see if the count is greater than 0 here and in other
spots? If so, add the if statement, otherwise remove it from ReportStats().

[kmadhusu, 2011-05-13 23:37:12]

thestig: Addressed review comments. Please review the new patch.

Thanks.

http://codereview.chromium.org/7016039/diff/1/chrome/browser/ui/webui/print_p...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7016039/diff/1/chrome/browser/ui/webui/print_p...
chrome/browser/ui/webui/print_preview_handler.cc:102: int i = 0;
On 2011/05/13 22:56:20, Lei Zhang wrote:
> This is part of the for loop below. Don't separate it from the for loop.

Done.

http://codereview.chromium.org/7016039/diff/1/chrome/browser/ui/webui/print_p...
chrome/browser/ui/webui/print_preview_handler.cc:105:
UMA_HISTOGRAM_COUNTS("PrintPreview.TotalNumberOfPrinters",
On 2011/05/13 22:56:20, Lei Zhang wrote:
> Can you just name this "NumberOfPrinters" ?

Done.

http://codereview.chromium.org/7016039/diff/1/chrome/browser/ui/webui/print_p...
chrome/browser/ui/webui/print_preview_handler.cc:311:
"PrintPreview.RegeneratePreviewRequestCount.BeforePrint",
On 2011/05/13 22:56:20, Lei Zhang wrote:
> I don't think we need Count in the name.

Done.

http://codereview.chromium.org/7016039/diff/1/chrome/browser/ui/webui/print_p...
chrome/browser/ui/webui/print_preview_handler.cc:394:
manage_printers_dialog_request_count_++;
On 2011/05/13 22:56:20, Lei Zhang wrote:
> nit: can we be consistent about ++i vs i++?

Done.

http://codereview.chromium.org/7016039/diff/1/chrome/browser/ui/webui/print_p...
chrome/browser/ui/webui/print_preview_handler.cc:404: UMA_HISTOGRAM_COUNTS(
(repeating our in-person conversation) I have added the if statement only in the
places where we don't want to track the '0' values.

[jar (doing other things), 2011-05-14 03:19:52]

LGTM... but I focused more on the use of histograms than on the logic driving
the selection of samples.

[Lei Zhang, 2011-05-14 04:01:00]

On 2011/05/14 03:19:52, jar wrote:
> LGTM... but I focused more on the use of histograms than on the logic driving
> the selection of samples.

jar: thanks for the feedback.

kmadhusu: perhaps checking the # of printers once per browser session is a
reasonable approximation. Autofill.StoredProfileCount is one such example.

[kmadhusu, 2011-05-16 23:58:32]

thestig: Addressed review comments.

[Lei Zhang, 2011-05-17 00:09:37]

LGTM

http://codereview.chromium.org/7016039/diff/9001/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7016039/diff/9001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:289: if
(!has_logged_printers_count_)
nit: you don't need the if statement. The branching is likely more expensive
than just setting the bit.

[kmadhusu, 2011-05-17 00:22:09]

thestig: Fixed nit.

http://codereview.chromium.org/7016039/diff/9001/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7016039/diff/9001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:289: if
(!has_logged_printers_count_)
On 2011/05/17 00:09:38, Lei Zhang wrote:
> nit: you don't need the if statement. The branching is likely more expensive
> than just setting the bit.

Done.

[kmadhusu, 2011-05-18 19:15:00]

jar: Thanks for your comments. As per your suggestion,  I have created a new
enum to track the different user actions that can be taken from a preview tab.

thestig: I have created new enum and declared some const variables for the
histogram names. Rest of the code is the same.

Please review the new patch.

[jar (doing other things), 2011-05-18 19:27:15]

Again... just focusing on the histogram usage... LGTM.

Thanks for combining things into an enumerated histogram... I think you'll find
the data more readable.

[kmadhusu, 2011-05-23 16:58:31]

thestig: ping

[Lei Zhang, 2011-05-23 18:12:48]

LGTM