Record whether a file picker specialized for media content can be used

Record whether a file picker specialized for media content can be used

Finnur is working on a new file picker (Issue 656015), and in order to
decide on a shipping plan we'd like to gather some data on whether it
would be appropriate to ship in multiple stages.

R=finnur
BUG=665241
Review-Url: https://codereview.chromium.org/2649033007
Cr-Commit-Position: refs/heads/master@{#446337}
Committed: https://chromium.googlesource.com/chromium/src/+/47b929861e84d93735a00b76a79faf1c40097078

[Peter Beverloo, 2017-01-24 15:49:48]

Patchset #1 (id:1) has been deleted

[Peter Beverloo, 2017-01-24 18:33:59]

Patchset #1 (id:20001) has been deleted

[Peter Beverloo, 2017-01-24 18:34:28]

peter@chromium.org changed reviewers:
+ isherman@chromium.org, miguelg@chromium.org

[Peter Beverloo, 2017-01-24 18:34:28]

+finnur for review
+miguelg for OWNERS
+isherman for UMA

[Peter Beverloo, 2017-01-24 18:34:31]

The CQ bit was checked by peter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 18:35:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Miguel Garcia, 2017-01-24 18:42:26]

https://codereview.chromium.org/2649033007/diff/40001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right):

https://codereview.chromium.org/2649033007/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:375: private int
determineSelectFileDialogScope() {
So this will log as generic when asking for just video files?
(shouldShowImageTypes = false) ?
It will also log as generic when we want images + videos + audio.

[Peter Beverloo, 2017-01-24 19:13:18]

The CQ bit was checked by peter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 19:13:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 20:44:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 20:44:10]

Dry run: This issue passed the CQ dry run.

[Finnur, 2017-01-25 11:26:41]

https://codereview.chromium.org/2649033007/diff/60001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right):

https://codereview.chromium.org/2649033007/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:391: }
Just wondering... Shouldn't we be looking at mFileTypes to be sure nothing extra
slips in? What I mean is something along these lines:

    if (!noSpecificType() && mFileTypes.size() <= 2) {
        boolean showsImages = shouldShowImageTypes();
        boolean showsVideos = shouldShowVideoTypes();

        if (showsImages && showsVideos) {
            return SELECT_FILE_DIALOG_SCOPE_IMAGES_AND_VIDEO;
        } else if (mFileTypes.size() == 1) {
            if (showsImages)
               return SELECT_FILE_DIALOG_SCOPE_IMAGES;
            if (showsVideos)
               return SELECT_FILE_DIALOG_SCOPE_VIDEOS;
        }
    }

    return SELECT_FILE_DIALOG_SCOPE_GENERIC;

[Peter Beverloo, 2017-01-25 15:53:54]

PTAL. With a test because it's not very obvious to get right.

https://codereview.chromium.org/2649033007/diff/60001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right):

https://codereview.chromium.org/2649033007/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:391: }
On 2017/01/25 11:26:41, Finnur wrote:
> Just wondering... Shouldn't we be looking at mFileTypes to be sure nothing
extra
> slips in? What I mean is something along these lines:
> 
>     if (!noSpecificType() && mFileTypes.size() <= 2) {
>         boolean showsImages = shouldShowImageTypes();
>         boolean showsVideos = shouldShowVideoTypes();
> 
>         if (showsImages && showsVideos) {
>             return SELECT_FILE_DIALOG_SCOPE_IMAGES_AND_VIDEO;
>         } else if (mFileTypes.size() == 1) {
>             if (showsImages)
>                return SELECT_FILE_DIALOG_SCOPE_IMAGES;
>             if (showsVideos)
>                return SELECT_FILE_DIALOG_SCOPE_VIDEOS;
>         }
>     }
> 
>     return SELECT_FILE_DIALOG_SCOPE_GENERIC;

Actually, our noSpecificType() check makes sure that there's only a single
entry, so the _IMAGES_AND_VIDEOS case can't even happen.

Rewrote this.

[Peter Beverloo, 2017-01-25 15:54:01]

The CQ bit was checked by peter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 15:54:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Miguel Garcia, 2017-01-25 16:00:00]

lgtm for the java pieces

Thanks for the test!

[commit-bot: I haz the power, 2017-01-25 16:12:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 16:12:42]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Peter Beverloo, 2017-01-25 16:18:21]

The CQ bit was checked by peter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 16:18:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 17:14:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 17:14:31]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-01-25 18:14:06]

https://codereview.chromium.org/2649033007/diff/90001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right):

https://codereview.chromium.org/2649033007/diff/90001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:60: * with their
definition in //tools/metrics/histograms/histograms.xml.
Please also document that this enum should be treated as append-only.

https://codereview.chromium.org/2649033007/diff/90001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:200:
determineSelectFileDialogScope(), SELECT_FILE_DIALOG_SCOPE_MAX);
This final parameter should be "max + 1".  The most common convention is to name
the variable "COUNT" and include the increment within it.

[Peter Beverloo, 2017-01-25 18:21:06]

https://codereview.chromium.org/2649033007/diff/90001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right):

https://codereview.chromium.org/2649033007/diff/90001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:60: * with their
definition in //tools/metrics/histograms/histograms.xml.
On 2017/01/25 18:14:06, Ilya Sherman wrote:
> Please also document that this enum should be treated as append-only.

Done.

https://codereview.chromium.org/2649033007/diff/90001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:200:
determineSelectFileDialogScope(), SELECT_FILE_DIALOG_SCOPE_MAX);
On 2017/01/25 18:14:06, Ilya Sherman wrote:
> This final parameter should be "max + 1".  The most common convention is to
name
> the variable "COUNT" and include the increment within it.

Done.

[Ilya Sherman, 2017-01-25 18:29:19]

Metrics LGTM, thanks

[Peter Beverloo, 2017-01-26 14:37:46]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-01-26 14:37:47]

The patchset sent to the CQ was uploaded after l-g-t-m from miguelg@chromium.org
Link to the patchset: https://codereview.chromium.org/2649033007/#ps110001
(title: "UMA fixes")

[commit-bot: I haz the power, 2017-01-26 14:37:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Finnur, 2017-01-26 14:46:37]

Sorry for the late reply, was preparing for the presentation later on. Love the
test. LGTM. Thanks!

[commit-bot: I haz the power, 2017-01-26 16:09:53]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1485441466731920,
"parent_rev": "9899c468303d9472eeac076b486689753545f827", "commit_rev":
"47b929861e84d93735a00b76a79faf1c40097078"}

[commit-bot: I haz the power, 2017-01-26 16:10:28]

Description was changed from

==========
Record whether a file picker specialized for media content can be used

Finnur is working on a new file picker (Issue 656015), and in order to
decide on a shipping plan we'd like to gather some data on whether it
would be appropriate to ship in multiple stages.

R=finnur
BUG=665241
==========

to

==========
Record whether a file picker specialized for media content can be used

Finnur is working on a new file picker (Issue 656015), and in order to
decide on a shipping plan we'd like to gather some data on whether it
would be appropriate to ship in multiple stages.

R=finnur
BUG=665241

Review-Url: https://codereview.chromium.org/2649033007
Cr-Commit-Position: refs/heads/master@{#446337}
Committed:
https://chromium.googlesource.com/chromium/src/+/47b929861e84d93735a00b76a79f...
==========

[commit-bot: I haz the power, 2017-01-26 16:10:29]

Committed patchset #5 (id:110001) as
https://chromium.googlesource.com/chromium/src/+/47b929861e84d93735a00b76a79f...