Blob/File constructors/slice method shouldn't throw on invalid types

Blob/File constructors/slice method shouldn't throw on invalid types

Per spec[1] and tests[2] the constructors for Blob and File and
their slice() methods should not throw if non-printable-ASCII type
is supplied; instead it should use "" as the type. In addition, the
type should be consistently lower-cased, by both the constructors
and slice() method.

[1] https://w3c.github.io/FileAPI/
[2] https://github.com/w3c/web-platform-tests/tree/master/FileAPI

BUG=509786
R=kinuko@chromium.org

Review-Url: https://codereview.chromium.org/1362963003
Cr-Commit-Position: refs/heads/master@{#455965}
Committed: https://chromium.googlesource.com/chromium/src/+/0b46f3a24bf36d7cf91ec2d88132d15563b85d0e

[jsbell, 2015-09-23 17:33:52]

migrated from https://codereview.chromium.org/1235213004/

[kinuko, 2015-09-24 05:52:44]

lgtm

[jsbell, 2015-09-24 16:51:24]

FYI, holding on this since I discovered that the spec allows invalid types but
then returns the empty string when accessed. We probably want to do both at the
same time but I haven't had cycles.

[jsbell, 2015-10-05 21:29:48]

jsbell@chromium.org changed reviewers:
+ mkwst@chromium.org

[jsbell, 2015-10-05 21:29:48]

+mkwst

Ostensibly this is correct - added a new MIMEUtils file/namespace to implement
the MIMESNIFF algorithm.

But:

* This is somewhat redundant with Chrome's net/base/mime_util - would be nice to
unify with that.
* No-one else actually implements the "parsable MIME type" restriction

So maybe I should back down to PS1.

[Mike West, 2015-10-06 07:14:17]

The changes to the layout tests look pretty reasonable to me. I don't think I
understand the concern you're raising with regard to their "ostensible"
correctness. What do other browsers do? If not this, do they plan to change?

[kinuko, 2015-10-06 07:53:03]

On 2015/10/06 07:14:17, Mike West (slow) wrote:
> The changes to the layout tests look pretty reasonable to me. I don't think I
> understand the concern you're raising with regard to their "ostensible"
> correctness. What do other browsers do? If not this, do they plan to change?

Having dup'ed logic with net/base/mime_util here feels a bit unfortunate, I
guess that's part of what he meant to say?

[jsbell, 2015-10-06 16:08:59]

Don't worry about reviewing this at this time - I'm still not sure where I want
to take this CL. Mostly talking to myself.

On 2015/10/06 07:14:17, Mike West (slow) wrote:
> What do other browsers do? If not this, do they plan to change?

FF converts non-ASCII to blank on Blob create (i.e. same as PS1), but does not
convert non-parsable to blank when .type is accessed. (i.e. PS4)

On 2015/10/06 07:53:03, kinuko wrote:
> Having dup'ed logic with net/base/mime_util here feels a bit unfortunate, I
> guess that's part of what he meant to say?

Yes. I'm waiting to see what esprehn@ recommends for Blink->Chromium
dependencies. 

Also, if other browsers don't match the spec it's unclear we should change.

[jsbell, 2016-10-11 16:47:10]

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

[commit-bot: I haz the power, 2016-10-11 16:47:22]

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, 2016-10-11 20:45:53]

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

[commit-bot: I haz the power, 2016-10-11 20:45:54]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jsbell, 2017-01-04 19:49:37]

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

[commit-bot: I haz the power, 2017-01-04 19:49:58]

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

[jsbell, 2017-01-04 20:26:46]

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

[commit-bot: I haz the power, 2017-01-04 20:27:02]

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

[jsbell, 2017-01-04 20:51:07]

jsbell@chromium.org changed reviewers:
- mkwst@chromium.org

[jsbell, 2017-01-04 20:51:07]

Okay, latest iteration rips out the "parsable mime type" checks. It just
enforces U+0020...U+007E (yielding empty-string if anything is not in that
range) and lowercases the type otherwise. This is what an upcoming version of
Firefox is doing, and what web-platform-tests expects.

kinuko@ - can you re-review?

[jsbell, 2017-01-04 20:51:58]

Description was changed from

==========
Blob/File constructors/slice method shouldn't throw on invalid types

Per spec[1] and tests[2] the constructors for Blob and File and their
slice() methods should not throw if non-ASCII type is supplied;
instead it should use "" as the type. In addition, the type should be
consistently lower-cased, by both the constructors and slice() method.

[1] https://w3c.github.io/FileAPI/
[2] https://github.com/w3c/web-platform-tests/tree/master/FileAPI

BUG=509786
R=kinuko@chromium.org
==========

to

==========
Blob/File constructors/slice method shouldn't throw on invalid types

