adding protocol.RequestBufferJob api #1208
Conversation
| v8::Local<v8::Object> buffer); | ||
|
|
||
| // URLRequestSimpleJob: | ||
| int GetData(std::string* mime_type, |
There was a problem hiding this comment.
It is not safe to store binary data in std::string, you should override GetRefCountedData instead of GetData.
There was a problem hiding this comment.
@zcbenz url_request_simple_job.h in libchromium has GetData implemented as pure virtual method and there is no GetRefCountedData virtual method to override, which is different from https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/url_request_simple_job.h&q=url_request_simple&sq=package:chromium&l=55 . Are we on an old version ? GetRefCountedData seems to have been implemented https://code.google.com/p/chromium/issues/detail?id=422489 here.
There was a problem hiding this comment.
Ah GetRefCountedData is not available until Chrome 41, I think we can revisit this after #1243 is finished.
deepak1556
force-pushed
the
protocol
branch
2 times, most recently
from
509528d
to
37f7a27
Compare
|
works after the upgrade to 41, any comments @zcbenz ? |
| @@ -10,6 +10,7 @@ | |||
| #include "base/memory/weak_ptr.h" | |||
| #include "net/url_request/url_request_job.h" | |||
| #include "net/url_request/url_request_job_factory.h" | |||
| #include "atom/common/node_includes.h" | |||
There was a problem hiding this comment.
We should include v8.h here, node_includes.h should only be used when you use APIs in node.h, because node_includes.h did many hacks to make it possible.
|
fixed, thanks! |
|
✨ |
adding protocol.RequestBufferJob api
Need to work on the crashing tests