[Files.app] Record metrics in the suggest apps dialog

[Files.app] Record metrics in the suggest apps dialog

To record metrics, this patch introduces some additional state. This should not change anything functionally.

BUG=307342
TEST=manually tested
R=isherman@chromium.org, mtomasz@chromium.org

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

[yoshiki, 2013-10-15 06:52:44]

@mtomasz: PTAL. Thanks.

[mtomasz, 2013-10-15 07:30:29]

lgtm with nits.

https://codereview.chromium.org/27301002/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/suggest_apps_dialog.js (right):

https://codereview.chromium.org/27301002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/suggest_apps_dialog.js:351:
metrics.recordEnum('SuggestApps.Install', 0, 3);
metrics.recordEnum with enums is easier to read than raw numbers, but I think it
is OK with numbers if you add comments to every invocation.

https://codereview.chromium.org/27301002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/suggest_apps_dialog.js:381:
metrics.recordEnum('SuggestApps.Load', 1, 3);  // Load cancelled.
(like here is good)

[mtomasz, 2013-10-15 07:30:50]

On 2013/10/15 07:30:29, mtomasz wrote:
> lgtm with nits.
> 
>
https://codereview.chromium.org/27301002/diff/1/chrome/browser/resources/file...
> File chrome/browser/resources/file_manager/js/suggest_apps_dialog.js (right):
> 
>
https://codereview.chromium.org/27301002/diff/1/chrome/browser/resources/file...
> chrome/browser/resources/file_manager/js/suggest_apps_dialog.js:351:
> metrics.recordEnum('SuggestApps.Install', 0, 3);
> metrics.recordEnum with enums is easier to read than raw numbers, but I think
it
> is OK with numbers if you add comments to every invocation.
> 
>
https://codereview.chromium.org/27301002/diff/1/chrome/browser/resources/file...
> chrome/browser/resources/file_manager/js/suggest_apps_dialog.js:381:
> metrics.recordEnum('SuggestApps.Load', 1, 3);  // Load cancelled.
> (like here is good)

one more: testted -> tested in the cl description. thanks.

[yoshiki, 2013-10-15 07:47:02]

@jar: PTAL at histograms.xml? Thanks.

https://codereview.chromium.org/27301002/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/suggest_apps_dialog.js (right):

https://codereview.chromium.org/27301002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/suggest_apps_dialog.js:351:
metrics.recordEnum('SuggestApps.Install', 0, 3);
On 2013/10/15 07:30:30, mtomasz wrote:
> metrics.recordEnum with enums is easier to read than raw numbers, but I think
it
> is OK with numbers if you add comments to every invocation.

Done.

[mtomasz, 2013-10-15 07:51:11]

lgtm with nits

https://codereview.chromium.org/27301002/diff/7001/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/suggest_apps_dialog.js (right):

https://codereview.chromium.org/27301002/diff/7001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/suggest_apps_dialog.js:359:
metrics.recordEnum('SuggestApps.Install', 2, 3);  // 2: Intall failed.
nit: Intall -> Install

https://codereview.chromium.org/27301002/diff/7001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/suggest_apps_dialog.js:442: // Record
the reson of closing. 0: Unknown error.
nit: reson -> reason
here and in other places

[yoshiki, 2013-10-17 07:15:09]

@jar: PTAL. Thanks.

https://codereview.chromium.org/27301002/diff/7001/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/suggest_apps_dialog.js (right):

https://codereview.chromium.org/27301002/diff/7001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/suggest_apps_dialog.js:359:
metrics.recordEnum('SuggestApps.Install', 2, 3);  // 2: Intall failed.
On 2013/10/15 07:51:11, mtomasz wrote:
> nit: Intall -> Install

Done.

https://codereview.chromium.org/27301002/diff/7001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/suggest_apps_dialog.js:442: // Record
the reson of closing. 0: Unknown error.
On 2013/10/15 07:51:11, mtomasz wrote:
> nit: reson -> reason
> here and in other places

Done.

[mtomasz, 2013-10-17 07:16:06]

lgtm

[yoshiki, 2013-10-17 16:37:03]

@isherman: Could you take a look in case of jar's absence? Thanks.

[Ilya Sherman, 2013-10-17 22:13:10]

https://codereview.chromium.org/27301002/diff/13001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/suggest_apps_dialog.js (right):

https://codereview.chromium.org/27301002/diff/13001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/suggest_apps_dialog.js:204:
metrics.recordUserAction('SuggestApps.ShowDialog');
Please run extract_actions.py to add this to chromeactions.txt.  You'll need to
either tweak the Python code a bit or temporarily patch in the changes to
about_os_credits.html from https://chromiumcodereview.appspot.com/27418004/ to
make the script work -- I'm working on landing that fix so that it just works.

