make custom schemes and HandleBeforeResourceLoad return HTTP status codes for jQuery XHR

[GoogleCodeExporter]

Currently there is no way to return a HTTP status code when using either a 
custom scheme or during HandleBeforeResourceLoad. This causes issues when using 
jQuery XHR.  Please see the following thread:

http://magpcss.org/ceforum/viewtopic.php?f=7&t=250&start=0

Original issue reported on code.google.com by crazy.fu...@gmail.com on 9 Mar 2011 at 9:58

• Blocked on: #221

[GoogleCodeExporter]

Issue 168 has been merged into this issue.

Original comment by magreenb...@gmail.com on 30 Mar 2011 at 1:13

[GoogleCodeExporter]

I have started working on a fix for this, that thus far seems to be working 
well.  I figured I would rough though my steps and see what you thought. I 
don't know the repository well enough to post a patch.

first I defined:

typedef struct _cef_response_info_t
{
  size_t size;
  int response_code;
  cef_string_t response_text;
} cef_response_info_t;

and a matching CefResponseInfo.

I added this to HandleBeforeResourceLoad

In 'browser_resource_loader_bridge.cc'

before 'handler->HandleBeforeResourceLoad'

I defined 'CefResponseInfo responseInfo'

in the 'else if(resourceStream.get())' block I added:

std::stringstream raw_headers;
raw_headers << "HTTP/1.1 " << responseInfo.response_code << " ";
if( !responseInfo.response_text.length )
 raw_headers << ( responseInfo.response_code == 200 ? "OK" : "ERROR" ) << "\n";
else
 raw_headers << responseInfo.response_text.str << "\n";

Couple random thoughts:

Currently I set responseInfo.response_code to 200 after the define. Not sure if 
this would be better in the class definition. As I'm typing this might be 
better to leave it to 0 and use it as another way to way to determine not to 
continue.

I wasn't sure on the use of the stringstream. Couldn't see a standard for this, 
but didn't look too hard.

I would possibly merge the mime type into this, since they both get put into 
the ResourceResponseInfo structure and thus make it easy to expand for other 
items latter (for example http version which I have hard coded ).

Cheers,
-cfr

Original comment by crazy.fu...@gmail.com on 5 Apr 2011 at 4:35

[GoogleCodeExporter]

We currently have a CefResponse class. The best approach would likely be making 
this class read/write (similar to CefRequest) and passing a reference to 
CefBrowser::HandleBeforeResourceLoad and CefSchemeHandler::ProcessRequest. I 
agree that mime type could also be made part of CefResponse.

For HandleBeforeResourceLoad the HTTP response code needs to be put into the 
ResourceResponseInfo::headers member in RequestProxy::AsyncStart via a call to 
HttpResponseHeaders::ParseStatusLine before calling OnReceivedResponse so that 
it can be retrieved in webkit/glue/weburlloader_impl.cc PopulateURLResponse 
(called via RequestProxy::NotifyReceivedResponse).

For ProcessRequest we'll need to override URLRequestJob::GetResponseInfo in 
CefUrlRequestJob and then do the same for the HttpResponseInfo::headers member.

Original comment by magreenb...@gmail.com on 5 Apr 2011 at 5:39

[GoogleCodeExporter]

I did make my change in AsyncStart before OnReceivedResponse, although not via 
HttpResponseHeaders::ParseStatusLine.

I would love to help get this implemented, but don't know what the best next 
step is for me.

Original comment by crazy.fu...@gmail.com on 5 Apr 2011 at 7:02

[GoogleCodeExporter]

I just realized I left out a piece of my code. after 'raw_headers <<' I have:

scoped_refptr<net::HttpResponseHeaders> headers(
 new net::HttpResponseHeaders(raw_headers.str()));
info.headers = headers;

Which calls HttpResponseHeaders::ParseStatusLine

Original comment by crazy.fu...@gmail.com on 5 Apr 2011 at 9:13

[GoogleCodeExporter]

You're on the right path. Instead of creating a CefResponseInfo structure you 
should use the existing CefResponse class. Change the CefResponse class so that 
it has both set and get methods -- see CefRequest for an example of how to 
implement it.

You should consider using TortoiseSVN on Windows if you're not already doing 
so. It integrates with Windows Explorer and makes it very easy to work with SVN 
repositories, create patches, etc.

Original comment by magreenb...@gmail.com on 6 Apr 2011 at 1:50

[GoogleCodeExporter]

I'll try TortoiseSVN and see if I can make a patch of what I've done so far.

Currently I have manually edited the cpptoc/ctocpp files. Once I update cef.h 
do I run the translator.bat to re-create these files, then make any changes in 
the body?

Original comment by crazy.fu...@gmail.com on 6 Apr 2011 at 3:13

[GoogleCodeExporter]

Correct, run translator.bat to create the cpptoc/ctocpp files and then edit the 
body.

Original comment by magreenb...@gmail.com on 6 Apr 2011 at 3:17

[GoogleCodeExporter]

For building the status line, do you have a built in string builder?

