Add UMA statistics to the clipboard

The purpose of this patch is to collect some statistics on how often a user copies something from incognito window, closes it and tries to paste. The statistics will serve to justify or delay the whole feature described in the bug.


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

[vasilii, 2013-02-20 18:14:37]

Hi, please review these changes

Tony: Clipboard
Scott: BrowserContext
Dominic: everything

[sky, 2013-02-20 18:26:02]

Wow, talk about nanny browser. UGH!

browser_context LGTM

https://codereview.chromium.org/12313009/diff/1/content/browser/browser_conte...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/12313009/diff/1/content/browser/browser_conte...
content/browser/browser_context.cc:118: if (buffer ==
ui::Clipboard::BUFFER_STANDARD)
use {} around these.

https://codereview.chromium.org/12313009/diff/1/content/browser/browser_conte...
content/browser/browser_context.cc:120: // collect statistic later.
statistic -> statistics

[tony, 2013-02-20 19:25:06]

LGTM, thanks!

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.cc
File ui/base/clipboard/clipboard.cc (right):

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.c...
ui/base/clipboard/clipboard.cc:193: tag == SourceTag() ?
WRITE_CLIPBOARD_NO_SOURCE_TAG :
Did you mean to wrap this line?  I normally line up the : below the ?, but
indenting is OK too.

[battre, 2013-02-20 23:01:02]

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.cc
File ui/base/clipboard/clipboard.cc (right):

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.c...
ui/base/clipboard/clipboard.cc:313: switch (action){
nit: space before {

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.c...
ui/base/clipboard/clipboard.cc:327:
UMA_HISTOGRAM_ENUMERATION("Clipboard.Actions",
The UMA name indicates that it records the number of times something is done.
This is not the case with the current implementation. You don't report the
number of times, the clipboard is read here.

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.h
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.h...
ui/base/clipboard/clipboard.h:176: // kInvalidSourceTag is not NULL but a
special value != any pointer
Nit: End comments with .

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.h...
ui/base/clipboard/clipboard.h:361: enum TrackedActions {
I would rename this to TrackedAction.

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.h...
ui/base/clipboard/clipboard.h:366: READ_RTF,
Is there a reason to distinguish between these 3 read variations?

[vasilii, 2013-02-21 10:36:00]

Tony, I need your help after landing this patch. I don't have a commiter status.
Please update histograms.xml for me to finish the UMA stuff.

https://codereview.chromium.org/12313009/diff/1/content/browser/browser_conte...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/12313009/diff/1/content/browser/browser_conte...
content/browser/browser_context.cc:118: if (buffer ==
ui::Clipboard::BUFFER_STANDARD)
On 2013/02/20 18:26:02, sky wrote:
> use {} around these.

Done.

https://codereview.chromium.org/12313009/diff/1/content/browser/browser_conte...
content/browser/browser_context.cc:120: // collect statistic later.
On 2013/02/20 18:26:02, sky wrote:
> statistic -> statistics

Done.

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.cc
File ui/base/clipboard/clipboard.cc (right):

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.c...
ui/base/clipboard/clipboard.cc:193: tag == SourceTag() ?
WRITE_CLIPBOARD_NO_SOURCE_TAG :
On 2013/02/20 19:25:07, tony wrote:
> Did you mean to wrap this line?  I normally line up the : below the ?, but
> indenting is OK too.

Done.

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.c...
ui/base/clipboard/clipboard.cc:313: switch (action){
On 2013/02/20 23:01:03, battre wrote:
> nit: space before {

Done.

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.c...
ui/base/clipboard/clipboard.cc:327:
UMA_HISTOGRAM_ENUMERATION("Clipboard.Actions",
On 2013/02/20 23:01:03, battre wrote:
> The UMA name indicates that it records the number of times something is done.
> This is not the case with the current implementation. You don't report the
> number of times, the clipboard is read here.
Changed the name.

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.h
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.h...
ui/base/clipboard/clipboard.h:176: // kInvalidSourceTag is not NULL but a
special value != any pointer
On 2013/02/20 23:01:03, battre wrote:
> Nit: End comments with .

Done.

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.h...
ui/base/clipboard/clipboard.h:361: enum TrackedActions {
On 2013/02/20 23:01:03, battre wrote:
> I would rename this to TrackedAction.

Done.

https://codereview.chromium.org/12313009/diff/1/ui/base/clipboard/clipboard.h...
ui/base/clipboard/clipboard.h:366: READ_RTF,
On 2013/02/20 23:01:03, battre wrote:
> Is there a reason to distinguish between these 3 read variations?
I think that more information is better.

[battre, 2013-02-21 15:54:22]

https://codereview.chromium.org/12313009/diff/12/ui/base/clipboard/clipboard.cc
File ui/base/clipboard/clipboard.cc (right):

https://codereview.chromium.org/12313009/diff/12/ui/base/clipboard/clipboard....
ui/base/clipboard/clipboard.cc:317: MAX_TRACKED_ACTION);
Should we only handle WRITE_CLIPBOARD_SOURCE_TAG here and ignore
WRITE_CLIPBOARD_NO_SOURCE_TAG actions?

[vasilii, 2013-02-21 18:04:03]

https://codereview.chromium.org/12313009/diff/12/ui/base/clipboard/clipboard.cc
File ui/base/clipboard/clipboard.cc (right):

https://codereview.chromium.org/12313009/diff/12/ui/base/clipboard/clipboard....
ui/base/clipboard/clipboard.cc:317: MAX_TRACKED_ACTION);
On 2013/02/21 15:54:22, battre wrote:
> Should we only handle WRITE_CLIPBOARD_SOURCE_TAG here and ignore
> WRITE_CLIPBOARD_NO_SOURCE_TAG actions?
No. This gives us an idea how often users copy something from Incognito window
in comparison with a usual one.

[tony, 2013-02-21 18:52:58]

On 2013/02/21 10:36:00, vasilii wrote:
> Tony, I need your help after landing this patch. I don't have a commiter
status.
> Please update histograms.xml for me to finish the UMA stuff.

Sure, I can update histograms.xml after this lands.  If you make the diff, it'll
save me some time :)

[battre, 2013-02-25 18:27:43]

I still think that we don't need to distinguish between the 3 read types.
Besides this: LGTM

[commit-bot: I haz the power, 2013-02-26 10:52:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/12313009/10001

[commit-bot: I haz the power, 2013-02-26 12:06:27]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-26 12:11:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/12313009/10001

[commit-bot: I haz the power, 2013-02-26 13:27:35]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-26 15:07:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/12313009/10001

[commit-bot: I haz the power, 2013-02-26 16:23:13]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-27 08:56:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/12313009/10001

[commit-bot: I haz the power, 2013-02-27 11:06:32]

Change committed as 184931