https://codereview.chromium.org/27301002/diff/13001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/suggest_apps_dialog.js:295:
metrics.recordEnum('SuggestApps.Load', 2, 3);  // 2: Load failure.
I strongly recommend creating a wrapper function for logging each histogram, so
that there's less chance of typos sneaking into the histogram name in just one
of the callsites.

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

https://codereview.chromium.org/27301002/diff/13001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:4824: +<histogram
name="FileBrowser.SuggestApps.ShowDialog">
It doesn't look like the JS code records this as a histogram; rather it seems to
be recorded as a user action.

[yoshiki, 2013-10-28 17:14:18]

@isherman: Sorry for late response. I addressed your comments. PTAL again?
Thanks.

https://codereview.chromium.org/27301002/diff/13001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/suggest_apps_dialog.js (right):

https://codereview.chromium.org/27301002/diff/13001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/suggest_apps_dialog.js:204:
metrics.recordUserAction('SuggestApps.ShowDialog');
On 2013/10/17 22:13:11, Ilya Sherman wrote:
> Please run extract_actions.py to add this to chromeactions.txt.  You'll need
to
> either tweak the Python code a bit or temporarily patch in the changes to
> about_os_credits.html from https://chromiumcodereview.appspot.com/27418004/ to
> make the script work -- I'm working on landing that fix so that it just works.

Thanks for guidance. I didn't know extract_actions.py. Done.

https://codereview.chromium.org/27301002/diff/13001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/suggest_apps_dialog.js:295:
metrics.recordEnum('SuggestApps.Load', 2, 3);  // 2: Load failure.
On 2013/10/17 22:13:11, Ilya Sherman wrote:
> I strongly recommend creating a wrapper function for logging each histogram,
so
> that there's less chance of typos sneaking into the histogram name in just one
> of the callsites.

Done.

[Ilya Sherman, 2013-10-28 23:00:55]

LGTM, thanks.

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

https://codereview.chromium.org/27301002/diff/74001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:4797: +    Chrome OS File Browser: the
reason why the suggest apps dialog is closed.
nit: "is" -> "was"

https://codereview.chromium.org/27301002/diff/74001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:4804: +    Chrome OS File Browser: if
the Webstore item user selected is successfully
nit: "if" -> "whether"; "is" -> "was"

https://codereview.chromium.org/27301002/diff/74001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:4811: +    Chrome OS File Browser: if
the initialization of the dialog is successed or
nit: "if" -> "whether"; "is" -> "was"

https://codereview.chromium.org/27301002/diff/74001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:4811: +    Chrome OS File Browser: if
the initialization of the dialog is successed or
nit: "is successed" -> "succeeded"

https://codereview.chromium.org/27301002/diff/74001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:4819: +    between the moment window
appears and the moment it is all available after
I'm not sure what "it is all available" means.  Could you please try rephrasing
that to be a little clearer?

[yoshiki, 2013-10-30 18:48:52]

Thanks!

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

https://codereview.chromium.org/27301002/diff/74001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:4797: +    Chrome OS File Browser: the
reason why the suggest apps dialog is closed.
On 2013/10/28 23:00:56, Ilya Sherman wrote:
> nit: "is" -> "was"

Done.

https://codereview.chromium.org/27301002/diff/74001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:4804: +    Chrome OS File Browser: if
the Webstore item user selected is successfully
On 2013/10/28 23:00:56, Ilya Sherman wrote:
> nit: "if" -> "whether"; "is" -> "was"

Done.

https://codereview.chromium.org/27301002/diff/74001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:4811: +    Chrome OS File Browser: if
the initialization of the dialog is successed or
On 2013/10/28 23:00:56, Ilya Sherman wrote:
> nit: "is successed" -> "succeeded"

Done.

https://codereview.chromium.org/27301002/diff/74001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:4811: +    Chrome OS File Browser: if
the initialization of the dialog is successed or
On 2013/10/28 23:00:56, Ilya Sherman wrote:
> nit: "if" -> "whether"; "is" -> "was"

Done.

https://codereview.chromium.org/27301002/diff/74001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:4819: +    between the moment window
appears and the moment it is all available after
On 2013/10/28 23:00:56, Ilya Sherman wrote:
> I'm not sure what "it is all available" means.  Could you please try
rephrasing
> that to be a little clearer?

Done.

[yoshiki, 2013-10-31 04:25:31]

Committed patchset #6 manually as r231999 (presubmit successful).