Add UMA histogram Arc.AppsInstalledAtStartup to measure the number of ARC apps installed.

Add UMA histogram Arc.AppsInstalledAtStartup to measure the number of ARC apps installed.

BUG=b:27344858
TEST=Tested on minnie
R=tapted@chromium.org,jwd@chromium.org

Committed: https://crrev.com/251ebaaaeb87ba075e854700d0ca33b5e0c5e634
Cr-Commit-Position: refs/heads/master@{#383924}

[Chi-Ngai Wan, 2016-03-21 10:25:22]

Description was changed from

==========
Add UMA histogram to measure the number of ARC apps installed.

BUG=b:27344858
TEST=Tested on minnie
R=jennyz@chromium.org,holte@chromium.org
==========

to

==========
Add UMA histogram Arc.AppsInstalledAtStartup to measure the number of ARC apps
installed.

BUG=b:27344858
TEST=Tested on minnie
R=jennyz@chromium.org,holte@chromium.org
==========

[Chi-Ngai Wan, 2016-03-23 06:51:05]

Description was changed from

==========
Add UMA histogram Arc.AppsInstalledAtStartup to measure the number of ARC apps
installed.

BUG=b:27344858
TEST=Tested on minnie
R=jennyz@chromium.org,holte@chromium.org
==========

to

==========
Add UMA histogram Arc.AppsInstalledAtStartup to measure the number of ARC apps
installed.

BUG=b:27344858
TEST=Tested on minnie
R=tapted@chromium.org,jwd@chromium.org
==========

[Chi-Ngai Wan, 2016-03-23 06:51:05]

cnwan@chromium.org changed reviewers:
+ jwd@chromium.org, tapted@chromium.org
- holte@chromium.org, jennyz@chromium.org

[Chi-Ngai Wan, 2016-03-23 06:52:06]

Hi, I would like to ask you for a code review. Thanks

[tapted, 2016-03-23 11:46:02]

tapted@chromium.org changed reviewers:
+ xiyuan@chromium.org

[tapted, 2016-03-23 11:46:02]

subbing in xiyuan (I haven't really been following the arc changes [should we
add a chrome/browser/ui/app_list/arc/OWNERS?])

One comment would be - maybe there's a way to group this with the other UMA
counts gathered in InstalledLoader::RecordExtensionsMetrics
https://chromium.googlesource.com/chromium/src/+/bccaa6b939b3daed06d54bd99ffb...

but I don't know enough about how arc fits in to the extension system (poking
around seems like maybe it doesn't at all)

[jwd, 2016-03-23 19:23:46]

https://codereview.chromium.org/1818933002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1818933002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:838: +<histogram
name="Arc.AppsInstalledAtStartup">
Can you add a meaningful units here please. Maybe something like App Count.

[Chi-Ngai Wan, 2016-03-24 10:12:44]

Unfortunately ARC apps are loaded asynchronously (due to ARC's long bootstrap
time) and not available by the time ::RecordExtensionsMetrics() is called.


On 2016/03/23 11:46:02, tapted (OOO.travel to Apr 5th) wrote:
> subbing in xiyuan (I haven't really been following the arc changes [should we
> add a chrome/browser/ui/app_list/arc/OWNERS?])
> 
> One comment would be - maybe there's a way to group this with the other UMA
> counts gathered in InstalledLoader::RecordExtensionsMetrics
>
https://chromium.googlesource.com/chromium/src/+/bccaa6b939b3daed06d54bd99ffb...
> 
> but I don't know enough about how arc fits in to the extension system (poking
> around seems like maybe it doesn't at all)

https://codereview.chromium.org/1818933002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1818933002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:838: +<histogram
name="Arc.AppsInstalledAtStartup">
On 2016/03/23 19:23:45, Jesse Doherty wrote:
> Can you add a meaningful units here please. Maybe something like App Count.

Done.

[xiyuan, 2016-03-24 16:00:25]

https://codereview.chromium.org/1818933002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/1818933002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:408:
UMA_HISTOGRAM_COUNTS_1000("Arc.AppsInstalledAtStartup", ready_apps_.size());
Can you clarify when would OnAppListRefreshed be called, after ARC++ is started?
What about after user installs an app from Play?

