-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review92343
-----------------------------------------------------------
Thanks Isabel!
Could we remove the changes to 'acceptEncoding'? The references to the RFC are
more appropriate in the implementation, rather than the API documentation. In
the API documentation, we can refer people to the RFC, and we may want to take
note of places where we're not following the RFC (e.g. missing
'Accept-Encoding' header and curl). But I originally wrote these comments to
guide someone who is trying to understand the implementation. Also, it's
independent :)
How about using candidates (similarly to 'acceptsEncoding')? That would make
both of these pretty similar and easier to understand. Also note that we should
watch out for the existing bug in 'acceptsEncoding':
```
bool Request::acceptsMediaType(const string& mediaType_) const
{
vector<string> mediaType = strings::tokenize(mediaType_, "/");
if (mediaType.size() != 2) {
return false;
}
Option<string> accept = headers.get("Accept");
// If no Accept header field is present, then it is assumed
// that the client accepts all media types.
if (accept.isNone()) {
return true;
}
// Remove spaces and tabs for easier parsing.
accept = strings::remove(accept.get(), " ");
accept = strings::remove(accept.get(), "\t");
accept = strings::remove(accept.get(), "\n");
// First look for the exact media type, followed by
// a type/* match, followed by */*.
vector<string> candidates;
candidates.push_back(mediaType[0] + "/" + mediaType[1]);
candidates.push_back(mediaType[0] + "/*");
candidates.push_back("*/*");
foreach (const string& candidate, candidates) {
// Similar code to 'acceptsEncoding'.
// NOTE that it has a bug because startswith will match
// only by prefix!
}
...
}
```
3rdparty/libprocess/src/http.cpp (lines 122 - 127)
<https://reviews.apache.org/r/36402/#comment146454>
Actually, thinking over this again, I recall that curl doesn't set
Accept-Encoding by default and does not decompress by default, which is pretty
annoying. We should probably leave a NOTE here about this for now, as to why
this returns false.
Also, s/Accept/Accept-Encoding/
3rdparty/libprocess/src/http.cpp (line 142)
<https://reviews.apache.org/r/36402/#comment146471>
Only doing startswith was actually a bug on my part! e.g. gzipper;q=1.0
would match gzip..
3rdparty/libprocess/src/http.cpp (line 168)
<https://reviews.apache.org/r/36402/#comment146456>
What do you mean here by "fully support", can you spell out what part is
not supported? It looks like we don't need to:
"Note that [HTML20] included an optional "level" parameter; in practice,
this parameter was never used and has been removed from this specification.
[HTML30] also suggested a "version" parameter; in practice, this parameter also
was never used and has been removed from this specification."
3rdparty/libprocess/src/http.cpp (line 170)
<https://reviews.apache.org/r/36402/#comment146462>
s/accepted/accept/
3rdparty/libprocess/src/http.cpp (line 183)
<https://reviews.apache.org/r/36402/#comment146464>
s/content/accepted/ or s/content/acceptable/
3rdparty/libprocess/src/http.cpp (lines 185 - 187)
<https://reviews.apache.org/r/36402/#comment146461>
This seems like an malformed 'Accept' header, any reason to prefer
returning false instead of just skipping? Ideally, we've validated already
against malformed requests?
3rdparty/libprocess/src/http.cpp (lines 189 - 190)
<https://reviews.apache.org/r/36402/#comment146466>
Whoops, don't you need to check the size of this? It looks like valid input
needs 2 tokens? Perhaps we should do this near the beginning?
3rdparty/libprocess/src/http.cpp (lines 190 - 199)
<https://reviews.apache.org/r/36402/#comment146465>
Let's get some newlines in here to make it more readable :)
3rdparty/libprocess/src/http.cpp (lines 192 - 193)
<https://reviews.apache.org/r/36402/#comment146467>
Is startswith sufficient? Seems like you need a full match?
Hm.. it doesn't look like `"*/foo"` should be considered valid, but this
code considers `"*/foo"` to match `"bar/foo"`.
3rdparty/libprocess/src/http.cpp (lines 197 - 199)
<https://reviews.apache.org/r/36402/#comment146468>
Hm.. we should split on ';' before, it's not really part of the content
token.
- Ben Mahler
On July 15, 2015, 11:54 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
>
> (Updated July 15, 2015, 11:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and
> Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding a method for Accept header in request + refactor of Accept-Encoding
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 72b6d27
> 3rdparty/libprocess/src/http.cpp d168579
> 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c
>
> Diff: https://reviews.apache.org/r/36402/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
