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 TextEncoder (fixes #4768). #5025

Closed
wants to merge 3 commits into from

Conversation

@yodalee
Copy link
Contributor

yodalee commented Feb 23, 2015

This PR need modification, but I think some review is fine.
I run the wpt test, there are some issues:

  • The textencoder constructor test will fail, because it return IndexError instead of RangeError in spec.
  • Other fail I cannot understand why....
@highfive
Copy link

highfive commented Feb 23, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Feb 23, 2015

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

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.

@jdm
Copy link
Member

jdm commented Feb 23, 2015

What are the other failures you see?

@yodalee
Copy link
Contributor Author

yodalee commented Feb 23, 2015

hmm, there are so many, most of them are decoder fail. Here is the link to log summary.
https://dl.dropboxusercontent.com/u/3192346/log.txt

  • There is other error like USVString handling, since USVString is not supported now.
  • Another error is Non-UTF encodings supported only for decode, not encode, which... is weird. I don't thine I support Non-UTF encoding.
    btw, can I run wpt-test encoding section only, run all wpt-test really takes many time.
@jdm
Copy link
Member

jdm commented Feb 23, 2015

Yes, ./mach test tests/wpt/web-platform-tests/path/to/dir --processes=N should work.

@jdm
Copy link
Member

jdm commented Feb 23, 2015

Ah. My recommendation with the failures is to make a PR that enables the encoding directory and following the steps at http://mxr.mozilla.org/servo/source/tests/wpt/README.md#71 to save the default failures that occur without any other changes. Then you can apply your other changes on top of that and see which failures go away.

@yodalee
Copy link
Contributor Author

yodalee commented Feb 23, 2015

I see, let me try this and give you a clear log.

@jdm
Copy link
Member

jdm commented Feb 24, 2015

I've left comments on Critic :)

@yodalee yodalee force-pushed the yodalee:issue4768-textencoder branch from 5f8ff15 to 1f5973f Feb 27, 2015
@yodalee
Copy link
Contributor Author

yodalee commented Feb 27, 2015

I rebase the branch to newest. Also, I found that only 4 tests being passed after the textencoder added T_T

@jdm
Copy link
Member

jdm commented Feb 28, 2015

Yeah, I suspect the tests are relying on things like document.inputEncoding and TextDecoder, unfortunately.

@yodalee yodalee force-pushed the yodalee:issue4768-textencoder branch from 1f5973f to 8e85388 Mar 1, 2015
@yodalee
Copy link
Contributor Author

yodalee commented Mar 1, 2015

rebase to newest master and solve datatype issue. Though I think it still cast usize to u32 and causing some loss.

@pcwalton pcwalton force-pushed the servo:master branch from fed8787 to bf60477 Mar 2, 2015
@yodalee yodalee force-pushed the yodalee:issue4768-textencoder branch from 8e85388 to d2baf23 Mar 9, 2015
@jdm
Copy link
Member

jdm commented Mar 12, 2015

I believe there's just one nit left here, based on the Critic review - there should be a space before the { on struct TextEncoder{.

@yodalee yodalee force-pushed the yodalee:issue4768-textencoder branch from d2baf23 to ff76b2f Mar 12, 2015
@yodalee
Copy link
Contributor Author

yodalee commented Mar 12, 2015

I didn't notice the new issue, should be fixed now.

* mark most tests as fail
* remove 10 passed tests in wpt tests
* add textencoder into exposed interface
@yodalee yodalee force-pushed the yodalee:issue4768-textencoder branch from 7b8c622 to 314bf79 Mar 14, 2015
@yodalee
Copy link
Contributor Author

yodalee commented Mar 14, 2015

Done. I squash it into three commits: constructor, implement method, and enable testings

@jdm

This comment has been minimized.

Copy link

jdm commented on 314bf79 Mar 14, 2015

r+

This comment has been minimized.

Copy link

jdm replied Mar 14, 2015

@bors: retry

@jdm jdm added S-awaiting-merge and removed S-needs-squash labels Mar 14, 2015
@jdm
Copy link
Member

jdm commented Mar 14, 2015

Thanks!

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented on 314bf79 Mar 14, 2015

saw approval from jdm
at yodalee@314bf79

This comment has been minimized.

Copy link
Contributor

bors-servo replied Mar 14, 2015

merging yodalee/servo/issue4768-textencoder = 314bf79 into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Mar 14, 2015

yodalee/servo/issue4768-textencoder = 314bf79 merged ok, testing candidate = db21010

This comment has been minimized.

Copy link
Contributor

bors-servo replied Mar 14, 2015

This comment has been minimized.

Copy link
Contributor

bors-servo replied Mar 14, 2015

saw approval from jdm
at yodalee@314bf79

This comment has been minimized.

Copy link
Contributor

bors-servo replied Mar 14, 2015

merging yodalee/servo/issue4768-textencoder = 314bf79 into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Mar 14, 2015

yodalee/servo/issue4768-textencoder = 314bf79 merged ok, testing candidate = 0b2d983

This comment has been minimized.

Copy link
Contributor

bors-servo replied Mar 14, 2015

bors-servo pushed a commit that referenced this pull request Mar 14, 2015
This PR need modification, but I think some review is fine.
I run the wpt test, there are some issues:
* The textencoder constructor test will fail, because it return `IndexError` instead of `RangeError` in spec.
* Other fail I cannot understand why....
@yodalee
Copy link
Contributor Author

yodalee commented Mar 14, 2015

I cannot reproduce the crash result of /encoding/single-byte-decoder.html, it end normally at my local test OAO

@jdm
Copy link
Member

jdm commented Mar 14, 2015

Yeah, that looks kind of like resource exhaustion on the builder machine. It's weird that it's happening for this test in particular :/

bors-servo pushed a commit that referenced this pull request Mar 14, 2015
This PR need modification, but I think some review is fine.
I run the wpt test, there are some issues:
* The textencoder constructor test will fail, because it return `IndexError` instead of `RangeError` in spec.
* Other fail I cannot understand why....
@yodalee
Copy link
Contributor Author

yodalee commented Mar 16, 2015

@jdm
on my local computer, I change the async_test into test in single-byte-decoder.html. Passed subtest increase to 334 from 158. Maybe we can try this on remote test?

@Ms2ger
Copy link
Contributor

Ms2ger commented Mar 16, 2015

You mean you made the test not test anything.

@yodalee
Copy link
Contributor Author

yodalee commented Mar 16, 2015

oops, I think I misunderstand the meaning of async_test and test.

@jdm
Copy link
Member

jdm commented Mar 16, 2015

The good news is that I can reproduce the failure locally:

godot2:servo jdm$ ./mach test tests/wpt/web-platform-tests/encoding/single-byte-decoder.html
 0:03.60 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) Full command: /Users/jdm/src/servo/components/servo/target/servo --cpu --hard-fail -z http://localhost:8000/encoding/single-byte-decoder.html
