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

Servo should create a headers object in file_loader #4212

Closed
djzager opened this issue Dec 4, 2014 · 8 comments
Closed

Servo should create a headers object in file_loader #4212

djzager opened this issue Dec 4, 2014 · 8 comments
Labels
A-network C-assigned There is someone working on resolving the issue E-more-complex Variable effort required; may require a mentor. Recommended solution is clearly described in the iss

Comments

@djzager
Copy link

djzager commented Dec 4, 2014

Follow on issue to #3144

Currently when a file from the filesystem is loaded in servo, no response headers are generated. This prevents testing via XMLHttpRequest.

For reference:
http://pastebin.com/k3kyci7m

@jdm jdm added the A-network label Dec 4, 2014
@jdm
Copy link
Member

jdm commented Dec 4, 2014

<html>
<head>
  <script src="harness.js"></script>
</head>
<script>

  var url = "parsable_mime/image/png/test.png";
  var request = new XMLHttpRequest();
  request.open("GET", url, false);
  request.send(null);

  is(request.status, 200, 'Check for Status 200');
  // Test to see if any headers exist
  is_not(request.getAllResponseHeaders(), '', 'PNG test');
  // Test Content Type
  is(request.getResponseHeader('Content-Type'), 'image/png', 'PNG test');

/*
  var url = "";
  var request = new XMLHttpRequest();
  request.open("GET", url, false);
  request.send(null);

  is(request.getResponseHeader('Content-Type'), 'text/html', 'HTML test');
  is(request.getAllResponseHeaders(), 'image/png', 'PNG test');
  */


</script>
</html>

@jdm jdm added the E-more-complex Variable effort required; may require a mentor. Recommended solution is clearly described in the iss label Aug 26, 2015
@jdm
Copy link
Member

jdm commented Aug 26, 2015

For determining a Content-Type header, we could use https://github.com/cybergeek94/mime_guess .

@AdamSzopa
Copy link

I'll see what I can do!

@jdm jdm added the C-assigned There is someone working on resolving the issue label Oct 20, 2015
@Ms2ger Ms2ger removed the C-assigned There is someone working on resolving the issue label Nov 16, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 16, 2015

For future reference, jdm's test uses a test harness that no longer exists. Since the replacement (tests/wpt) runs from a http server, it's not trivial to translate the test.

An alternative could be a unit test (tests/unit/net/); I'm not sure what that would look like, though.

@KiChjang
Copy link
Contributor

I'll take this one.

@jdm jdm added the C-assigned There is someone working on resolving the issue label Nov 19, 2015
@KiChjang
Copy link
Contributor

I've gotten to the part where I've set content_type for metadata, yet metadata is still missing an appropriate Header. What should be included in a Header?

@jdm
Copy link
Member

jdm commented Nov 19, 2015

Plausibly we don't actually want a headers object, since that's very HTTP specific. If we just make the content_type value available, that should be enough.

@eefriedman
Copy link
Contributor

We need to fake up some actual headers for XMLHTTPRequest... see https://fetch.spec.whatwg.org/#concept-basic-fetch . Granted, it would be reasonable to argue that we don't need to do that in the network layer, and we should construct the fake headers in script/ where necessary.

For reference, the HTTP unit-tests manipulate Header objects in various ways: https://github.com/servo/servo/blob/master/tests/unit/net/http_loader.rs#L358 .

bors-servo pushed a commit that referenced this issue Nov 24, 2015
Add content_type to metadata in file_loader

Fixes #4212.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8609)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Nov 24, 2015
Add content_type to metadata in file_loader

Fixes #4212.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8609)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network C-assigned There is someone working on resolving the issue E-more-complex Variable effort required; may require a mentor. Recommended solution is clearly described in the iss
Projects
None yet
Development

No branches or pull requests

6 participants