Adding basic Metro Print Metrics.

Adding basic Metro Print Metrics.

BUG=136041
TEST=Make sure metro print actions are logged as histograms.


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

[MAD, 2012-08-10 21:44:51]

Are you OK with these?
(feel free to redirect to another owner/reviewer).

I use hard-coded strings and numbers because I couldn't find an obvious shared
place for this (though print_destination_interface.h was tempting), and I plan
on removing these as soon as the metro code will be part of the chrome dll and
I'll be able to put the metrics code there... Currently, I can't because it's a
separate DLL... :-/

Thanks!

BYE
MAD...

[Albert Bodenhamer, 2012-08-10 21:48:07]

lgtm

https://chromiumcodereview.appspot.com/10831272/diff/1/chrome/browser/printin...
File chrome/browser/printing/print_view_manager.cc (right):

https://chromiumcodereview.appspot.com/10831272/diff/1/chrome/browser/printin...
chrome/browser/printing/print_view_manager.cc:113: // TODO(mad): Remove this
once we can send user metrics from the metro driver.
File a bug on moving the code and reference it from the TODO.

[MAD, 2012-08-13 14:38:09]

Cool, thanks... CQing now... 

BYE
MAD!

https://chromiumcodereview.appspot.com/10831272/diff/1/chrome/browser/printin...
File chrome/browser/printing/print_view_manager.cc (right):

https://chromiumcodereview.appspot.com/10831272/diff/1/chrome/browser/printin...
chrome/browser/printing/print_view_manager.cc:113: // TODO(mad): Remove this
once we can send user metrics from the metro driver.
On 2012/08/10 21:48:07, Albert Bodenhamer wrote:
> File a bug on moving the code and reference it from the TODO.

Done.

[commit-bot: I haz the power, 2012-08-13 14:38:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/10831272/3002

[commit-bot: I haz the power, 2012-08-13 16:36:05]

Change committed as 151285

[Ilya Sherman, 2012-08-13 22:34:00]

Apologies for the delay.  Neither of these comments is super important, but
they're bits of cleanup that would be nice to address in a follow-up CL if you
can.  Thanks.

http://codereview.chromium.org/10831272/diff/3002/chrome/browser/printing/pri...
File chrome/browser/printing/print_view_manager.cc (right):

http://codereview.chromium.org/10831272/diff/3002/chrome/browser/printing/pri...
chrome/browser/printing/print_view_manager.cc:115:
UMA_HISTOGRAM_ENUMERATION("Metro.Print", 0, 2);
nit: Please use named constants from an enum rather than fixed numbers.  The two
advantages of using enum constants are that (1) they're self documenting, and
(2) it's harder to get the number of buckets wrong due to a typo, especially if
you later decide to add additional metrics to this histogram.

http://codereview.chromium.org/10831272/diff/3002/printing/printing_context_w...
File printing/printing_context_win.cc (right):

http://codereview.chromium.org/10831272/diff/3002/printing/printing_context_w...
printing/printing_context_win.cc:237: UMA_HISTOGRAM_ENUMERATION("Metro.Print",
1, 2);
nit: I recommend having a single location in the codebase that logs any given
histogram.  Typically, I've seen people create small utility
header+implementation files for this purpose.  The advantages are (1) it's much
less likely that you'll accidentally name the histogram differently in different
call sites due to a typo; (2) you can't forget to update one of the call sites
when updating the histogram; and (3) Jim tells me we end up with slightly better
compiled code if there's only one definition for each histogram (this last
reason is pretty minor though).

[MAD, 2012-08-14 16:08:28]

As mentioned in the TODOs, this code will go away once we can send UMA logs
from the Metro code (which currently can't be done because it's in a
separate DLL)... Once it's in the Metro Driver code, it will satisfy the
suggested requirements, which I agree with... For this temporary code, I
didn't think it was worth finding a common place for this... But we wanted
to get some preliminary data in the mean time...

Hope it's OK...

On Mon, Aug 13, 2012 at 6:34 PM, <isherman@chromium.org> wrote:

> Apologies for the delay.  Neither of these comments is super important, but
> they're bits of cleanup that would be nice to address in a follow-up CL if
> you
> can.  Thanks.
>
>
> http://codereview.chromium.**org/10831272/diff/3002/chrome/**
>
browser/printing/print_view_**manager.cc<http://codereview.chromium.org/10831272/diff/3002/chrome/browser/printing/print_view_manager.cc>
> File chrome/browser/printing/print_**view_manager.cc (right):
>
> http://codereview.chromium.**org/10831272/diff/3002/chrome/**
>
browser/printing/print_view_**manager.cc#newcode115<http://codereview.chromium.org/10831272/diff/3002/chrome/browser/printing/print_view_manager.cc#newcode115>
> chrome/browser/printing/print_**view_manager.cc:115:
>
> UMA_HISTOGRAM_ENUMERATION("**Metro.Print", 0, 2);
> nit: Please use named constants from an enum rather than fixed numbers.
> The two advantages of using enum constants are that (1) they're self
> documenting, and (2) it's harder to get the number of buckets wrong due
> to a typo, especially if you later decide to add additional metrics to
> this histogram.
>
> http://codereview.chromium.**org/10831272/diff/3002/**
>
printing/printing_context_win.**cc<http://codereview.chromium.org/10831272/diff/3002/printing/printing_context_win.cc>
> File printing/printing_context_win.**cc (right):
>
> http://codereview.chromium.**org/10831272/diff/3002/**
>
printing/printing_context_win.**cc#newcode237<http://codereview.chromium.org/10831272/diff/3002/printing/printing_context_win.cc#newcode237>
> printing/printing_context_win.**cc:237:
>
> UMA_HISTOGRAM_ENUMERATION("**Metro.Print", 1, 2);
> nit: I recommend having a single location in the codebase that logs any
> given histogram.  Typically, I've seen people create small utility
> header+implementation files for this purpose.  The advantages are (1)
> it's much less likely that you'll accidentally name the histogram
> differently in different call sites due to a typo; (2) you can't forget
> to update one of the call sites when updating the histogram; and (3) Jim
> tells me we end up with slightly better compiled code if there's only
> one definition for each histogram (this last reason is pretty minor
> though).
>
>
http://codereview.chromium.**org/10831272/<http://codereview.chromium.org/108...
>

[Ilya Sherman, 2012-08-14 18:26:36]

On 2012/08/14 16:08:28, MAD wrote:
> As mentioned in the TODOs, this code will go away once we can send UMA logs
> from the Metro code (which currently can't be done because it's in a
> separate DLL)... Once it's in the Metro Driver code, it will satisfy the
> suggested requirements, which I agree with... For this temporary code, I
> didn't think it was worth finding a common place for this... But we wanted
> to get some preliminary data in the mean time...
> 
> Hope it's OK...

Ok, that should be fine :)