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

Additional SchemeHandlerResponse options #184

Merged
merged 5 commits into from
Oct 29, 2013

Conversation

provegard
Copy link
Contributor

This PR adds a number of properties/options to SchemeHandlerResponse:

  • StatusCode, to return something else than status 200 to CEF.
  • RedirectUrl, to have CEF do redirects.
  • ContentLength, to provide a content length to CEF regardless of whether the response stream is seekable or not, for optimal response reading.
  • CloseStream, so that SchemeHandlerWrapper can close the response stream once it has read all data.

It also fixes a bug in SchemeHandlerWrapper, which didn't return false upon response completion in ReadResponse.

The ultimate goal of all changes were to be able to write a SchemeHandler implementation for proxying HTTP requests, which now is possible.

I'm sorry to say that I couldn't write any unit tests. I don't know how to instantiate SchemeHandlerWrapper from the CefSharp.Test project. Any suggestions?

@perlun
Copy link
Member

perlun commented Oct 13, 2013

Thanks - looks good! Will merge this asap. As for the unit tests, I guess (don't have it open at the moment) that the test project in C#. If so, that means it is not possible to instantiate unmanaged types so I guess this could be what you're seeing? Or is it because it's internal?

Nonetheless, the test coverage isn't that great with the existing code either, so I don't think that's a big deal with this PR (meaning - it can be merged anyway, but if you or someone else manages to add unit tests for it, it's of course a Good Thing also). It would be nice to try and get the general test coverage improved; feel free to elaborate to that avail if you have time & energy.

@provegard
Copy link
Contributor Author

Great!

You're right, I couldn't instantiate an unmanaged type (the C# code only sees a generated struct without constructor), and the only factory I could find is internal.

@perlun
Copy link
Member

perlun commented Oct 21, 2013

(Will merge this asap, but I don't want to do it yet since I must remember to also graft over the changes to the CefSharp3 branch, so they aren't "forgotten". CefSharp1 will be frozen during this fall I hope, so it's important that all changes made at this moment are made on both branches.)

@provegard
Copy link
Contributor Author

Sure!

@perlun
Copy link
Member

perlun commented Oct 21, 2013

If you want to be really kind & helpful, you could even fix it on CefSharp3 and send a separate PR for that. 😄 (but it will only work with VS2012/x64 for the time being, only because I haven't had time yet to recompile the CEF wrapper with VS2010 and/or x86...)

@perlun
Copy link
Member

perlun commented Oct 29, 2013

Checked it now, looks good. Extra credits to you for putting in proper XML comments - that's really good, we should try to improve the general API documentation in that regard I think. I will merge you stuff now and also try and cherry pick it over to the CefSharp3 branch. Thanks for your contribution, looking for more nice changes coming from my original home country. 😄

perlun added a commit that referenced this pull request Oct 29, 2013
Additional SchemeHandlerResponse options
@perlun perlun merged commit f59f02f into cefsharp:CefSharp1 Oct 29, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants