Move view change analytics tracking into DirectoryModel where implementation details can be hidden.

Move view change analytics tracking into DirectoryModel where implementation details can be hidden.

Include certain qualifications in the view names reported to analytics, like 'with-media-dir', or the FSP provider extension id.

Background:

Note there was a potential race condition relating to sendAppView and other events. In order to qualify events by view, we needed to be certain to send appView before any other activity that might be kicked off by a directory change event (like importing).

BUG=None
TEST=manual

Committed: https://crrev.com/c0fdb3e0566d3e81134c56e618b2577a8a15b549
Cr-Commit-Position: refs/heads/master@{#322197}

[Steve McKay, 2015-03-23 19:48:19]

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

[Steve McKay, 2015-03-23 20:13:44]

Use slashes to separate view names from 'extra info'

[Steve McKay, 2015-03-23 20:15:51]

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

https://codereview.chromium.org/1022423005/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/common/js/importer_common.js:407:
importer.listEntries_ = function(directory, callback) {
Note, I had previously defined this in fileOperatoinUtil, but as a background
class that file isn't available to foreground code. So I inlined this method
here. There are no remaining uses of fileOperaitonUtil.listEntries, but I left
it as it seems useful. I can remove if that's desired. Just lemme know.

[mtomasz, 2015-03-24 01:34:58]

We can put the utility in ui/file_manager/file_manager/common/js/util.js, so we
don't have dependency on importer code in DirectoryModel.

Please fix the CL description format.

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/common/js/importer_common.js (right):

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:204:
importer.hasMediaDirectory = function(directory) {
nit: This may be slow. Are we fine with it? Instead we could just go the other
way and try to resolve possible roots, as there are only two.

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:420:
entries.forEach(callback);
nit: I think this should cause a js compile error, as forEach requires a
callback with 3 arguments. Could you double check?

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/directory_model.js (right):

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/directory_model.js:986: // But we can
do stuff like
nit: I'm OK not to fill to 80 characters, but this is really hard to read. Can
we at least try to fill least half of each line of comments?

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/directory_model.js:994: return
importer.hasMediaDirectory(volumeInfo.fileSystem.root)
I'm worried about performance here. Switching volumes is already heavy. Could
you confirm that this is fast on Lucas with a lot of files/dirs on root?

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/directory_model.js:996: /** @param
{boolean} hasMedia */
nit: Do we need @return here?

[mtomasz, 2015-03-24 01:35:26]

lgtm with nits if performance concerns are addressed/resolved.

[Steve McKay, 2015-03-24 20:04:53]

Respond to review comments.

[Steve McKay, 2015-03-24 20:05:49]

On 2015/03/24 20:04:53, Steve McKay wrote:
> Respond to review comments.

I don't know what is wrong with the change description formatting. If formatting
is really that important I think the tool needs to be fixed.

[Steve McKay, 2015-03-24 22:39:20]

Oh, and my responses (dones).

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/common/js/importer_common.js (right):

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:204:
importer.hasMediaDirectory = function(directory) {
On 2015/03/24 01:34:58, mtomasz wrote:
> nit: This may be slow. Are we fine with it? Instead we could just go the other
> way and try to resolve possible roots, as there are only two.

I agree about this possibly being slow. Fixed.

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:420:
entries.forEach(callback);
On 2015/03/24 01:34:58, mtomasz wrote:
> nit: I think this should cause a js compile error, as forEach requires a
> callback with 3 arguments. Could you double check?

Callback is required, all others are optional.

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/directory_model.js (right):

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/directory_model.js:986: // But we can
do stuff like
On 2015/03/24 01:34:58, mtomasz wrote:
> nit: I'm OK not to fill to 80 characters, but this is really hard to read. Can
> we at least try to fill least half of each line of comments?

Done.

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/directory_model.js:994: return
importer.hasMediaDirectory(volumeInfo.fileSystem.root)
On 2015/03/24 01:34:58, mtomasz wrote:
> I'm worried about performance here. Switching volumes is already heavy. Could
> you confirm that this is fast on Lucas with a lot of files/dirs on root?

That's a valid concern. Fixed the hasMediaDirectory to check for specific dirs.

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/directory_model.js:996: /** @param
{boolean} hasMedia */
On 2015/03/24 01:34:58, mtomasz wrote:
> nit: Do we need @return here?

Done.

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/directory_model.js:1003: //
TODO(smckay): Can we supply a human readable name?
Tomasz, this is a question for you.

[Steve McKay, 2015-03-24 23:49:22]

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/directory_model.js (right):

https://codereview.chromium.org/1022423005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/directory_model.js:1003: //
TODO(smckay): Can we supply a human readable name?
On 2015/03/24 22:39:20, Steve McKay wrote:
> Tomasz, this is a question for you.

Actually. I saw your response. Ignore this.

[mtomasz, 2015-03-25 00:33:46]

On 2015/03/24 20:05:49, Steve McKay wrote:
> On 2015/03/24 20:04:53, Steve McKay wrote:
> > Respond to review comments.
> 
> I don't know what is wrong with the change description formatting. If
formatting
> is really that important I think the tool needs to be fixed.

Description field contains what will be shown in git log.
Title\n\nDescription\n\nBUG=None

I just fixed it, you can commit.
Unfortunately we don't have a tool for it.

[mtomasz, 2015-03-25 00:40:13]

lgtm with minor nits

https://codereview.chromium.org/1022423005/diff/40001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/common/js/importer_common.js (right):

https://codereview.chromium.org/1022423005/diff/40001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:209: if
(results.some(function(hasDirectory) {return hasDirectory})) {
nit: return hasDirectory -> \sreturn hasDirectory;\s

https://codereview.chromium.org/1022423005/diff/40001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:221: * @param 
{[type]} name      [description]
nit: Remove or write [description]

https://codereview.chromium.org/1022423005/diff/40001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:221: * @param 
{[type]} name      [description]
nit: \s\s -> \s

https://codereview.chromium.org/1022423005/diff/40001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:222: * @return
{[type]}           [description]
nit: No need to align, as we never do that.

https://codereview.chromium.org/1022423005/diff/40001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:225: //
console.log('CHECKING parent ' + parent.name + ' FOR DIRECTORY > ' + name);
nit: Please remove.

https://codereview.chromium.org/1022423005/diff/40001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:238: * @param 
{string} name
nit: \s\s -> \s

[Steve McKay, 2015-03-25 16:48:20]

Dones.

https://codereview.chromium.org/1022423005/diff/40001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/common/js/importer_common.js (right):

https://codereview.chromium.org/1022423005/diff/40001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:209: if
(results.some(function(hasDirectory) {return hasDirectory})) {
On 2015/03/25 00:40:13, mtomasz wrote:
> nit: return hasDirectory -> \sreturn hasDirectory;\s

Extracted it to a local funciton.

https://codereview.chromium.org/1022423005/diff/40001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:221: * @param 
{[type]} name      [description]
On 2015/03/25 00:40:13, mtomasz wrote:
> nit: Remove or write [description]

Done.

https://codereview.chromium.org/1022423005/diff/40001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:221: * @param 
{[type]} name      [description]
On 2015/03/25 00:40:13, mtomasz wrote:
> nit: \s\s -> \s

Done.

https://codereview.chromium.org/1022423005/diff/40001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:222: * @return
{[type]}           [description]
On 2015/03/25 00:40:13, mtomasz wrote:
> nit: No need to align, as we never do that.

This was just auto generated jsdoc I forgot to fix up. Fixed up!

https://codereview.chromium.org/1022423005/diff/40001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:225: //
console.log('CHECKING parent ' + parent.name + ' FOR DIRECTORY > ' + name);
On 2015/03/25 00:40:13, mtomasz wrote:
> nit: Please remove.

Done.

https://codereview.chromium.org/1022423005/diff/40001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:238: * @param 
{string} name
On 2015/03/25 00:40:13, mtomasz wrote:
> nit: \s\s -> \s

Done.

[Steve McKay, 2015-03-25 16:53:58]

Respond to review comments.

[Steve McKay, 2015-03-25 16:55:23]

The CQ bit was checked by smckay@chromium.org

[Steve McKay, 2015-03-25 16:55:23]

The patchset sent to the CQ was uploaded after l-g-t-m from mtomasz@chromium.org
Link to the patchset: https://codereview.chromium.org/1022423005/#ps60001
(title: "Respond to review comments.")

[commit-bot: I haz the power, 2015-03-25 16:55:48]

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

[Steve McKay, 2015-03-25 17:01:36]

The CQ bit was unchecked by smckay@chromium.org

[Steve McKay, 2015-03-25 17:54:35]

Add FSP extension whitelist so we can report names to analytics rather than
extension ids for provided files systems

[Steve McKay, 2015-03-25 17:54:43]

The CQ bit was checked by smckay@chromium.org

[Steve McKay, 2015-03-25 17:54:44]

The patchset sent to the CQ was uploaded after l-g-t-m from mtomasz@chromium.org
Link to the patchset: https://codereview.chromium.org/1022423005/#ps80001
(title: "Add FSP extension whitelist so we can report names to analytics rather
than extension ids for provi…")

[commit-bot: I haz the power, 2015-03-25 17:55:04]

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

[Steve McKay, 2015-03-25 17:56:06]

I put this on submitque with the addition of whitelist for provided files system
names. Please treat the new code as TBR. I'll be happy to make a followup change
if desired.

[commit-bot: I haz the power, 2015-03-25 18:39:51]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-25 18:41:17]

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