XMLHttpRequest API

This is only the starting point: a _request.GET() method is being injected now, a JS-based XMLHttpRequest implementation needs to be built around that. Also, a callback to retrieve URLs has been added but no implementations for this callback yet (there will be three implementations from the look of it).

[Felix Dahlke, 2013-04-11 09:33:42]

http://codereview.adblockplus.org/10259001/diff/1/include/AdblockPlus/WebRequ...
File include/AdblockPlus/WebRequest.h (right):

http://codereview.adblockplus.org/10259001/diff/1/include/AdblockPlus/WebRequ...
include/AdblockPlus/WebRequest.h:2: #define ADBLOCKPLUS_WEB_REQUEST_H
We usually have ADBLOCK_PLUS (with an underscore) as a prefix for include
guards.

http://codereview.adblockplus.org/10259001/diff/1/include/AdblockPlus/WebRequ...
include/AdblockPlus/WebRequest.h:9: typedef std::vector< std::pair<std::string,
std::string> > HeadersList;
We usually only have whitespace between >/< if it'd end up being >>/<<
otherwise, i.e. not the one after the < here.

http://codereview.adblockplus.org/10259001/diff/1/include/AdblockPlus/WebRequ...
include/AdblockPlus/WebRequest.h:13: HeadersList responseHeaders;
How about "Headers" or "HeaderList"? I prefer the former.

http://codereview.adblockplus.org/10259001/diff/1/src/GlobalJsObject.cpp
File src/GlobalJsObject.cpp (right):

http://codereview.adblockplus.org/10259001/diff/1/src/GlobalJsObject.cpp#newc...
src/GlobalJsObject.cpp:104: global->Set(v8::String::New("_request"), request);
I'd prefer _webRequest for consistency.

http://codereview.adblockplus.org/10259001/diff/1/src/WebRequest.cpp
File src/WebRequest.cpp (right):

http://codereview.adblockplus.org/10259001/diff/1/src/WebRequest.cpp#newcode3
src/WebRequest.cpp:3: AdblockPlus::WebRequest::~WebRequest()
I know I've done it like this all the time, but I think we should define the
virtual destructor of interfaces inline in the future. I'm a fan of keeping
definitions out of header files, but I think this is an exception worth making.
This is one of the refactorings I'm planning to do, actually.

http://codereview.adblockplus.org/10259001/diff/1/src/WebRequestJsObject.cpp
File src/WebRequestJsObject.cpp (right):

http://codereview.adblockplus.org/10259001/diff/1/src/WebRequestJsObject.cpp#...
src/WebRequestJsObject.cpp:34: if (!url.length())
Another refactoring I'm planning to do is to not parse the arguments in
SetTimeoutThread, but in SetTimeoutCallback, as I think that makes things
cleaner. But we can leave it like this or refactor it later if you prefer.

