Implemented curl support for web requests

Implemented curl support for web requests

[Oleksandr, 2013-04-11 16:47:18]

http://codereview.adblockplus.org/10252013/diff/6001/include/AdblockPlus/WebR...
File include/AdblockPlus/WebRequest.h (right):

http://codereview.adblockplus.org/10252013/diff/6001/include/AdblockPlus/WebR...
include/AdblockPlus/WebRequest.h:47: };
Wouldn't it be better to have this class in a separate file? Especially seeing
there will be another class derived from WebRequest for Windows.

[Felix Dahlke, 2013-04-12 03:53:50]

http://codereview.adblockplus.org/10252013/diff/6001/check_curl.py
File check_curl.py (right):

http://codereview.adblockplus.org/10252013/diff/6001/check_curl.py#newcode8
check_curl.py:8: (fd, name) = tempfile.mkstemp(dir=os.path.join(baseDir,
'build'), suffix='.h')
This won't work for a fresh clone, as the build directory wouldn't exist.
Something like this'd do:

build_dir = os.path.join(baseDir, 'build')
if not os.path.exists(build_dir):
  os.makedirs(build_dir)
(fd, name) = tempfile.mkstemp(dir=build_dir, suffix='.h')

http://codereview.adblockplus.org/10252013/diff/6001/include/AdblockPlus/WebR...
File include/AdblockPlus/WebRequest.h (right):

http://codereview.adblockplus.org/10252013/diff/6001/include/AdblockPlus/WebR...
include/AdblockPlus/WebRequest.h:7: enum
I think this should be in the AdblockPlus namespace, maybe even
AdblockPlus::ServerResponse. We shouldn't put much in the global namespace.

http://codereview.adblockplus.org/10252013/diff/6001/include/AdblockPlus/WebR...
include/AdblockPlus/WebRequest.h:47: };
On 2013/04/11 16:47:18, Oleksandr wrote:
> Wouldn't it be better to have this class in a separate file? Especially seeing
> there will be another class derived from WebRequest for Windows.

Agreed.

http://codereview.adblockplus.org/10252013/diff/6001/src/WebRequest.cpp
File src/WebRequest.cpp (right):

http://codereview.adblockplus.org/10252013/diff/6001/src/WebRequest.cpp#newco...
src/WebRequest.cpp:77: // Parse the status code out of something like "HTTP/1.1
200 OK"
I think using std::tr1::regex would be preferable.

http://codereview.adblockplus.org/10252013/diff/6001/src/WebRequest.cpp#newco...
src/WebRequest.cpp:138: curl_slist_append(headerList, (it->first + ": " +
it->second).c_str());
According to the documentation, I think this should be headerList =
curl_slist_append(...), otherwise headerList would always be 0

http://codereview.adblockplus.org/10252013/diff/6001/src/WebRequest.cpp#newco...
src/WebRequest.cpp:149: // Parse header name and value out of something like
"Foo: bar"
I'd vote for std::tr1::regex here as well.

[Wladimir Palant, 2013-04-12 13:39:32]

http://codereview.adblockplus.org/10252013/diff/6001/src/WebRequest.cpp
File src/WebRequest.cpp (right):

http://codereview.adblockplus.org/10252013/diff/6001/src/WebRequest.cpp#newco...
src/WebRequest.cpp:77: // Parse the status code out of something like "HTTP/1.1
200 OK"
On 2013/04/12 03:53:50, Felix H. Dahlke wrote:
> I think using std::tr1::regex would be preferable.

See http://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html#status.iso.tr1 -
gcc didn't implement it. The headers are there but linking fails.

[Felix Dahlke, 2013-04-16 11:53:01]

Sorry, didn't notice the new change set. I'd still like to see DefaultWebRequest
in its own header, otherwise LGTM.

[Oleksandr, 2013-04-16 12:02:01]

http://codereview.adblockplus.org/10252013/diff/14001/include/AdblockPlus/Web...
File include/AdblockPlus/WebRequest.h (right):

