Make Blob#slice() contentType argument to be non nullable.

Make Blob#slice() contentType argument to be non nullable.

Compatibility:
new Blob().slice(0,0,null).type
Edge: "null" (match with the spec)
Firefox: "null" (match with the spec)
Chrome: ""
Safari: ""

BUG=662005
Committed: https://crrev.com/68dddcc6a3b8aea8c1836ad353e81993ef27d1ee
Cr-Commit-Position: refs/heads/master@{#433890}

[lunalu1, 2016-11-16 20:36:32]

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

[commit-bot: I haz the power, 2016-11-16 20:37:33]

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-11-16 22:15:47]

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

[commit-bot: I haz the power, 2016-11-16 22:15:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lunalu1, 2016-11-17 16:40:42]

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

[commit-bot: I haz the power, 2016-11-17 16:41:00]

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

[lunalu1, 2016-11-17 16:58:28]

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

[lunalu1, 2016-11-17 16:58:53]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-11-17 16:59:05]

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

[lunalu1, 2016-11-17 19:02:08]

lunalu@chromium.org changed reviewers:
+ foolip@chromium.org

[lunalu1, 2016-11-17 19:02:09]

Hi foolip, could you please take a look at this CL?

Thanks

[foolip, 2016-11-17 19:48:16]

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

[foolip, 2016-11-17 19:48:17]

+jsbell@ FYI and to shout if there are existing tests that this should be folded
into.

https://codereview.chromium.org/2509833002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/files/blob-slice-type-test.html
(right):

https://codereview.chromium.org/2509833002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/files/blob-slice-type-test.html:10: },
'Blob.slice() argument "type" should be nullable.');
Oops, it should *not* be nullable. But even better, describe what is tested
without including the pass condition, e.g. "blob.slice() with type argument as
null"

For good measure, can you also add tests for "blob.slice() with no type
argument" and "blob.slice() with type argument as undefined", which should both
behave the same, assert_equals(c.type, "");

[foolip, 2016-11-17 19:48:33]

lgtm % nits

[foolip, 2016-11-17 19:51:27]

Also, can you say in the description which browsers already share this behavior?
I just tested `new Blob().slice(0,0,null).type` in BrowserStack and the result
is that Edge and Firefox say "null", while Chrome and Safari say "".

[jsbell, 2016-11-17 19:52:24]

https://codereview.chromium.org/2509833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fileapi/Blob.idl (right):

https://codereview.chromium.org/2509833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fileapi/Blob.idl:45: // TODO(foolip): contentType
should not be nullable.
remove this comment too!

[lunalu1, 2016-11-17 19:53:53]

Description was changed from

==========
Make Blob#slice() contentType argument to be non nullable.

BUG=662005
==========

to

==========
Make Blob#slice() contentType argument to be non nullable.

Compatibility:
new Blob().slice(0,0,null).type
Edge: "null" (match with spec)
Firefox: "null"
Chrome: ""
Safari: ""

BUG=662005
==========

[lunalu1, 2016-11-17 19:54:10]

Description was changed from

==========
Make Blob#slice() contentType argument to be non nullable.

Compatibility:
new Blob().slice(0,0,null).type
Edge: "null" (match with spec)
Firefox: "null"
Chrome: ""
Safari: ""

BUG=662005
==========

to

==========
Make Blob#slice() contentType argument to be non nullable.

Compatibility:
new Blob().slice(0,0,null).type
Edge: "null" (match with the spec)
Firefox: "null" (match with the spec)
Chrome: ""
Safari: ""

BUG=662005
==========

[jsbell, 2016-11-17 19:59:16]

On 2016/11/17 19:48:17, foolip wrote:
> +jsbell@ FYI and to shout if there are existing tests that this should be
folded
> into.

The test could merge into fast/files/blob-slice-test.html but I think it's fine
on its own. I would drop "-test" from the filename though - it's implicit.

We should also add a test into
https://github.com/w3c/web-platform-tests/blob/master/FileAPI/blob/Blob-slice...
upstream to ensure we converge on behaviors.

[commit-bot: I haz the power, 2016-11-17 20:00:43]

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

[commit-bot: I haz the power, 2016-11-17 20:00:44]

Dry run: This issue passed the CQ dry run.

[lunalu1, 2016-11-17 20:02:28]

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

[lunalu1, 2016-11-17 20:03:01]

Hi jsbell and foolip,

Thanks for the comments. I have made the changes. Please take another look.

https://codereview.chromium.org/2509833002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/files/blob-slice-type-test.html
(right):

https://codereview.chromium.org/2509833002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/files/blob-slice-type-test.html:10: },
'Blob.slice() argument "type" should be nullable.');
On 2016/11/17 19:48:16, foolip wrote:
> Oops, it should *not* be nullable. But even better, describe what is tested
> without including the pass condition, e.g. "blob.slice() with type argument as
> null"
> 
> For good measure, can you also add tests for "blob.slice() with no type
> argument" and "blob.slice() with type argument as undefined", which should
both
> behave the same, assert_equals(c.type, "");

Done.

https://codereview.chromium.org/2509833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fileapi/Blob.idl (right):

https://codereview.chromium.org/2509833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fileapi/Blob.idl:45: // TODO(foolip): contentType
should not be nullable.
On 2016/11/17 19:52:24, jsbell wrote:
> remove this comment too!

My bad.

[commit-bot: I haz the power, 2016-11-17 20:04:50]

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

[lunalu1, 2016-11-17 20:26:59]