(pid:72816) "thread 'PaintTask' panicked at 'failed to spawn native thread: 35', /Users/larsberg/rust/src/libstd/sys/unix/thread.rs:229"
 0:03.60 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "thread 'LayoutTask' panicked at 'failed to spawn native thread: 35', /Users/larsberg/rust/src/libstd/sys/unix/thread.rs:229"
 0:03.60 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "thread 'PaintTask' panicked at 'failed to spawn native thread: 35', /Users/larsberg/rust/src/libstd/sys/unix/thread.rs:229"
 0:03.60 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "thread 'LayoutTask' panicked at 'failed to spawn native thread: 35', /Users/larsberg/rust/src/libstd/sys/unix/thread.rs:229"
 0:03.60 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "thread 'Constellation' panicked at 'failed to spawn native thread: 35', /Users/larsberg/rust/src/libstd/sys/unix/thread.rs:229"
 0:03.60 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "thread 'PaintTask' panicked at 'failed to spawn native thread: 35', /Users/larsberg/rust/src/libstd/sys/unix/thread.rs:229"
 0:03.60 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "thread 'ScriptTask' panicked at 'failed to spawn native thread: 35', /Users/larsberg/rust/src/libstd/sys/unix/thread.rs:229"
 0:03.61 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "thread 'LayoutWorker worker 1/6' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', /Users/larsberg/rust/src/libcore/result.rs:743"
 0:03.61 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "thread 'LayoutWorker worker 2/6' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', /Users/larsberg/rust/src/libcore/result.rs:743"
 0:03.61 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "thread 'LayoutWorker worker 3/6' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', /Users/larsberg/rust/src/libcore/result.rs:743"
 0:03.61 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', /Users/larsberg/rust/src/libcore/result.rs:743"
 0:03.61 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', /Users/larsberg/rust/src/libcore/result.rs:743"
 0:03.61 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', /Users/larsberg/rust/src/libcore/result.rs:743"
 0:03.61 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', /Users/larsberg/rust/src/libcore/result.rs:743"
 0:03.61 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', /Users/larsberg/rust/src/libcore/result.rs:743"
 0:03.61 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "thread '<unnamed>thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', /Users/larsberg/rust/src/libcore/result.rs:743"
 0:03.61 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', /Users/larsberg/rust/src/libcore/result.rs:743"
 0:03.61 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:72816) "fatal runtime error: Could not unwind stack, error = 5"