http://codereview.adblockplus.org/10259001/diff/1/src/WebRequestJsObject.cpp#...
src/WebRequestJsObject.cpp:129: v8::FunctionTemplate::New(::GETCallback,
The :: is not necessary here, was just necessary in ConsoleJsObject to avoid
ambiguity with the class called ErrorCallback.

http://codereview.adblockplus.org/10259001/diff/1/src/WebRequestJsObject.h
File src/WebRequestJsObject.h (right):

http://codereview.adblockplus.org/10259001/diff/1/src/WebRequestJsObject.h#ne...
src/WebRequestJsObject.h:6: class WebRequest;
Should be in namespace AdblockPlus.

[Wladimir Palant, 2013-04-11 16:32:32]

All review comments should be addressed now.

http://codereview.adblockplus.org/10259001/diff/1/include/AdblockPlus/WebRequ...
File include/AdblockPlus/WebRequest.h (right):

http://codereview.adblockplus.org/10259001/diff/1/include/AdblockPlus/WebRequ...
include/AdblockPlus/WebRequest.h:2: #define ADBLOCKPLUS_WEB_REQUEST_H
On 2013/04/11 09:33:42, Felix H. Dahlke wrote:
> We usually have ADBLOCK_PLUS (with an underscore) as a prefix for include
> guards.

You better check that again, all include files have ADBLOCKPLUS as prefix.

http://codereview.adblockplus.org/10259001/diff/1/include/AdblockPlus/WebRequ...
include/AdblockPlus/WebRequest.h:9: typedef std::vector< std::pair<std::string,
std::string> > HeadersList;
On 2013/04/11 09:33:42, Felix H. Dahlke wrote:
> We usually only have whitespace between >/< if it'd end up being >>/<<
> otherwise, i.e. not the one after the < here.

I prefer proper grouping as done here. Seeing the other style prevalent in the
adblockplusie repository however I changed this regardless.

http://codereview.adblockplus.org/10259001/diff/1/src/WebRequest.cpp
File src/WebRequest.cpp (right):

http://codereview.adblockplus.org/10259001/diff/1/src/WebRequest.cpp#newcode3
src/WebRequest.cpp:3: AdblockPlus::WebRequest::~WebRequest()
On 2013/04/11 09:33:42, Felix H. Dahlke wrote:
> I know I've done it like this all the time, but I think we should define the
> virtual destructor of interfaces inline in the future. I'm a fan of keeping
> definitions out of header files, but I think this is an exception worth
making.
> This is one of the refactorings I'm planning to do, actually.

As you wish. I'll keep this file around however because I have a follow-up patch
adding to it.

http://codereview.adblockplus.org/10259001/diff/1/src/WebRequestJsObject.cpp
File src/WebRequestJsObject.cpp (right):

http://codereview.adblockplus.org/10259001/diff/1/src/WebRequestJsObject.cpp#...
src/WebRequestJsObject.cpp:34: if (!url.length())
On 2013/04/11 09:33:42, Felix H. Dahlke wrote:
> Another refactoring I'm planning to do is to not parse the arguments in
> SetTimeoutThread, but in SetTimeoutCallback, as I think that makes things
> cleaner. But we can leave it like this or refactor it later if you prefer.

I'll leave it like that for now.

http://codereview.adblockplus.org/10259001/diff/1/src/WebRequestJsObject.cpp#...
src/WebRequestJsObject.cpp:129: v8::FunctionTemplate::New(::GETCallback,
On 2013/04/11 09:33:42, Felix H. Dahlke wrote:
> The :: is not necessary here, was just necessary in ConsoleJsObject to avoid
> ambiguity with the class called ErrorCallback.

:: is actually considered good style to indicate references to global
functions/variables so I'd rather keep it. I know that I didn't use it
consistently however.

[Felix Dahlke, 2013-04-11 17:39:27]

LGTM, I don't mind the include guard since it's already inconsistent.

http://codereview.adblockplus.org/10259001/diff/1/include/AdblockPlus/WebRequ...
File include/AdblockPlus/WebRequest.h (right):

http://codereview.adblockplus.org/10259001/diff/1/include/AdblockPlus/WebRequ...
include/AdblockPlus/WebRequest.h:2: #define ADBLOCKPLUS_WEB_REQUEST_H
On 2013/04/11 16:32:33, Wladimir Palant wrote:
> On 2013/04/11 09:33:42, Felix H. Dahlke wrote:
> > We usually have ADBLOCK_PLUS (with an underscore) as a prefix for include
> > guards.
> 
> You better check that again, all include files have ADBLOCKPLUS as prefix.

No, not really. Just the one in Thread.h for some reason.

[Oleksandr, 2013-04-11 17:53:04]

LGTM for now. I might have some comments a bit later (while working on Windows
implementation)

On 2013/04/11 17:39:27, Felix H. Dahlke wrote:
> LGTM, I don't mind the include guard since it's already inconsistent.
> 
>
http://codereview.adblockplus.org/10259001/diff/1/include/AdblockPlus/WebRequ...
> File include/AdblockPlus/WebRequest.h (right):
> 
>
http://codereview.adblockplus.org/10259001/diff/1/include/AdblockPlus/WebRequ...
> include/AdblockPlus/WebRequest.h:2: #define ADBLOCKPLUS_WEB_REQUEST_H
> On 2013/04/11 16:32:33, Wladimir Palant wrote:
> > On 2013/04/11 09:33:42, Felix H. Dahlke wrote:
> > > We usually have ADBLOCK_PLUS (with an underscore) as a prefix for include
> > > guards.
> > 
> > You better check that again, all include files have ADBLOCKPLUS as prefix.
> 
> No, not really. Just the one in Thread.h for some reason.