> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote: > > 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/) :)
Just added the dependency patch in the description of this RR. > On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/http.cpp, lines 158-160 > > <https://reviews.apache.org/r/36402/diff/6/?file=1028798#file1028798line158> > > > > We don't need to use pairs anymore, right? Looks like we should just > > call tokenize again on 'tokens[1]' (if present). I don't think there's a real difference. Is this change really necessary? > On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/http.cpp, lines 220-222 > > <https://reviews.apache.org/r/36402/diff/6/?file=1028798#file1028798line220> > > > > Ditto here, can we tokenize 'tokens[1]' instead of using strings::pairs? See comment above. > On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/http.cpp, line 236 > > <https://reviews.apache.org/r/36402/diff/6/?file=1028798#file1028798line236> > > > > newline here? Added. > On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/tests/http_tests.cpp, line 693 > > <https://reviews.apache.org/r/36402/diff/6/?file=1028799#file1028799line693> > > > > Mind moving this above with the other http tests? Moved. > On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 696-703 > > <https://reviews.apache.org/r/36402/diff/6/?file=1028799#file1028799line696> > > > > We don't generally wrap things this far, can you just do the following? > > > > ``` > > vector<string> wrongHeaders = { > > "test,q=0.0", > > ... > > }; > > ``` > > > > Ditto below. Changed style. > On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 709-710 > > <https://reviews.apache.org/r/36402/diff/6/?file=1028799#file1028799line709> > > > > Ditto here, can you just print 'accept'? Printing the exact case where it failed may help later to debug in case of failure. > On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/tests/http_tests.cpp, line 706 > > <https://reviews.apache.org/r/36402/diff/6/?file=1028799#file1028799line706> > > > > '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 I don't see why it will not help with readability. - Isabel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/#review94105 ----------------------------------------------------------- On Aug. 4, 2015, 11:05 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, 11:05 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 > >
