Fix UMA metric to not count multiple image/ tags as generic.

Fix UMA metric to not count multiple image/ tags as generic.

BUG=699536
Review-Url: https://codereview.chromium.org/2737843004
Cr-Commit-Position: refs/heads/master@{#455771}
Committed: https://chromium.googlesource.com/chromium/src/+/384e24962ca4e9f0b9c8b821f5c021db93693d2a

[Finnur, 2017-03-08 13:31:25]

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

[commit-bot: I haz the power, 2017-03-08 13:31:39]

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

[Finnur, 2017-03-08 13:33:08]

finnur@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2017-03-08 14:02:01]

lgtm

https://codereview.chromium.org/2737843004/diff/1/ui/android/java/src/org/chr...
File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right):

https://codereview.chromium.org/2737843004/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:444: private int
acceptSpecificType(String accept) {
nit: maybe name this countAcceptTypesFor? It'd read as follows, which is fairly
clear:

countAcceptTypesFor(IMAGE_TYPE)

[Miguel Garcia, 2017-03-08 14:03:35]

miguelg@chromium.org changed reviewers:
+ miguelg@chromium.org

[Miguel Garcia, 2017-03-08 14:03:35]

lgtm

[commit-bot: I haz the power, 2017-03-08 14:13:42]

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

[commit-bot: I haz the power, 2017-03-08 14:13:43]

Dry run: This issue passed the CQ dry run.

[Finnur, 2017-03-09 16:03:31]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2017-03-09 16:03:32]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
miguelg@chromium.org
Link to the patchset: https://codereview.chromium.org/2737843004/#ps20001
(title: "Address review feedback")

[commit-bot: I haz the power, 2017-03-09 16:03:47]

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-03-09 16:50:38]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489075411924200,
"parent_rev": "c182846fcda260cc3d11dbac775b1b7a666729c8", "commit_rev":
"384e24962ca4e9f0b9c8b821f5c021db93693d2a"}

[commit-bot: I haz the power, 2017-03-09 16:51:38]

Description was changed from

==========
Fix UMA metric to not count multiple image/ tags as generic.

BUG=699536
==========

to

==========
Fix UMA metric to not count multiple image/ tags as generic.

BUG=699536

Review-Url: https://codereview.chromium.org/2737843004
Cr-Commit-Position: refs/heads/master@{#455771}
Committed:
https://chromium.googlesource.com/chromium/src/+/384e24962ca4e9f0b9c8b821f5c0...
==========

[commit-bot: I haz the power, 2017-03-09 16:51:39]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/384e24962ca4e9f0b9c8b821f5c0...

[Finnur, 2017-03-10 16:02:19]

Description was changed from

==========
Fix UMA metric to not count multiple image/ tags as generic.

BUG=699536

Review-Url: https://codereview.chromium.org/2737843004
Cr-Commit-Position: refs/heads/master@{#455771}
Committed:
https://chromium.googlesource.com/chromium/src/+/384e24962ca4e9f0b9c8b821f5c0...
==========

to

==========
Fix UMA metric to not count multiple image/ tags as generic.

BUG=699536

Review-Url: https://codereview.chromium.org/2737843004
Cr-Commit-Position: refs/heads/master@{#455771}
Committed:
https://chromium.googlesource.com/chromium/src/+/384e24962ca4e9f0b9c8b821f5c0...
==========

[Finnur, 2017-03-10 16:02:32]

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

[commit-bot: I haz the power, 2017-03-10 16:02:57]

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

[Finnur, 2017-03-10 16:03:10]

Can you take another look?

[commit-bot: I haz the power, 2017-03-10 16:05:33]

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

[commit-bot: I haz the power, 2017-03-10 16:05:34]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Finnur, 2017-03-10 16:19:50]

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

[commit-bot: I haz the power, 2017-03-10 16:20:22]

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

[Finnur, 2017-03-10 16:24:35]

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

[commit-bot: I haz the power, 2017-03-10 16:31:56]

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-03-10 17:50:29]

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

[commit-bot: I haz the power, 2017-03-10 17:50:30]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-03-12 23:55:12]

nit: please create a new CL for a new commit, we only occasionally reuse CLs for
relands following a revert.

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

https://codereview.chromium.org/2737843004/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:65: ".divx",
".flv", ".mov", ".mp4", ".mpeg", ".mpg", ".swf", ".wmv", ".webm", ".mkv"};
Are you trying to compensate for developers who provide an @accept attribute
whose syntax does not conform to the specification? Do you have an example of a
site that does this?

|mFileType| should contain a list of *mime* types (it's a bit of a misnomer),
not file extensions. For example, the mime type of a JPEG image would be
"image/jpeg".

At a higher level, maintaining our own list of types doesn't seem maintainable.
For example, the image extensions include various types that our own image
decoders aren't able to handle (PDF, TIF(F), XCF).

[Finnur, 2017-03-13 13:22:14]

Just to close the loop:

> Are you trying to compensate for developers who provide an @accept attribute
> whose syntax does not conform to the specification? 

Extensions are not just allowed under the spec, they're encouraged (in addition
to mime types).
https://html.spec.whatwg.org/multipage/forms.html#attr-input-accept

> Do you have an example of a site that does this?

Imgur.

> At a higher level, maintaining our own list of types doesn't seem
maintainable.

I didn't see us maintaining this list at all, it was supposed to go away as soon
as we got an answer to the metrics question.

> |mFileType| should contain a list of *mime* types (it's a bit of a misnomer),
> not file extensions. For example, the mime type of a JPEG image would be
> "image/jpeg".

You raise a good point here. Further testing indicates that if you provide only
extensions then mFileType will be blank, which means we should abandon this
update to the CL. I still don't have a good understanding of why the extensions
are ignored -- but we seem to catch that case with a generic picker.

[Finnur, 2017-03-13 14:24:59]

Sorry, my testing erroneously specified *.gif, not .gif, so I was not following
spec. 

So, the file extensions actually do reach the Java side (all the way to
mFileTypes), so I think this metrics still makes sense.

I'll spin another CL.

[Finnur, 2017-03-13 14:49:14]

> I'll spin another CL.

Done: https://codereview.chromium.org/2746143002/