----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/#review94105 -----------------------------------------------------------
Looking better! Can you please pull out the fixes to 'acceptsEncoding' into a separate review? Also, would like to see tests for the cases that weren't handled correctly (e.g. "gzipp;q=1.0" should not match "gzip"). Pulling the changes apart helps avoid extra round-trips on reviews (see here: http://mesos.apache.org/documentation/latest/effective-code-reviewing/) :) 3rdparty/libprocess/include/process/http.hpp (lines 119 - 126) <https://reviews.apache.org/r/36402/#comment148611> Let's keep the RFC reference for the implementation, mind moving it back? Also, please remove my TODO for now, we can see how Request evolves over time. 3rdparty/libprocess/src/http.cpp (line 146) <https://reviews.apache.org/r/36402/#comment148609> Why the change here? Should still say 'encoding' right? 3rdparty/libprocess/src/http.cpp (line 155) <https://reviews.apache.org/r/36402/#comment148607> This will crash the program if tokens is empty! 3rdparty/libprocess/src/http.cpp (line 156) <https://reviews.apache.org/r/36402/#comment148620> The style checker doesn't like this 3rdparty/libprocess/src/http.cpp (lines 157 - 159) <https://reviews.apache.org/r/36402/#comment148610> We don't need to use pairs anymore, right? Looks like we should just call tokenize again on 'tokens[1]' (if present). 3rdparty/libprocess/src/http.cpp (line 218) <https://reviews.apache.org/r/36402/#comment148608> This will crash the program if tokens is empty! 3rdparty/libprocess/src/http.cpp (lines 219 - 221) <https://reviews.apache.org/r/36402/#comment148612> Ditto here, can we tokenize 'tokens[1]' instead of using strings::pairs? 3rdparty/libprocess/src/http.cpp (line 235) <https://reviews.apache.org/r/36402/#comment148619> newline here? 3rdparty/libprocess/src/tests/http_tests.cpp (line 693) <https://reviews.apache.org/r/36402/#comment148613> Mind moving this above with the other http tests? 3rdparty/libprocess/src/tests/http_tests.cpp (lines 696 - 703) <https://reviews.apache.org/r/36402/#comment148614> We don't generally wrap things this far, can you just do the following? ``` vector<string> wrongHeaders = { "test,q=0.0", ... }; ``` Ditto below. 3rdparty/libprocess/src/tests/http_tests.cpp (line 706) <https://reviews.apache.org/r/36402/#comment148615> 'auto' here isn't buying us much over just 'const string&', we try to use 'auto' where it helps readability Also please use foreach for now 3rdparty/libprocess/src/tests/http_tests.cpp (lines 709 - 710) <https://reviews.apache.org/r/36402/#comment148618> Ditto here, can you just print 'accept'? 3rdparty/libprocess/src/tests/http_tests.cpp (line 727) <https://reviews.apache.org/r/36402/#comment148616> Ditto here, 'auto' doesn't seem very helpful to readability here. Also, please use foreach for now. 3rdparty/libprocess/src/tests/http_tests.cpp (lines 729 - 731) <https://reviews.apache.org/r/36402/#comment148617> Can you just print out 'accept' rather than grabbing the header and calling .get()? - Ben Mahler On Aug. 4, 2015, 5:42 p.m., Isabel Jimenez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36402/ > ----------------------------------------------------------- > > (Updated Aug. 4, 2015, 5:42 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 b8d9300 > 3rdparty/libprocess/src/http.cpp 4dcbd74 > 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5 > > Diff: https://reviews.apache.org/r/36402/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Isabel Jimenez > >