Original comment by crazy.fu...@gmail.com on 6 Apr 2011 at 3:42

[GoogleCodeExporter]

You can use base::SStringPrintf from base/stringprintf.h

Original comment by magreenb...@gmail.com on 6 Apr 2011 at 3:46

[GoogleCodeExporter]

in responce_impl.cc, do I need the AutoLock lock_scope(this) in each get/set 
like in request_impl.cc?

Original comment by crazy.fu...@gmail.com on 6 Apr 2011 at 5:12

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Ok, here is my first pass. This is for HandleBeforeResourceLoad only. Thoughts?

Original comment by crazy.fu...@gmail.com on 6 Apr 2011 at 6:42

Attachments:

• issue_202_fix_rev1.patch

[GoogleCodeExporter]

A few comments:

1. Use AutoLock in CefResponseImpl because the CefResponse methods may be 
called from multiple threads.

2. GenerateResponseLine() isn't something that will be called by API users so 
make it a non-virtual member of the CefResponseImpl class instead.

3. Change the HandleBeforeResourceLoad signature/usage in cefclient_mac.mm and 
the cef_unittests project.

Other than that looks good.

Original comment by magreenb...@gmail.com on 6 Apr 2011 at 6:43

[GoogleCodeExporter]

Will do.

I'm looking at CefUrlRequestJob

in CefUrlRequestJob::AsyncResolver::Resolve there is currently:

owner_->x_ = x;

For each var.  Should I implement the same thing and do for each var:

owner_->response_->SetX( response->GetX() );

Or implement a copy?

owner_->response_= new CefResponseImpl(response)

Original comment by crazy.fu...@gmail.com on 6 Apr 2011 at 7:41

[GoogleCodeExporter]

Instead of copying the CefResponseImpl you could just keep a reference to it.

Original comment by magreenb...@gmail.com on 6 Apr 2011 at 7:47

[GoogleCodeExporter]

so create a new CefResponseImpl in CefUrlRequestJob and pass that instance to 
ProcessRequest?

Original comment by crazy.fu...@gmail.com on 6 Apr 2011 at 7:49

[GoogleCodeExporter]

Correct. Create the instance in AsyncResolver::Resolve, assign it to a new 
CefRefPtr<CefResponse> member of CefUrlRequestJob, and pass that same instance 
to ProcessRequest. It would then be accessed from the new GetResponseInfo 
method as well.

Original comment by magreenb...@gmail.com on 6 Apr 2011 at 8:00

[GoogleCodeExporter]

Ok, so here's rev2. Implemented your comments above and first pass for 
CefUrlRequestJob
.  Checked using cefclient|debug tools|network|refresh all items have a valid 
status

Original comment by crazy.fu...@gmail.com on 6 Apr 2011 at 8:00

Attachments:

• issue_202_fix_rev2.patch

[GoogleCodeExporter]

so in my above patch, i created it in CefUrlRequestJob::Start. I didn't know if 
somehow GetResponseInfo or GetMimeType could get called before 
AsyncResolver::Resolve was done and didn't want the null pointer.  Thoughts?

Original comment by crazy.fu...@gmail.com on 6 Apr 2011 at 8:06

[GoogleCodeExporter]

Creating the CefRequestImpl instance in CefUrlRequestJob::Start is fine.

A few comments on your most recent patch:

1. Wrap all lines at 80 characters. Follow the existing code patterns for 
proper indenting. See the "Column Limit" section here for a useful Visual 
Studio trick: http://dev.chromium.org/developers/how-tos/visualstudio-tricks

2. Add a CefUrlResponse::SetHeaderMap method and then AutoLock the 
header-related methods as well. The header information should then also be 
transferred to the HttpResponseHeaders instance.

Original comment by magreenb...@gmail.com on 6 Apr 2011 at 8:49

[GoogleCodeExporter]

That should say "Creating the CefResponseImpl instance in 
CefUrlRequestJob::Start is fine.
"

Original comment by magreenb...@gmail.com on 6 Apr 2011 at 8:50

[GoogleCodeExporter]

I assume that it should be add 'CefResponse::SetHeaderMap'? also should there 
be a SetHeader as well?

Original comment by crazy.fu...@gmail.com on 6 Apr 2011 at 9:20

[GoogleCodeExporter]

if not here, is rev3

Original comment by crazy.fu...@gmail.com on 6 Apr 2011 at 9:22

Attachments:

• issue_202_fix_rev3.patch

[GoogleCodeExporter]

Looks good. The next step is to create unit tests for the new functionality. We 
don't have any unit tests currently for scheme handlers so we need to create a 
new file for those. The HandleBeforeResourceLoad change can be tested in 
request_unittest.cc.

Original comment by magreenb...@gmail.com on 7 Apr 2011 at 6:01

[GoogleCodeExporter]

Original comment by magreenb...@gmail.com on 18 Apr 2011 at 2:49

• Changed state: Accepted

[GoogleCodeExporter]

Committed with minor modifications as revision 222.

Original comment by magreenb...@gmail.com on 21 Apr 2011 at 4:49

• Changed state: Fixed