Per spec[1] and tests[2] the constructors for Blob and File and
their slice() methods should not throw if non-printable-ASCII type
is supplied; instead it should use "" as the type. In addition, the
type should be consistently lower-cased, by both the constructors
and slice() method.

[1] https://w3c.github.io/FileAPI/
[2] https://github.com/w3c/web-platform-tests/tree/master/FileAPI

BUG=509786
R=kinuko@chromium.org
==========

[commit-bot: I haz the power, 2017-01-04 23:48:05]

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

[commit-bot: I haz the power, 2017-01-04 23:48:06]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jsbell, 2017-03-08 00:08:11]

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

[commit-bot: I haz the power, 2017-03-08 00:09:08]

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-08 04:11:43]

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

[commit-bot: I haz the power, 2017-03-08 04:11:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kinuko, 2017-03-08 06:13:39]

On 2017/01/04 20:51:07, jsbell wrote:
> Okay, latest iteration rips out the "parsable mime type" checks. It just
> enforces U+0020...U+007E (yielding empty-string if anything is not in that
> range) and lowercases the type otherwise. This is what an upcoming version of
> Firefox is doing, and what web-platform-tests expects.
> 
> kinuko@ - can you re-review?

Sorry I just realized you were asking me.  lgtm mod some commenting.

(Btw we now can rely on net/mime_util in platform/, but I assume you figured
that it's not necessary to do so in the newer patch)

https://codereview.chromium.org/1362963003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fileapi/Blob.h (right):

https://codereview.chromium.org/1362963003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fileapi/Blob.h:135: // |type| ASCII-lowercased
otherwise.
Could we make this comment a little clearer why we do this? (E.g. to make the
behavior match with what other browsers do)

[jsbell, 2017-03-10 01:04:35]

On 2017/03/08 06:13:39, kinuko wrote:
> Sorry I just realized you were asking me.  lgtm mod some commenting.

Thanks!
 
> (Btw we now can rely on net/mime_util in platform/, but I assume you figured
> that it's not necessary to do so in the newer patch)

Correct. I'll pass the issue off to mek@ as editor to figure out if we want to
keep the "parsable mime type" handling, given that no-one implements it.

https://codereview.chromium.org/1362963003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fileapi/Blob.h (right):

https://codereview.chromium.org/1362963003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fileapi/Blob.h:135: // |type| ASCII-lowercased
otherwise.
On 2017/03/08 06:13:39, kinuko wrote:
> Could we make this comment a little clearer why we do this? (E.g. to make the
> behavior match with what other browsers do)

Done. Well, I referenced the spec, since this is used by the constructor
implementations to conform to the spec.

[jsbell, 2017-03-10 01:04:42]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2017-03-10 01:04:43]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1362963003/#ps220001
(title: "Updated win expectation")

[commit-bot: I haz the power, 2017-03-10 01:05:12]

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 02:48:11]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1489107882559270,
"parent_rev": "351e7eca8d363d99a4847df4e750daa07a428440", "commit_rev":
"d813d801b722e3b5592abf13e8021278281935bb"}

[commit-bot: I haz the power, 2017-03-10 02:53:05]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1489107882559270,
"parent_rev": "91ed4483ca7d2ccaef43aaa54996ebffbbccfb53", "commit_rev":
"0b46f3a24bf36d7cf91ec2d88132d15563b85d0e"}

[commit-bot: I haz the power, 2017-03-10 02:53:59]

Description was changed from

==========
Blob/File constructors/slice method shouldn't throw on invalid types

Per spec[1] and tests[2] the constructors for Blob and File and
their slice() methods should not throw if non-printable-ASCII type
is supplied; instead it should use "" as the type. In addition, the
type should be consistently lower-cased, by both the constructors
and slice() method.

[1] https://w3c.github.io/FileAPI/
[2] https://github.com/w3c/web-platform-tests/tree/master/FileAPI

BUG=509786
R=kinuko@chromium.org
==========

to

==========
Blob/File constructors/slice method shouldn't throw on invalid types

Per spec[1] and tests[2] the constructors for Blob and File and
their slice() methods should not throw if non-printable-ASCII type
is supplied; instead it should use "" as the type. In addition, the
type should be consistently lower-cased, by both the constructors
and slice() method.

[1] https://w3c.github.io/FileAPI/
[2] https://github.com/w3c/web-platform-tests/tree/master/FileAPI

BUG=509786
R=kinuko@chromium.org

Review-Url: https://codereview.chromium.org/1362963003
Cr-Commit-Position: refs/heads/master@{#455965}
Committed:
https://chromium.googlesource.com/chromium/src/+/0b46f3a24bf36d7cf91ec2d88132...
==========

[commit-bot: I haz the power, 2017-03-10 02:54:01]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/0b46f3a24bf36d7cf91ec2d88132...