Files.app: Add analytics code to instrument cloud import flows.

Files.app: Add analytics code to instrument cloud import flows.

DuplicateFinder now aggregates stats over its lifetime.

MediaImportHandler was changed to create a DuplicateFinder instance per import, so stats relating to duplicate-finding are reported per import session (instead of per-file).

Other changes are just straight-up addition of analytics logging code.

BUG=457736
Committed: https://crrev.com/fbfba350a7a163a73daf982dc90648ab6cddf64b
Cr-Commit-Position: refs/heads/master@{#316913}

[Ben Kwa, 2015-02-11 20:37:16]

kenobi@chromium.org changed reviewers:
+ mtomasz@chromium.org, smckay@chromium.org

[Ben Kwa, 2015-02-11 20:40:15]

cc Sankar for privacy review.

[mtomasz, 2015-02-12 01:51:03]

lgtm with nits

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/background.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/background.js:24: var
analyticsService = analytics.getService('Files.app');
nit: We usually write Files app without a dot, but I know we're not very
consistent with it.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/duplicate_finder.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:30:
importer.DuplicateFinder.Factory.prototype.get;
nit: Does it create or get? Maybe get -> create?

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/media_scanner.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:484:
importer.DefaultScanResult.prototype.getStatistics = function() {
nit: jsdoc missing?

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/mock_media_scanner.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/mock_media_scanner.js:169:
TestScanResult.prototype.getStatistics = function() {
nit: jsdoc missing?

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/test_duplicate_finder.js
(right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/test_duplicate_finder.js:42:
importer.TestDuplicateFinder.Factory.prototype.get = function() {
nit: Can we reset this.testInstance_ to null after returning it, as this method
should never return the same value more than once?

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/common/js/importer_common.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/importer_common.js:749: 
nit: Remove \n.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/common/js/test_tracker.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/test_tracker.js:25:
TestTracker.prototype.sendAppView= function() {};
nit: =\s -> \s=\s, please check also other places.

[Steve McKay, 2015-02-12 17:59:14]

LGTM w/ nits.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/duplicate_finder.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:27:
importer.DuplicateFinder.Factory = function() {};
Don't introduce a factory, just use a function typed to return
importer.DuplicateFinder.

@param {function(): !importer.DuplicateFinder} dupeFinderFactory

It'll be easier to test (no custom class required, just an anon function).

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:181:
importer.DriveDuplicateFinder.Factory.prototype.get = function() {
/**
 * @return {!importer.DriveDuplicateFinder}
 */
importer.DriveDuplicateFinder.crete = function() {
  return new importer.DriveDuplicateFinder();
};


Then where you need a "factory", just pass importer.DriveDuplicateFinder.crete,
easy peasy.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/common/js/importer_common.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/importer_common.js:687:
importer.metrics.Dimension_ = {
Custom dimensions are app Files.app wide, not importer specific.

To that end, we should move both importer.metrics.Dimension_ and
importer.metrics.Dimensions to a new file, with exclusive ownership of Tomasz,
you and me.

I think it would be fine to just rename
s/importer.metrics.Dimension/metrics.Dimension/g for now, and do the new file in
a followup CL.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/importer_common.js:714:
DEDUPE_TYPE_HISTORY: {
I don't think we need any of the custom dimensions from here on down. User type
and Session type, are awesome, though.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/import_controller.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/import_controller.js:308:
util.bytesToString(opt_scan.getStatistics().sizeBytes)),
DOn't do this. It'll create a merge conflict with the work I'm doing. Just add
the getTotalBytes() method back to ScanResult, and have it return the value from
statistics. Then revert this change here.

Feel free to mark getStatistics @deprecated

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/import_controller.js:366: return
availableSpace > scanResult.getStatistics().sizeBytes;
Ditto.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/import_controller.js:670:
util.bytesToString(scan.getStatistics().sizeBytes);
Ditto.

[Steve McKay, 2015-02-12 18:16:35]

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/common/js/importer_common.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/importer_common.js:698:
USER_TYPE_GENERAL: {
We should double check with the GA folks about this. It seems very bummerama
that we'd have to mark traffic NOT included in a Dimension. Maybe I was just
doing something wrong in Pulsar.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/importer_common.js:714:
DEDUPE_TYPE_HISTORY: {
On 2015/02/12 17:59:14, Steve McKay wrote:
> I don't think we need any of the custom dimensions from here on down. User
type
> and Session type, are awesome, though.

Basically, dimensions are best used to create large buckets of traffic, in which
we want to ferret around. For small stuff like, this was a dedupe, I can't see
we'll need to be ferreting around in such a bucket. So I think the best way to
qualify dedupe hits, or import ended hits, is with a simple label on the hit.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/importer_common.js:757:
.category(importer.metrics.CATEGORY);
inline the category here, then just:

importer.metrics.Import_ should be sufficiently qualified.

[Ben Kwa, 2015-02-17 22:55:39]

New patchsets have been uploaded after l-g-t-m from
mtomasz@chromium.org,smckay@chromium.org

[Ben Kwa, 2015-02-17 23:01:30]

Done, PTAL.  Thanks!

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/background.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/background.js:24: var
analyticsService = analytics.getService('Files.app');
On 2015/02/12 01:51:03, mtomasz wrote:
> nit: We usually write Files app without a dot, but I know we're not very
> consistent with it.

Fixed in another CL.  Tracker creation now takes place in metrics.js.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/duplicate_finder.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:27:
importer.DuplicateFinder.Factory = function() {};
On 2015/02/12 17:59:14, Steve McKay wrote:
> Don't introduce a factory, just use a function typed to return
> importer.DuplicateFinder.
> 
> @param {function(): !importer.DuplicateFinder} dupeFinderFactory
> 
> It'll be easier to test (no custom class required, just an anon function).

Discussed offline, and leaving as-is for now.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:30:
importer.DuplicateFinder.Factory.prototype.get;
On 2015/02/12 01:51:03, mtomasz wrote:
> nit: Does it create or get? Maybe get -> create?

Done.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:181:
importer.DriveDuplicateFinder.Factory.prototype.get = function() {
On 2015/02/12 17:59:14, Steve McKay wrote:
> /**
>  * @return {!importer.DriveDuplicateFinder}
>  */
> importer.DriveDuplicateFinder.crete = function() {
>   return new importer.DriveDuplicateFinder();
> };
> 
> 
> Then where you need a "factory", just pass
importer.DriveDuplicateFinder.crete,
> easy peasy.

Acknowledged.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/media_scanner.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:484:
importer.DefaultScanResult.prototype.getStatistics = function() {
On 2015/02/12 01:51:03, mtomasz wrote:
> nit: jsdoc missing?

Done.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/mock_media_scanner.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/mock_media_scanner.js:169:
TestScanResult.prototype.getStatistics = function() {
On 2015/02/12 01:51:03, mtomasz wrote:
> nit: jsdoc missing?

Done.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/test_duplicate_finder.js
(right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/test_duplicate_finder.js:42:
importer.TestDuplicateFinder.Factory.prototype.get = function() {
On 2015/02/12 01:51:03, mtomasz wrote:
> nit: Can we reset this.testInstance_ to null after returning it, as this
method
> should never return the same value more than once?

I changed the factory to instead just do a create each time this is called, and
log the created instances.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/common/js/importer_common.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/importer_common.js:687:
importer.metrics.Dimension_ = {
On 2015/02/12 17:59:14, Steve McKay wrote:
> Custom dimensions are app Files.app wide, not importer specific.
> 
> To that end, we should move both importer.metrics.Dimension_ and
> importer.metrics.Dimensions to a new file, with exclusive ownership of Tomasz,
> you and me.
> 
> I think it would be fine to just rename
> s/importer.metrics.Dimension/metrics.Dimension/g for now, and do the new file
in
> a followup CL.

Done:
- removed the extra dimensions, replacing them with new hit types.
- changed importer.metrics to metrics (consolidating with the existing metrics
namespace).
- moved all this code into a new file (metrics_events.js)

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/importer_common.js:698:
USER_TYPE_GENERAL: {
On 2015/02/12 18:16:35, Steve McKay wrote:
> We should double check with the GA folks about this. It seems very bummerama
> that we'd have to mark traffic NOT included in a Dimension. Maybe I was just
> doing something wrong in Pulsar.

The GA folks indicated that 
1) all traffic we care about has to be tagged with the custom dimension
2) the buckets of a dimension are mutually exclusive.

So, to reflect this, instead of "General" vs "Import", I'll rename them
"Non-import" and "Import".

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/importer_common.js:714:
DEDUPE_TYPE_HISTORY: {
On 2015/02/12 17:59:14, Steve McKay wrote:
> I don't think we need any of the custom dimensions from here on down. User
type
> and Session type, are awesome, though.

Done.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/importer_common.js:714:
DEDUPE_TYPE_HISTORY: {
On 2015/02/12 18:16:35, Steve McKay wrote:
> On 2015/02/12 17:59:14, Steve McKay wrote:
> > I don't think we need any of the custom dimensions from here on down. User
> type
> > and Session type, are awesome, though.
> 
> Basically, dimensions are best used to create large buckets of traffic, in
which
> we want to ferret around. For small stuff like, this was a dedupe, I can't see
> we'll need to be ferreting around in such a bucket. So I think the best way to
> qualify dedupe hits, or import ended hits, is with a simple label on the hit.

I changed them to just be different actions.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/importer_common.js:749: 
On 2015/02/12 01:51:03, mtomasz wrote:
> nit: Remove \n.

Done.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/importer_common.js:757:
.category(importer.metrics.CATEGORY);
On 2015/02/12 18:16:35, Steve McKay wrote:
> inline the category here, then just:
> 
> importer.metrics.Import_ should be sufficiently qualified.

I renamed the BaseEvent to Import_, but I couldn't remove the category as it's
used elsewhere (when sending timings).  I turned the category into an enum too,
so Files app code can add to it in the future.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/common/js/test_tracker.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/test_tracker.js:25:
TestTracker.prototype.sendAppView= function() {};
On 2015/02/12 01:51:03, mtomasz wrote:
> nit: =\s -> \s=\s, please check also other places.

Done.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/import_controller.js (right):

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/import_controller.js:308:
util.bytesToString(opt_scan.getStatistics().sizeBytes)),
On 2015/02/12 17:59:14, Steve McKay wrote:
> DOn't do this. It'll create a merge conflict with the work I'm doing. Just add
> the getTotalBytes() method back to ScanResult, and have it return the value
from
> statistics. Then revert this change here.
> 
> Feel free to mark getStatistics @deprecated

Already merged.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/import_controller.js:366: return
availableSpace > scanResult.getStatistics().sizeBytes;
On 2015/02/12 17:59:14, Steve McKay wrote:
> Ditto.

Acknowledged.

https://codereview.chromium.org/918713003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/import_controller.js:670:
util.bytesToString(scan.getStatistics().sizeBytes);
On 2015/02/12 17:59:14, Steve McKay wrote:
> Ditto.

Acknowledged.

[Steve McKay, 2015-02-17 23:47:51]

We need to add an OWNERS file for metrics_events, but can do that in a followup
CL...post dev branch.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/media_import_handler.js (right):

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:289:
this.tracker_.send(metrics.Events.IMPORT_FILE_COUNT
Report file and byte count only when finished.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/media_scanner.js (right):

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:303:
importer.DefaultMediaScanner.prototype.onDuplicateFileFound_ =
I don't see this being called.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:486: return {
I grab-bag of stats feels very Pulsar, but I suppose it makes for less work when
adding new stats. Okay.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/test_duplicate_finder.js
(right):

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/test_duplicate_finder.js:26: return {
Ahhh, but I like the getStatics symmetry with dupe-finder. That's nice.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/common/js/metrics_events.js (right):

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:9: metrics.Categories =
{
No need for string constants. Just inline the value where used, then build off
of that.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:20: DEDUPE_TYPE: 3,
Remove DEDUPE_TYPE and IMPORT_RESULT

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:53:
metrics.event.Import_ = analytics.EventBuilder.builder()
Make this an enum:


@private @enum {analytics.EventBuilder}
metrics.event.Shared_ = {
  IMPORT: metrics.event.Import_
};

The, as needed you can hoist shared events up here.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:54:
.category(metrics.Categories.IMPORT);
IMPORT > Acquisition, because we may want to track manually imported stuff
differently.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:59: metrics.Events = {
metrics.ImportEvents, then you can remove the IMPORT_ qualification on each
entry.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:61:
.action('ImportStart')
Prefer regular phrases in your strings...

"Import Started"
"Import Ended"

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:66:
.action('ImportSuccess'),
Import Completed

you get the idea

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:75:
.action('importFileCount'),
Files Imported

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:78:
.action('importByteCount'),
Total Bytes Imported

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:84:
.action('historyDedupeCount'),
Num. Files History Deduped
Num. Files Content Deduped

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:95: COMPUTE_HASH:
'ComputeHash',
Any reason we need to qualify this? We're computing it in-place, maybe we should
include that. "Compute Content Hash (in-place)". Or maybe you can apply a label
for "in-place". I don't recall if timings support labels.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/import_controller.js (right):

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/import_controller.js:115:
this.activeImport_.task.requestCancel();
Nice! Thanks for taking care of this.

[Ben Kwa, 2015-02-17 23:54:24]

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/common/js/metrics_events.js (right):

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:9: metrics.Categories =
{
On 2015/02/17 23:47:51, Steve McKay wrote:
> No need for string constants. Just inline the value where used, then build off
> of that.

I'd agree if this value was only used in the metrics namespace, but it's used in
client code (e.g.
this.tracker_.sendTiming(metrics.Categories.IMPORT, ...) as well, so I think the
enum works better.  WDYT?

[Steve McKay, 2015-02-17 23:56:08]

On 2015/02/17 23:54:24, Ben Kwa wrote:
>
https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
> File ui/file_manager/file_manager/common/js/metrics_events.js (right):
> 
>
https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
> ui/file_manager/file_manager/common/js/metrics_events.js:9: metrics.Categories
=
> {
> On 2015/02/17 23:47:51, Steve McKay wrote:
> > No need for string constants. Just inline the value where used, then build
off
> > of that.
> 
> I'd agree if this value was only used in the metrics namespace, but it's used
in
> client code (e.g.
> this.tracker_.sendTiming(metrics.Categories.IMPORT, ...) as well, so I think
the
> enum works better.  WDYT?

Ah. No worries, then.

[Ben Kwa, 2015-02-18 22:23:02]

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/media_import_handler.js (right):

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:289:
this.tracker_.send(metrics.Events.IMPORT_FILE_COUNT
On 2015/02/17 23:47:50, Steve McKay wrote:
> Report file and byte count only when finished.

File counts - done.  Byte counts - as discussed offline, taking these out for
now.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/media_scanner.js (right):

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:303:
importer.DefaultMediaScanner.prototype.onDuplicateFileFound_ =
On 2015/02/17 23:47:50, Steve McKay wrote:
> I don't see this being called.

Thanks.  Fixed.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:486: return {
On 2015/02/17 23:47:50, Steve McKay wrote:
> I grab-bag of stats feels very Pulsar, but I suppose it makes for less work
when
> adding new stats. Okay.

Acknowledged.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/test_duplicate_finder.js
(right):

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/test_duplicate_finder.js:26: return {
On 2015/02/17 23:47:50, Steve McKay wrote:
> Ahhh, but I like the getStatics symmetry with dupe-finder. That's nice.

Acknowledged.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/common/js/metrics_events.js (right):

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:20: DEDUPE_TYPE: 3,
On 2015/02/17 23:47:50, Steve McKay wrote:
> Remove DEDUPE_TYPE and IMPORT_RESULT

Done.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:53:
metrics.event.Import_ = analytics.EventBuilder.builder()
On 2015/02/17 23:47:50, Steve McKay wrote:
> Make this an enum:
> 
> 
> @private @enum {analytics.EventBuilder}
> metrics.event.Shared_ = {
>   IMPORT: metrics.event.Import_
> };
> 
> The, as needed you can hoist shared events up here.

Done.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:54:
.category(metrics.Categories.IMPORT);
On 2015/02/17 23:47:50, Steve McKay wrote:
> IMPORT > Acquisition, because we may want to track manually imported stuff
> differently.

Done.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:59: metrics.Events = {
On 2015/02/17 23:47:50, Steve McKay wrote:
> metrics.ImportEvents, then you can remove the IMPORT_ qualification on each
> entry.

Done.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:61:
.action('ImportStart')
On 2015/02/17 23:47:50, Steve McKay wrote:
> Prefer regular phrases in your strings...
> 
> "Import Started"
> "Import Ended"

Done.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:66:
.action('ImportSuccess'),
On 2015/02/17 23:47:50, Steve McKay wrote:
> Import Completed
> 
> you get the idea

Done.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:75:
.action('importFileCount'),
On 2015/02/17 23:47:50, Steve McKay wrote:
> Files Imported

Done.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:78:
.action('importByteCount'),
On 2015/02/17 23:47:50, Steve McKay wrote:
> Total Bytes Imported

Done.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:84:
.action('historyDedupeCount'),
On 2015/02/17 23:47:50, Steve McKay wrote:
> Num. Files History Deduped
> Num. Files Content Deduped

Done.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/metrics_events.js:95: COMPUTE_HASH:
'ComputeHash',
On 2015/02/17 23:47:50, Steve McKay wrote:
> Any reason we need to qualify this? We're computing it in-place, maybe we
should
> include that. "Compute Content Hash (in-place)". Or maybe you can apply a
label
> for "in-place". I don't recall if timings support labels.

I'll use a label.

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/import_controller.js (right):

https://codereview.chromium.org/918713003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/import_controller.js:115:
this.activeImport_.task.requestCancel();
On 2015/02/17 23:47:51, Steve McKay wrote:
> Nice! Thanks for taking care of this.

Acknowledged.

[Ben Kwa, 2015-02-18 22:27:18]

The CQ bit was checked by kenobi@chromium.org

[commit-bot: I haz the power, 2015-02-18 22:28:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/918713003/40001

[commit-bot: I haz the power, 2015-02-18 23:12:14]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-18 23:12:41]

Patchset 3 (id:??) landed as
https://crrev.com/fbfba350a7a163a73daf982dc90648ab6cddf64b
Cr-Commit-Position: refs/heads/master@{#316913}