@jdm
Copy link
Member

jdm commented Mar 16, 2015

Wow, that test actually does spawn an incredible number of tasks for some reason.

@jdm
Copy link
Member

jdm commented Mar 16, 2015

So it's the async_test that creates a lot of iframes that is doing us in, specifically. For the purposes of this PR, I suggest just annotating that a crash is expected from this test, and we can file another bug for improving our thread creation eagerness and making this test not crash.

@metajack
Copy link
Contributor

metajack commented Mar 31, 2015

@mbrubeck Assigned to you to figure out next steps.

@mbrubeck
Copy link
Contributor

mbrubeck commented Mar 31, 2015

I moved these patches to a new PR #5469 and added the suggested test change, so we can hopefully land them. Thank you, @yodalee!

@mbrubeck mbrubeck closed this Mar 31, 2015
bors-servo pushed a commit that referenced this pull request Mar 31, 2015
…=jdm

This is a series of already-reviewed changes by @yodalee from #5025, rebased onto current servo master, with some fixups applied:

* Fixed build errors/warnings from the latest rust upgrade.
* Marked `tests/wpt/web-platform-tests/encoding/single-byte-decoder.html` as expecting CRASH.

I could not verify locally that the new test annotation is correct, since the test appears to hang rather than crash on my Linux box.  (Or maybe I just didn't wait long enough.)  If this crash isn't consistent, or if it takes a long time, maybe we should skip this test instead?

r? @jdm
bors-servo pushed a commit that referenced this pull request Apr 1, 2015
…=jdm

This is a series of already-reviewed changes by @yodalee from #5025, rebased onto current servo master, with some fixups applied:

* Fixed build errors/warnings from the latest rust upgrade.
* Marked `tests/wpt/web-platform-tests/encoding/single-byte-decoder.html` as expecting CRASH.

I could not verify locally that the new test annotation is correct, since the test appears to hang rather than crash on my Linux box.  (Or maybe I just didn't wait long enough.)  If this crash isn't consistent, or if it takes a long time, maybe we should skip this test instead?

r? @jdm
bors-servo pushed a commit that referenced this pull request Apr 1, 2015
…=jdm

This is a series of already-reviewed changes by @yodalee from #5025, rebased onto current servo master, with some fixups applied:

* Fixed build errors/warnings from the latest rust upgrade.
* Marked `tests/wpt/web-platform-tests/encoding/single-byte-decoder.html` as expecting CRASH.

I could not verify locally that the new test annotation is correct, since the test appears to hang rather than crash on my Linux box.  (Or maybe I just didn't wait long enough.)  If this crash isn't consistent, or if it takes a long time, maybe we should skip this test instead?

r? @jdm
@yodalee yodalee deleted the yodalee:issue4768-textencoder branch Apr 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.