Since the UMA name contains "Startup", one would expect this would be called
only once after user session is started.

[Chi-Ngai Wan, 2016-03-25 07:01:56]

Thanks for your feedback. PTAL.

https://codereview.chromium.org/1818933002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/1818933002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:408:
UMA_HISTOGRAM_COUNTS_1000("Arc.AppsInstalledAtStartup", ready_apps_.size());
On 2016/03/24 16:00:25, xiyuan wrote:
> Can you clarify when would OnAppListRefreshed be called, after ARC++ is
started?
> What about after user installs an app from Play?
> 
> Since the UMA name contains "Startup", one would expect this would be called
> only once after user session is started.

OnAppListRefreshed() is called at both ARC startup and when locale is changed,
while OnAppAdded() is called when user installs an app from Play. 

Thanks for your comment. I've added an is_initialized_ check to safe-guard this
line.

[xiyuan, 2016-03-25 15:46:19]

lgtm

https://codereview.chromium.org/1818933002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):

https://codereview.chromium.org/1818933002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:174: bool is_initialized_;
nit: Prefer to initialize |is_initialized_| here,

i.e.
  bool is_initialized_ = false;

See https://chromium-cpp.appspot.com/, "Non-Static Class Member Initializers"

[Chi-Ngai Wan, 2016-03-28 06:47:37]

PTAL. Thanks

https://codereview.chromium.org/1818933002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):

https://codereview.chromium.org/1818933002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:174: bool is_initialized_;
On 2016/03/25 15:46:19, xiyuan wrote:
> nit: Prefer to initialize |is_initialized_| here,
> 
> i.e.
>   bool is_initialized_ = false;
> 
> See https://chromium-cpp.appspot.com/, "Non-Static Class Member Initializers"

Done.

[Chi-Ngai Wan, 2016-03-28 06:48:37]

Hi Jesse, I added a unit to my histogram definition. PTAL. Thank you.

[Chi-Ngai Wan, 2016-03-29 12:03:50]

On 2016/03/28 at 06:48:37, Chi-Ngai Wan wrote:
> Hi Jesse, I added a unit to my histogram definition. PTAL. Thank you.

ping

[jwd, 2016-03-29 14:52:46]

lgtm

[Chi-Ngai Wan, 2016-03-30 06:28:49]

The CQ bit was checked by cnwan@chromium.org

[Chi-Ngai Wan, 2016-03-30 06:28:50]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/1818933002/#ps80001
(title: "Rebased to ToT")

[commit-bot: I haz the power, 2016-03-30 06:29:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1818933002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1818933002/80001

[commit-bot: I haz the power, 2016-03-30 07:40:26]

Description was changed from

==========
Add UMA histogram Arc.AppsInstalledAtStartup to measure the number of ARC apps
installed.

BUG=b:27344858
TEST=Tested on minnie
R=tapted@chromium.org,jwd@chromium.org
==========

to

==========
Add UMA histogram Arc.AppsInstalledAtStartup to measure the number of ARC apps
installed.

BUG=b:27344858
TEST=Tested on minnie
R=tapted@chromium.org,jwd@chromium.org
==========

[commit-bot: I haz the power, 2016-03-30 07:40:27]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-30 07:42:39]

Description was changed from

==========
Add UMA histogram Arc.AppsInstalledAtStartup to measure the number of ARC apps
installed.

BUG=b:27344858
TEST=Tested on minnie
R=tapted@chromium.org,jwd@chromium.org
==========

to

==========
Add UMA histogram Arc.AppsInstalledAtStartup to measure the number of ARC apps
installed.

BUG=b:27344858
TEST=Tested on minnie
R=tapted@chromium.org,jwd@chromium.org

Committed: https://crrev.com/251ebaaaeb87ba075e854700d0ca33b5e0c5e634
Cr-Commit-Position: refs/heads/master@{#383924}
==========

[commit-bot: I haz the power, 2016-03-30 07:42:40]

Patchset 5 (id:??) landed as
https://crrev.com/251ebaaaeb87ba075e854700d0ca33b5e0c5e634
Cr-Commit-Position: refs/heads/master@{#383924}