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

Implement TextDecoder. #4769

Closed
Ms2ger opened this issue Jan 29, 2015 · 21 comments
Closed

Implement TextDecoder. #4769

Ms2ger opened this issue Jan 29, 2015 · 21 comments

Comments

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Jan 29, 2015

Spec: https://encoding.spec.whatwg.org/#interface-textdecoder.

Most of the actual decoding infrastructure should already be implemented in rust-encoding.

This includes enabling the relevant tests by adding

[encoding]
  skip: false

to tests/wpt/include.ini.

This may also include updating the mozjs and rust-mozjs dependencies to ensure we can work with typed arrays.

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Feb 7, 2015

Alright, time to actually move from easy to less easy, lemme try and flex my muscles on this one.
I'm assuming I'll have to create new files, yes? They should be located in the components/script/dom directory?

@jdm
Copy link
Member

@jdm jdm commented Feb 8, 2015

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Feb 8, 2015

Question... in the HTML spec it says that the TextDecoder::decode function must accept an optional BufferSource. What would the be the equivalent of BufferSource in rust-speak?

@jdm
Copy link
Member

@jdm jdm commented Feb 8, 2015

Good question! I've filed a spec bug about that: https://www.w3.org/Bugs/Public/show_bug.cgi?id=27979 . Googling around, I found http://heycam.github.io/webidl/#common-BufferSource, so I suggest adding that definition to the file for now.

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Feb 8, 2015

Okay, so the question becomes "what is an ArrayBuffer in rust?"
Looking at the Servo documentation, I found std::io::Buffer. Would that suffice as an ArrayBuffer?

@jdm
Copy link
Member

@jdm jdm commented Feb 8, 2015

Nope - ArrayBuffer actually corresponds to a *mut JSObject, as it's a type that the JS engine provides. See #4639 for another example of a DOM implementation that uses them.

@jdm
Copy link
Member

@jdm jdm commented Feb 8, 2015

Well, it's uses Uint8ClampedArray, but that's just a a specialized form of ArrayBuffer.

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Feb 14, 2015

Oh, big problem - the python script that generates the bindings threw an exception saying TypeError:: Can't handle SpiderMonkey Interface arguments yet whenever I tried to use ArrayBuffer or ArrayBufferView. Uint8Array gives me the same TypeError as well.

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Feb 15, 2015

Slight issue... now when JavaScript does new TextDecoder(), Rust throws a SyntaxError. The default label argument to the constructor is utf-8, but encoding_from_whatwg_labeldoesn't like the default utf-8 string generated by the python bindings. Instead, if you do new TextDecoder('utf-8') in JavaScript, then encoding_from_whatwg_label doesn't complain.

I tried trim() on the &str that gets passed as an argument to encoding_from_whatwg_label to no avail.

@Ms2ger
Copy link
Contributor Author

@Ms2ger Ms2ger commented Feb 15, 2015

Pre-existing bug; I'll deal with it.

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Feb 17, 2015

Is there an issue number that tracks the bug that @Ms2ger mentioned? The SpiderMonkey interface arguments for the WebIDL also needs to be implemented (Uint8Array, ArrayBuffer, ...).

@yodalee
Copy link
Contributor

@yodalee yodalee commented Feb 19, 2015

I see the the sample code posted by jdm using Uint8ClampedArray, I try to add

use js::jsfriendapi::bindgen::{JS_NewArrayBuffer, JS_GetArrayBufferData};

as depict in file: components/bindings/codegen/TypedArray.h
But compiler cannot resolve this include: unresolved import
I trace the code in components/servo/target/build/mozjs-sys-6b64ab2c5b5d63e7/out/dist/include, only Array of uint8clampedArray got JS_BEGIN_EXTERN_C and JS_END_EXTERN_C around them. I think mozjs doesn't support declaration of JS_BufferArray yet.

@Ms2ger
Copy link
Contributor Author

@Ms2ger Ms2ger commented Feb 19, 2015

You'll need changes like servo/mozjs#25 and servo/rust-mozjs#128, and then use ./mach update-cargo -p js to pull in those changes.

@yodalee
Copy link
Contributor

@yodalee yodalee commented Feb 19, 2015

See, I will do the changes to servo/mozjs first.

@yodalee
Copy link
Contributor

@yodalee yodalee commented Feb 21, 2015

@KiChjang Thanks Ms2ger, now we can use all TypedArray.
Sorry wait... I don't know why my servo still use mozjs with clamparray only.

@jdm
Copy link
Member

@jdm jdm commented Feb 21, 2015

Oh, #4998 only updated rust-mozjs, not mozjs. Use -p js-sys for that one.

@yodalee
Copy link
Contributor

@yodalee yodalee commented Feb 21, 2015

It says js-sys matched no packages

@yodalee
Copy link
Contributor

@yodalee yodalee commented Feb 21, 2015

OK I found, use -p mozjs-sys

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Feb 22, 2015

I'm not entirely sure how to use these yet... I did ./mach update-cargo -p js and ./mach update-cargo -p mozjs-sys, but it still gave me TypeError: Can't handle SpiderMonkey interface arguments yet. I even rebased my branch to the lastest commit on master, but that didn't work.

@Ms2ger
Copy link
Contributor Author

@Ms2ger Ms2ger commented Feb 22, 2015

I would suggest using object in the IDL for now.

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Feb 24, 2015

That wouldn't work either; it would just tell me TypeError: Can't handle object arguments yet. I think this issue is going to be blocked until CodegenRust.py can generate either Uint8Array/ArrayBuffer/BufferSource arguments or object arguments

@Ms2ger Ms2ger closed this in 4e5ab24 Apr 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
@jdm @Ms2ger @yodalee @KiChjang and others
You can’t perform that action at this time.