as jsbell suggested. I also added 3 tests to the web-platform tests

[foolip, 2016-11-17 20:39:34]

On 2016/11/17 20:26:59, loonybear wrote:
> as jsbell suggested. I also added 3 tests to the web-platform tests

Did you submit it at https://github.com/w3c/web-platform-tests/pulls?

[blink-reviews, 2016-11-17 20:49:24]

​Update Blob-slice.html #4224​



Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Thu, Nov 17, 2016 at 3:39 PM, <foolip@chromium.org> wrote:

> On 2016/11/17 20:26:59, loonybear wrote:
> > as jsbell suggested. I also added 3 tests to the web-platform tests
>
> Did you submit it at https://github.com/w3c/web-platform-tests/pulls?
> <https://github.com/w3c/web-platform-tests/pulls>
>
> https://codereview.chromium.org/2509833002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-11-17 20:49:25]

​Update Blob-slice.html #4224​



Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Thu, Nov 17, 2016 at 3:39 PM, <foolip@chromium.org> wrote:

> On 2016/11/17 20:26:59, loonybear wrote:
> > as jsbell suggested. I also added 3 tests to the web-platform tests
>
> Did you submit it at https://github.com/w3c/web-platform-tests/pulls?
> <https://github.com/w3c/web-platform-tests/pulls>
>
> https://codereview.chromium.org/2509833002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-11-17 22:22:22]

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

[commit-bot: I haz the power, 2016-11-17 22:22:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lunalu1, 2016-11-21 20:22:39]

Hi, thanks for reviewing my CL.

I have made the changes based on the comments. Please take another look. 

Thanks

[foolip, 2016-11-21 20:31:01]

lgtm % nits

https://codereview.chromium.org/2509833002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/files/blob-slice-type.html (right):

https://codereview.chromium.org/2509833002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/files/blob-slice-type.html:14: },
'Blob.slice() with no type argument paased.');
s/paased/passed/

But more importantly, this is actually the same as the first test, should be
assert_equals(new Blob().slice(0, 0).type, "");

[lunalu1, 2016-11-21 21:01:16]

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

[commit-bot: I haz the power, 2016-11-21 21:01:47]

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

[lunalu1, 2016-11-21 21:33:52]

Done. Thanks

https://codereview.chromium.org/2509833002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/files/blob-slice-type.html (right):

https://codereview.chromium.org/2509833002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/files/blob-slice-type.html:14: },
'Blob.slice() with no type argument paased.');
On 2016/11/21 20:31:01, foolip wrote:
> s/paased/passed/
> 
> But more importantly, this is actually the same as the first test, should be
> assert_equals(new Blob().slice(0, 0).type, "");

Done.

[jsbell, 2016-11-21 22:11:45]

lgtm as well

(I'd leave a space between the test functions, but that's really minor)

[jsbell, 2016-11-21 22:12:29]

Also, thank you very much for tackling this, and responding quickly to our
review feedback! We really appreciate your assistance here getting Chromium more
compliant with the standard.

[commit-bot: I haz the power, 2016-11-22 00:30:51]

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

[commit-bot: I haz the power, 2016-11-22 00:30:52]

Dry run: This issue passed the CQ dry run.

[lunalu1, 2016-11-22 15:09:07]

The CQ bit was checked by lunalu@chromium.org

[lunalu1, 2016-11-22 15:09:07]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org,
jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/2509833002/#ps100001
(title: "Reformat layout tests")

[commit-bot: I haz the power, 2016-11-22 15:09:24]

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-11-22 15:12:04]

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

[commit-bot: I haz the power, 2016-11-22 15:12:05]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lunalu1, 2016-11-22 16:24:49]

The CQ bit was checked by lunalu@chromium.org

[commit-bot: I haz the power, 2016-11-22 16:25:05]

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-11-22 17:09:46]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1479831889823380,
"parent_rev": "2b765735677b80c81e393542bf6b28943a5d7a53", "commit_rev":
"3f513fdf330bdc30cebfd87a70b08ff71a4dc376"}

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

Description was changed from

==========
Make Blob#slice() contentType argument to be non nullable.

Compatibility:
new Blob().slice(0,0,null).type
Edge: "null" (match with the spec)
Firefox: "null" (match with the spec)
Chrome: ""
Safari: ""

BUG=662005
==========

to

==========
Make Blob#slice() contentType argument to be non nullable.

Compatibility:
new Blob().slice(0,0,null).type
Edge: "null" (match with the spec)
Firefox: "null" (match with the spec)
Chrome: ""
Safari: ""

BUG=662005
==========

[commit-bot: I haz the power, 2016-11-22 17:10:13]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-11-22 17:15:23]

Description was changed from

==========
Make Blob#slice() contentType argument to be non nullable.

Compatibility:
new Blob().slice(0,0,null).type
Edge: "null" (match with the spec)
Firefox: "null" (match with the spec)
Chrome: ""
Safari: ""

BUG=662005
==========

to

==========
Make Blob#slice() contentType argument to be non nullable.

Compatibility:
new Blob().slice(0,0,null).type
Edge: "null" (match with the spec)
Firefox: "null" (match with the spec)
Chrome: ""
Safari: ""

BUG=662005

Committed: https://crrev.com/68dddcc6a3b8aea8c1836ad353e81993ef27d1ee
Cr-Commit-Position: refs/heads/master@{#433890}
==========

[commit-bot: I haz the power, 2016-11-22 17:15:24]

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