Skip to content

/ servo

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

Jump to bottom

Implement the blobParts argument to the Blob constructor correctly. #8397

Closed
Ms2ger opened this issue Nov 7, 2015 · 19 comments
Closed

Implement the blobParts argument to the Blob constructor correctly. #8397

Ms2ger opened this issue Nov 7, 2015 · 19 comments
Labels
A-content/dom C-assigned E-less easy I-wrong

Comments

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Nov 7, 2015

Depends on #544.
Code: components/script/dom/webidls/Blob.webidl, components/script/dom/blob.rs.
Spec: https://w3c.github.io/FileAPI/#constructorBlob
@frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Jan 12, 2016

#544 is now done
@jdm jdm added the E-less easy label Jan 12, 2016
@jdm
Copy link
Member

@jdm jdm commented Jan 12, 2016

This should cover turning the existing DOMString blobParts argument into the more correct sequence<(Blob or DOMString)> instead, and processing it according to the specification.
@stspyder
Copy link
Contributor

@stspyder stspyder commented Jan 18, 2016

Is someone working on this? If no, can I pick this up?
@jdm
Copy link
Member

@jdm jdm commented Jan 18, 2016

Looks like #9360 has already been submitted for this. Sorry!
@jdm jdm added the C-assigned label Jan 18, 2016
@KiChjang
Copy link
Member

@KiChjang KiChjang commented Jan 18, 2016

@jdm #9360 doesn't seem to be touching anything about the Blob, did you mean to link to another issue instead?
@jdm
Copy link
Member

@jdm jdm commented Jan 18, 2016

Oops! Go for it, @stspyder!
@jdm jdm added C-assigned and removed C-assigned labels Jan 18, 2016
@stspyder
Copy link
Contributor

@stspyder stspyder commented Jan 18, 2016

Great!!
@stspyder
Copy link
Contributor

@stspyder stspyder commented Jan 26, 2016

I did implement the new constructor with a Vec<BlobOrString> but this wpt test times out. I think the test crashes, but is being timed out by the test harness. Any help figuring this out would be appreciated!!
blob_blobparts_constructor.txt
@jdm
Copy link
Member

@jdm jdm commented Jan 26, 2016

@stspyder Have you tried narrowing down which case triggers the timeout?
@stspyder
Copy link
Contributor

@stspyder stspyder commented Jan 26, 2016

@jdm "Passing non-objects, Dates and RegExps for blobParts should throw a TypeError." is the test that causes the timeout. It doesn't get past passing null to Blob constructor. I tried debugging and had unexpected crashes with exit code of 8 when in the _constructor of BlobBinding.rs. But sometimes I was able to see that the conversion of the 1st argument (to blob constructor) fails and the method returns false.
@jdm
Copy link
Member

@jdm jdm commented Jan 29, 2016 

14:40 <jdm> stspyder: oho - https://gist.github.com/stspyder/19e49baf536cf0517938#file-blobbinding-rs-L23 and https://gist.github.com/stspyder/19e49baf536cf0517938#file-blobbinding-rs-L30 would cause these problems
14:40 <jdm> stspyder: since they're missing https://gist.github.com/stspyder/19e49baf536cf0517938#file-blobbinding-rs-L37
14:42 <jdm> stspyder: well, actually they're missing a throw_type_error call
@nox
Copy link
Member

@nox nox commented Jan 29, 2016

@jdm No, that throw_dom_exception is for the actual return value of the method. Instead we should call throw_type_error from the implementation of FromJSValConvertible on Vec<T>, like it is done in the inner BlobOrString impl.
@nox
Copy link
Member

@nox nox commented Jan 29, 2016

Oh I guess you mentioned the throw_type_error part.
@Ms2ger
Copy link
Contributor Author

@Ms2ger Ms2ger commented Feb 1, 2016

servo/rust-mozjs#237
@nox
Copy link
Member

@nox nox commented Mar 12, 2016

That PR landed a while ago.
tbu- added a commit to tbu-/servo that referenced this issue Mar 12, 2016
@tbu-
Start accepting correct parameters for the Blob constructor
c955a9e 
Touches servo#8397.
@tbu- tbu- mentioned this issue Mar 12, 2016
Closed
bors-servo added a commit that referenced this issue Mar 12, 2016
@bors-servo
Auto merge of #9977 - tbu-:pr_blob_constructor, r=<try>
Loading status checks…
4595dda 
Start accepting correct parameters for the Blob constructor

Touches #8397.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9977)
<!-- Reviewable:end -->
@stspyder
Copy link
Contributor

@stspyder stspyder commented Mar 12, 2016

Well, sorry it took me a while to come back on this. But most of my work is complete already. I'll submit a PR shortly. Don't want someone to duplicate efforts here.
@jdm
Copy link
Member

@jdm jdm commented Mar 12, 2016

@stspyder See also #9977 that has already been opened :/
@stspyder
Copy link
Contributor

@stspyder stspyder commented Mar 12, 2016

@jdm, yes noticed it. #9977 requires changes in tests for the wpt tests to pass which are available in #9979. I'll close #9979 if those changes can be done in #9977.
@stspyder stspyder mentioned this issue Mar 26, 2016
Merged
@jdm
Copy link
Member

@jdm jdm commented Mar 26, 2016

Fixed by #9979.
@jdm jdm closed this Mar 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-content/dom C-assigned E-less easy I-wrong
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
@jdm @Ms2ger @nox @frewsxcv @KiChjang @stspyder
You can’t perform that action at this time.