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 readablestream support #25873

Merged
merged 7 commits into from Jun 4, 2020

Conversation

@gterzian
Copy link
Member

gterzian commented Feb 29, 2020

FIX #21482
FIX #24876
FIX #26392


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #___ (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___
@highfive
Copy link

highfive commented Feb 29, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/interface.rs
  • @jgraham: tests/wpt/include.ini
  • @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/interface.rs
@gterzian gterzian force-pushed the gterzian:implement_readablestream_support branch 5 times, most recently from 93bfe8c to c6b6544 Feb 29, 2020
@gterzian gterzian force-pushed the gterzian:implement_readablestream_support branch 7 times, most recently from 7ab2dc8 to a54c975 Mar 5, 2020
@gterzian gterzian force-pushed the gterzian:implement_readablestream_support branch 4 times, most recently from 33a9a66 to a9f1ee2 Mar 13, 2020
@gterzian
Copy link
Member Author

gterzian commented Mar 15, 2020

@jdm Ok so this actually works now...

I still need to look at the net unit-tests, of which I think a few might be broken, but at the WPT level it's looking pretty good.

I squashed everything into one commit, sorry. The thing is that this grew quite organically and I didn't always knew what I was about to do in advance, so the various commits ended-up overlapping concerns.

It might actually make sense to review all in one go, since the various integration points use the various API's of the stream(which is why stuff grew organically as I added more integration, and then realized I need to change the API and add more integration or else break half of fetch...).

There are essentially five integrations:

  1. One with Request.
  2. One with Response.
  3. One with "consuming bodies"(both for responses and requests).
  4. One for "transmitting bodies", which is only about requests.
  5. One with getting a stream for a Blob.

This also depends on servo/rust-mozjs#495

@gterzian gterzian force-pushed the gterzian:implement_readablestream_support branch 4 times, most recently from bbc6812 to 3a2c24b Mar 15, 2020
@gterzian
Copy link
Member Author

gterzian commented Mar 15, 2020

Ok this breaks the request in devtools, however it doesn't look that hard to integrate that too. Now that is something I will do in a separate commit...

@bors-servo
Copy link
Contributor

bors-servo commented Mar 16, 2020

The latest upstream changes (presumably #25966) made this pull request unmergeable. Please resolve the merge conflicts.

@gterzian gterzian force-pushed the gterzian:implement_readablestream_support branch from 203426a to c395234 Mar 18, 2020
@gterzian
Copy link
Member Author

gterzian commented May 31, 2020

@jdm r?

@jdm
jdm approved these changes Jun 3, 2020
Copy link
Member

jdm left a comment

Looks good! Thanks for sticking with this!

Null,
/// Another Dom object as source,
/// TODO: store the actual object
/// and re-exctact a stream on re-direct.

This comment has been minimized.

Copy link
@jdm

jdm Jun 3, 2020

Member

Please file an issue for this if there isn't one already.

This comment has been minimized.

Copy link
@gterzian

gterzian Jun 4, 2020

Author Member

Yes that would be #26686

@gterzian gterzian force-pushed the gterzian:implement_readablestream_support branch from f3c70f1 to c1b7653 Jun 4, 2020
@gterzian
Copy link
Member Author

gterzian commented Jun 4, 2020

@bors-servo r=jdm

@jdm thanks for the review(s)!

@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2020

📌 Commit c1b7653 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2020

Testing commit c1b7653 with merge 8536cee...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 8536cee to master...

@bors-servo bors-servo merged commit 8536cee into servo:master Jun 4, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
@gterzian gterzian deleted the gterzian:implement_readablestream_support branch Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.