http://codereview.adblockplus.org/10252013/diff/14001/include/AdblockPlus/Web...
include/AdblockPlus/WebRequest.h:45: {
I don't quite understand the logic behind this naming here. Wouldn't it make
more sense to have each implementation to have a different name and have clients
just using the interface (ie WebRequest class) to operate. Is there going to be
any other implementation becides "default"?

http://codereview.adblockplus.org/10252013/diff/14001/test/WebRequest.cpp
File test/WebRequest.cpp (right):

http://codereview.adblockplus.org/10252013/diff/14001/test/WebRequest.cpp#new...
test/WebRequest.cpp:70: ASSERT_EQ("text/plain",
jsEngine.Evaluate("foo.responseHeaders['content-type'].substr(0, 10)"));
Do we care if the header is "content-type" or "Content-Type"? Same for all other
headers.

[Felix Dahlke, 2013-04-16 12:21:09]

http://codereview.adblockplus.org/10252013/diff/14001/include/AdblockPlus/Web...
File include/AdblockPlus/WebRequest.h (right):

http://codereview.adblockplus.org/10252013/diff/14001/include/AdblockPlus/Web...
include/AdblockPlus/WebRequest.h:40: virtual inline ~WebRequest() {};
Oh, just noticed, inline is actually not necessary for functions declared inside
a class. I think I said it was at some point, so sorry about that :P

http://codereview.adblockplus.org/10252013/diff/14001/include/AdblockPlus/Web...
include/AdblockPlus/WebRequest.h:45: {
On 2013/04/16 12:02:01, Oleksandr wrote:
> I don't quite understand the logic behind this naming here. Wouldn't it make
> more sense to have each implementation to have a different name and have
clients
> just using the interface (ie WebRequest class) to operate. Is there going to
be
> any other implementation becides "default"? 

Yeah, most likely. We cannot really use curl on Android, nor do I think we can
do it in IE. So it's only used by abpshell (and the tests) at this point, and
only when present.

http://codereview.adblockplus.org/10252013/diff/14001/test/WebRequest.cpp
File test/WebRequest.cpp (right):

http://codereview.adblockplus.org/10252013/diff/14001/test/WebRequest.cpp#new...
test/WebRequest.cpp:70: ASSERT_EQ("text/plain",
jsEngine.Evaluate("foo.responseHeaders['content-type'].substr(0, 10)"));
On 2013/04/16 12:02:01, Oleksandr wrote:
> Do we care if the header is "content-type" or "Content-Type"? Same for all
other
> headers.

HTTP headers are case insensitive, so we can easily make sure they're lowercase.
libcurl seems to do so, but I couldn't see that being documented actually, it's
probably better to make WebRequest convert all header names to lower case.

[Oleksandr, 2013-04-16 12:26:18]

http://codereview.adblockplus.org/10252013/diff/14001/include/AdblockPlus/Web...
File include/AdblockPlus/WebRequest.h (right):

http://codereview.adblockplus.org/10252013/diff/14001/include/AdblockPlus/Web...
include/AdblockPlus/WebRequest.h:45: {
Let me rephrase my question: why do all implementations have to have a name
"DefaultWebRequest"? 

Why not WebRequestCurl, WebRequestStub, WebRequestWinInet etc. Each of which
implements WebRequest.

On 2013/04/16 12:21:09, Felix H. Dahlke wrote:
> On 2013/04/16 12:02:01, Oleksandr wrote:
> > I don't quite understand the logic behind this naming here. Wouldn't it make
> > more sense to have each implementation to have a different name and have
> clients
> > just using the interface (ie WebRequest class) to operate. Is there going to
> be
> > any other implementation becides "default"? 
> 
> Yeah, most likely. We cannot really use curl on Android, nor do I think we can
> do it in IE. So it's only used by abpshell (and the tests) at this point, and
> only when present.

[Felix Dahlke, 2013-04-16 12:31:19]

On 2013/04/16 12:26:18, Oleksandr wrote:
>
http://codereview.adblockplus.org/10252013/diff/14001/include/AdblockPlus/Web...
> File include/AdblockPlus/WebRequest.h (right):
> 
>
http://codereview.adblockplus.org/10252013/diff/14001/include/AdblockPlus/Web...
> include/AdblockPlus/WebRequest.h:45: {
> Let me rephrase my question: why do all implementations have to have a name
> "DefaultWebRequest"? 
> 
> Why not WebRequestCurl, WebRequestStub, WebRequestWinInet etc. Each of which
> implements WebRequest.

Ah, I get it. Well I don't think we want to have concrete implementations in
libadblockplus that aren't useful for either the tests or abpshell. I frankly
wouldn't even have added the one we now have now, but since there's a fallback
when libcurl is not present, I don't mind.

[Wladimir Palant, 2013-04-16 13:30:32]

http://codereview.adblockplus.org/10252013/diff/14001/include/AdblockPlus/Web...
File include/AdblockPlus/WebRequest.h (right):

http://codereview.adblockplus.org/10252013/diff/14001/include/AdblockPlus/Web...
include/AdblockPlus/WebRequest.h:40: virtual inline ~WebRequest() {};
On 2013/04/16 12:21:09, Felix H. Dahlke wrote:
> Oh, just noticed, inline is actually not necessary for functions declared
inside
> a class. I think I said it was at some point, so sorry about that :P

Let's remove it everywhere then later, not only in this patch.

http://codereview.adblockplus.org/10252013/diff/14001/include/AdblockPlus/Web...
include/AdblockPlus/WebRequest.h:45: {
The thinking behind this:

1) We have clear preferences for the WebRequest implementation: WinHttp on
Win32, curl on Linux and OS X, Java URL on Android.

2) We don't want to compile more than one WebRequest implementation per platform
even if we could. Meaning for example: on Windows we are always going to use
WinHttp and ignore curl completely.

3) If we use different class names for all implementations then we move the
selection logic into the consumers: these will need to determine which
implementation they want to use. We will have to duplicate the selection rules
in each consumer. Alternatively we can use a factory method but that also
duplicates the logic we will need in the build config anyway.

My thought was to make it easy for WebRequest consumers while keeping things
flexible: they can just use DefaultWebRequest and forget about it or they can
use a custom implementation (e.g. for tests). The rules determining which
WebRequest implementation gets compiled and consequently goes by the name
DefaultWebRequest are centralized in the build configuration.

http://codereview.adblockplus.org/10252013/diff/14001/test/WebRequest.cpp
File test/WebRequest.cpp (right):

http://codereview.adblockplus.org/10252013/diff/14001/test/WebRequest.cpp#new...
test/WebRequest.cpp:70: ASSERT_EQ("text/plain",
jsEngine.Evaluate("foo.responseHeaders['content-type'].substr(0, 10)"));
I don't think that curl lowercases header names - but DefaultWebRequestCurl.cpp
does:

  std::transform(name.begin(), name.end(), name.begin(), ::tolower);

XMLHttpRequest.getResponseHeader() also lowercases the headers when looking them
up, this makes sure that they are actually treated as case-insensitive.

[Felix Dahlke, 2013-04-16 14:51:37]

http://codereview.adblockplus.org/10252013/diff/14001/include/AdblockPlus/Web...
File include/AdblockPlus/WebRequest.h (right):

http://codereview.adblockplus.org/10252013/diff/14001/include/AdblockPlus/Web...
include/AdblockPlus/WebRequest.h:40: virtual inline ~WebRequest() {};
On 2013/04/16 13:30:32, Wladimir Palant wrote:
> On 2013/04/16 12:21:09, Felix H. Dahlke wrote:
> > Oh, just noticed, inline is actually not necessary for functions declared
> inside
> > a class. I think I said it was at some point, so sorry about that :P
> 
> Let's remove it everywhere then later, not only in this patch.

Okay.

http://codereview.adblockplus.org/10252013/diff/14001/test/WebRequest.cpp
File test/WebRequest.cpp (right):

http://codereview.adblockplus.org/10252013/diff/14001/test/WebRequest.cpp#new...
test/WebRequest.cpp:70: ASSERT_EQ("text/plain",
jsEngine.Evaluate("foo.responseHeaders['content-type'].substr(0, 10)"));
On 2013/04/16 13:30:32, Wladimir Palant wrote:
> I don't think that curl lowercases header names - but
DefaultWebRequestCurl.cpp
> does:
> 
>   std::transform(name.begin(), name.end(), name.begin(), ::tolower);

Hm, seems I missed that then.

> XMLHttpRequest.getResponseHeader() also lowercases the headers when looking
them
> up, this makes sure that they are actually treated as case-insensitive.

It only lower cases the header name in the argument though. I think it's
easiest/safest if we just lower case them in XMLHttpRequest or
WebRequestJsObject, then it won't matter what the implementation does.

[Oleksandr, 2013-04-16 14:59:07]

LGTM

[Wladimir Palant, 2013-04-17 06:56:20]

http://codereview.adblockplus.org/10252013/diff/14001/test/WebRequest.cpp
File test/WebRequest.cpp (right):

http://codereview.adblockplus.org/10252013/diff/14001/test/WebRequest.cpp#new...
test/WebRequest.cpp:70: ASSERT_EQ("text/plain",
jsEngine.Evaluate("foo.responseHeaders['content-type'].substr(0, 10)"));
On 2013/04/16 14:51:37, Felix H. Dahlke wrote:
> It only lower cases the header name in the argument though. I think it's
> easiest/safest if we just lower case them in XMLHttpRequest or
> WebRequestJsObject, then it won't matter what the implementation does.

I don't quite get this suggestion, WebRequestJsObject works with plain JS
objects for headers - it cannot impose any semantics on them (and it's an
internal implementation detail anyway, real code should always use
XMLHttpRequest). XMLHttpRequest on the other hand can ensure that headers are
treated in a case-insensitive way (per spec).

[Felix Dahlke, 2013-04-17 07:06:15]

On 2013/04/17 06:56:20, Wladimir Palant wrote:
> http://codereview.adblockplus.org/10252013/diff/14001/test/WebRequest.cpp
> File test/WebRequest.cpp (right):
> 
>
http://codereview.adblockplus.org/10252013/diff/14001/test/WebRequest.cpp#new...
> test/WebRequest.cpp:70: ASSERT_EQ("text/plain",
> jsEngine.Evaluate("foo.responseHeaders['content-type'].substr(0, 10)"));
> On 2013/04/16 14:51:37, Felix H. Dahlke wrote:
> > It only lower cases the header name in the argument though. I think it's
> > easiest/safest if we just lower case them in XMLHttpRequest or
> > WebRequestJsObject, then it won't matter what the implementation does.
> 
> I don't quite get this suggestion, WebRequestJsObject works with plain JS
> objects for headers - it cannot impose any semantics on them (and it's an
> internal implementation detail anyway, real code should always use
> XMLHttpRequest). XMLHttpRequest on the other hand can ensure that headers are
> treated in a case-insensitive way (per spec).

I'm just saying that we might want to make sure the headers are lower case,
either in WebRequestJsObject or our XMLHttpRequest implementation.

It would result in very subtle errors if an implementation of WebRequest does
not make sure that all header names are lower case. Then again, it's not really
our problem, as long as we specify that they have to be lower case.

LGTM anyway