Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing response clone test and ironing utils.js for different UInt8Array behavior #3274

Merged
merged 1 commit into from Jul 13, 2016

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Jul 7, 2016

No description provided.

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/6678

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@wpt-pr-bot
Copy link
Collaborator

Reviewers for this pull request are: @jdm and @youennf.

@jdm
Copy link
Contributor

jdm commented Jul 7, 2016

It's not clear to me that papering over differences in the Uint8ArrayBuffer constructor is helpful. That's a standard API with well-defined semantics; if it's not implemented according to the specification, why do we want to hide that fact?

@youennf
Copy link
Contributor Author

youennf commented Jul 7, 2016

I believe all browsers accept "new Uint8Array()".
Some browsers seem to accept "new Uint8Array(undefined)" while others do not.
From a quick reading of the spec, the second behavior seems compliant.
Given that, since for empty streams, undefined may be passed to Uint8Array, I added a check in this method. I am fine changing the caller function though.

@youennf
Copy link
Contributor Author

youennf commented Jul 12, 2016

If needed, I can split the patch to at least fix the response clone test

@jdm jdm merged commit 60cbafb into web-platform-tests:master Jul 13, 2016
@jdm
Copy link
Contributor

jdm commented Jul 13, 2016

Thinking about it further, I'm fine with this change. There are other tests that can focus on ES type specification behaviour.

ddorwin pushed a commit to ddorwin/web-platform-tests that referenced this pull request Sep 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants