Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1431723002:
Add brotli content-encoding filter. (Closed)
|
Created:
5 years, 1 month ago by eustas Modified:
4 years, 11 months ago Reviewers:
cbentzel, rkaplow, Ryan Sleevi, jam, agrieve, Randy Smith (Not in Mondays), Nico, xunjieli CC:
chromium-reviews, cbentzel+watch_chromium.org, jschuh Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd brotli content-encoding filter.
BUG=472009
Committed: https://crrev.com/fbec913fc94fbf68bfbb3e9d46449d2cc8b31e10
Cr-Commit-Position: refs/heads/master@{#367153}
Patch Set 1 #Patch Set 2 : Added unittest. #Patch Set 3 : Fix gypi file #Patch Set 4 : Added runtime flag #Patch Set 5 : Fix deps #Patch Set 6 : Fix histograms.xml #
Total comments: 2
Patch Set 7 : Add brotli reference link #
Total comments: 12
Patch Set 8 : #Patch Set 9 : Added Accept-Ecnoding test #
Total comments: 2
Patch Set 10 : Minimize ifdefs. Revert FilterContext changes #Patch Set 11 : Added BrotliSlowRead to u_r_j_unittest #
Total comments: 44
Patch Set 12 : Address comments #
Total comments: 29
Patch Set 13 : Fixed nits, removed hack #Patch Set 14 : Fixed nit #
Total comments: 14
Patch Set 15 : Addressed comments. Use SchemeIsCryptographic #
Total comments: 2
Patch Set 16 : Use features instead of command line flags #
Total comments: 8
Patch Set 17 : Fixed nits #
Total comments: 19
Patch Set 18 : Addressed comments #Patch Set 19 : Removed build flag #Patch Set 20 : #Patch Set 21 : Rebaselined #Patch Set 22 : Cleanup build files #Patch Set 23 : Rebaseline #
Total comments: 20
Patch Set 24 : Addressed comments, moved feature declatation to content #
Total comments: 4
Patch Set 25 : More safe conversions #Patch Set 26 : Fixed path in net.gyp #Patch Set 27 : Exclude brotli_filter_unittest on ios - needs to read input data files #Messages
Total messages: 93 (19 generated)
eustas@chromium.org changed reviewers: + cbentzel@chromium.org
On 2015/11/09 11:52:13, eustas wrote: We chatted over IM, but I think this is going to want a mix of compile-time flags (which exist) as well as run-time flags for Chrome (which you talked about doing in io_thread.cc and passing into HttpNetworkSesssion).
eustas@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow@chromium.org: Please review changes in histogram.xml
On 2015/11/09 16:22:10, cbentzel wrote: > On 2015/11/09 11:52:13, eustas wrote: > > We chatted over IM, but I think this is going to want a mix of compile-time > flags (which exist) as well as run-time flags for Chrome (which you talked about > doing in io_thread.cc and passing into HttpNetworkSesssion). Added runtime flags (enable-/disable-brotli).
lgtm histograms lgtm
On 2015/11/10 18:48:02, rkaplow wrote: > lgtm > > histograms lgtm eustas: Is this ready for full review?
On 2015/11/11 04:50:33, cbentzel wrote: > On 2015/11/10 18:48:02, rkaplow wrote: > > lgtm > > > > histograms lgtm > > eustas: Is this ready for full review? I think, yes.
OK, I may be a little delayed this week. I'll try to see if there's some top-level issues but may not have time to get to the details this week. On Tue, Nov 10, 2015 at 10:27 PM <eustas@chromium.org> wrote: > On 2015/11/11 04:50:33, cbentzel wrote: > > On 2015/11/10 18:48:02, rkaplow wrote: > > > lgtm > > > > > > histograms lgtm > > > eustas: Is this ready for full review? > > I think, yes. > > https://codereview.chromium.org/1431723002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/11/11 17:48:12, cbentzel wrote: > OK, I may be a little delayed this week. I'll try to see if there's some > top-level issues but may not have time to get to the details this week. > > On Tue, Nov 10, 2015 at 10:27 PM <mailto:eustas@chromium.org> wrote: > > > On 2015/11/11 04:50:33, cbentzel wrote: > > > On 2015/11/10 18:48:02, rkaplow wrote: > > > > lgtm > > > > > > > > histograms lgtm > > > > > eustas: Is this ready for full review? > > > > I think, yes. > > > > https://codereview.chromium.org/1431723002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. At a high-level seems fine. Hope to have time to review on my flight later today. If not I will look at this early next week.
cbentzel@chromium.org changed reviewers: + xunjieli@chromium.org
Helen - could you take a look at this? The actual filter code is pretty small but I think you have been taking over some of the refactor work. Also want to make sure the way this disables Brotli by default in Cronet makes sense. https://codereview.chromium.org/1431723002/diff/100001/net/filter/brotli_filt... File net/filter/brotli_filter.h (right): https://codereview.chromium.org/1431723002/diff/100001/net/filter/brotli_filt... net/filter/brotli_filter.h:6: // stream. Can you include a reference to Brotli here?
https://codereview.chromium.org/1431723002/diff/100001/net/filter/brotli_filt... File net/filter/brotli_filter.h (right): https://codereview.chromium.org/1431723002/diff/100001/net/filter/brotli_filt... net/filter/brotli_filter.h:6: // stream. On 2015/11/14 01:20:10, cbentzel wrote: > Can you include a reference to Brotli here? Done.
Thanks, Chris. I will take a look later today or tomorrow.
Description was changed from ========== Add brotli content-encoding filter. BUG=472009 ========== to ========== Add brotli content-encoding filter. BUG=472009 ==========
xunjieli@chromium.org changed reviewers: + rdsmith@chromium.org
Haven't looked very closely. But the brotli filter does have a sync decompressor, so I don't imagine it will be too hard to change it to the new pull based design once that is ready. https://codereview.chromium.org/1431723002/diff/120001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1431723002/diff/120001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1304: return command_line.HasSwitch(switches::kEnableBrotli); Why do we have both the enable and disable switches? https://codereview.chromium.org/1431723002/diff/120001/chrome/common/chrome_s... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1431723002/diff/120001/chrome/common/chrome_s... chrome/common/chrome_switches.cc:1321: // Enables support for the Brotli Content-encoding. nit: s/Content-encoding/Content-Encoding same for line 1324. https://codereview.chromium.org/1431723002/diff/120001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/1431723002/diff/120001/components/cronet.gypi... components/cronet.gypi:7: 'DISABLE_BROTLI_SUPPORT=1', This should be moved to line 165. https://codereview.chromium.org/1431723002/diff/120001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1431723002/diff/120001/net/BUILD.gn#newcode1587 net/BUILD.gn:1587: "filter/brotli_filter_unittest.cc", The test file is already excluded on 1508. Isn't this not needed? https://codereview.chromium.org/1431723002/diff/120001/net/filter/brotli_filt... File net/filter/brotli_filter.h (right): https://codereview.chromium.org/1431723002/diff/120001/net/filter/brotli_filt... net/filter/brotli_filter.h:41: // stream_buffer_. On the other hand, *dest_len can be 0 upon successful nit: Please surround the variables using pipes. e.g. |stream_buffer_| and |*dest_len| https://codereview.chromium.org/1431723002/diff/120001/net/http/http_network_... File net/http/http_network_session.h (right): https://codereview.chromium.org/1431723002/diff/120001/net/http/http_network_... net/http/http_network_session.h:117: // Enables Brotli Content-Encoding support nit: Need a period at the end.
https://codereview.chromium.org/1431723002/diff/120001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1431723002/diff/120001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1304: return command_line.HasSwitch(switches::kEnableBrotli); On 2015/11/17 16:16:12, xunjieli wrote: > Why do we have both the enable and disable switches? This makes rolling out feature easier. User have combo-box in about:flags UI with 3 choices - default / enable / disable. On the first stage, feature is disabled by default and experienced users can enable it. On the later stages this leaves user ability to disable the feature (when it will be enabled by default). https://codereview.chromium.org/1431723002/diff/120001/chrome/common/chrome_s... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1431723002/diff/120001/chrome/common/chrome_s... chrome/common/chrome_switches.cc:1321: // Enables support for the Brotli Content-encoding. On 2015/11/17 16:16:12, xunjieli wrote: > nit: s/Content-encoding/Content-Encoding > same for line 1324. Done. https://codereview.chromium.org/1431723002/diff/120001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/1431723002/diff/120001/components/cronet.gypi... components/cronet.gypi:7: 'DISABLE_BROTLI_SUPPORT=1', On 2015/11/17 16:16:12, xunjieli wrote: > This should be moved to line 165. Done. https://codereview.chromium.org/1431723002/diff/120001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1431723002/diff/120001/net/BUILD.gn#newcode1587 net/BUILD.gn:1587: "filter/brotli_filter_unittest.cc", On 2015/11/17 16:16:12, xunjieli wrote: > The test file is already excluded on 1508. Isn't this not needed? Right you are. Fixed. https://codereview.chromium.org/1431723002/diff/120001/net/filter/brotli_filt... File net/filter/brotli_filter.h (right): https://codereview.chromium.org/1431723002/diff/120001/net/filter/brotli_filt... net/filter/brotli_filter.h:41: // stream_buffer_. On the other hand, *dest_len can be 0 upon successful On 2015/11/17 16:16:12, xunjieli wrote: > nit: Please surround the variables using pipes. > e.g. |stream_buffer_| and |*dest_len| Surely. Done. https://codereview.chromium.org/1431723002/diff/120001/net/http/http_network_... File net/http/http_network_session.h (right): https://codereview.chromium.org/1431723002/diff/120001/net/http/http_network_... net/http/http_network_session.h:117: // Enables Brotli Content-Encoding support On 2015/11/17 16:16:12, xunjieli wrote: > nit: Need a period at the end. Fixed.
Sorry for the delay. I was hoping Randy could take a look first, but he'll be out until after thanksgiving break. I think the CL looks pretty good, but I don't really know the filters well. Could you write a unit test in net/url_request/url_request_job_unittest.cc to make sure the filter works w.r.t the state machines of url request job? There's a gzip test which you can use as an example. You can use TEST_MODE_SLOW_READ to make the server return one byte at a time to trigger BROTLI_RESULT_NEEDS_MORE_INPUT, and make sure url_request_job.cc handles that appropriately.
Quick set of high level questions: * Why are we not enabled on IOS? * Is there any way we can do less compile time #ifdefs? In my ideal world, we wouldn't have any outside of brotli_filter.cc. The filter code's already got too many conditionals on the different filter implementations; I'd rather not adds compile time conditionalization as well. * Is there a need for the IsBrotliEnabled() on the filter context? The filter context is a grab bag that I'd like to avoid adding more things to. The request won't advertise brotli if it's disabled; if the server sends brotli content-encoding, can we take advantage of the path that unknown encodings normally take to fail the request? https://codereview.chromium.org/1431723002/diff/160001/net/filter/filter.cc File net/filter/filter.cc (right): https://codereview.chromium.org/1431723002/diff/160001/net/filter/filter.cc#n... net/filter/filter.cc:44: const char kBrotli[] = "br"; I'd rather not use an abbreviation; can we say "brotili" instead?
1) iOS: initial intention was to enable brotli on platforms that support WOFF2 first. This way it will be nearly 0 footprint. But we will be happy to have brotli on iOS, if there are no objections. 2) ifdefs: they also have been added to make brotli have nearly-zero footprint on systems that doesn't support it (e.g. cronet). I can move all of them into brotli_filter, but in this case we have to add brotli_filter_impl to cut the dependencies... 3) To fail request on our side filter system needs to know that brotli is disabled. If we don't do so, we implicitly encourage developers to send brotli encoded content based on useragent that is completely wrong thing. If we would have brotli enabled only by compile flags, without runtime flags, it would be much simpler - no need to push and pull runtime flags to network stack.
https://codereview.chromium.org/1431723002/diff/160001/net/filter/filter.cc File net/filter/filter.cc (right): https://codereview.chromium.org/1431723002/diff/160001/net/filter/filter.cc#n... net/filter/filter.cc:44: const char kBrotli[] = "br"; On 2015/11/28 22:15:24, rdsmith wrote: > I'd rather not use an abbreviation; can we say "brotili" instead? This constant is used in ConvertEncodingToType used by URLRequestHttpJob::SetupFilter Brotli is associated with "br" in IANA (see section 12 https://datatracker.ietf.org/doc/draft-alakuijala-brotli/?include_text=1). Firefox already supports "br" accept/content-encoding.
On 2015/11/30 10:18:36, eustas wrote: > 1) iOS: initial intention was to enable brotli on platforms that support WOFF2 > first. This way it will be nearly 0 footprint. But we will be happy to have > brotli on iOS, if there are no objections. > 2) ifdefs: they also have been added to make brotli have nearly-zero footprint > on systems that doesn't support it (e.g. cronet). I can move all of them into > brotli_filter, but in this case we have to add brotli_filter_impl to cut the > dependencies... Ah, gotcha. When I do my detailed review, I'll see if I can come up with some ways to deal with zero-footprint with fewer compile time dependencies; I do think it's worth minimizing them. I'm not the right person to say that it's ok to enable on iOS if it's going to cost real memory. > 3) To fail request on our side filter system needs to know that brotli is > disabled. If we don't do so, we implicitly encourage developers to send brotli > encoded content based on useragent that is completely wrong thing. If we would > have brotli enabled only by compile flags, without runtime flags, it would be > much simpler - no need to push and pull runtime flags to network stack. Servers should not be sending content encoded with an encoding method that the client didn't advertise through Accept-Encoding; if they use the user agent to decide on the encoding, I'd consider that a (fairly large) bug on their part. I feel as if recognizing Brotli encoding when the client did not send the appropriate Accept-Encoding: header is what would encourage developers to do the completely wrong thing. What am I missing?
PTAL
On 2015/12/01 18:08:03, eustas wrote: > PTAL FYI. I am going to hold off the review on my part until you add some end-to-end test in url_request_job_unittest.cc per my previous comment. "Could you write a unit test in net/url_request/url_request_job_unittest.cc to make sure the filter works w.r.t the state machines of url request job? There's a gzip test which you can use as an example. You can use TEST_MODE_SLOW_READ to make the server return one byte at a time to trigger BROTLI_RESULT_NEEDS_MORE_INPUT, and make sure url_request_job.cc handles that appropriately."
On 2015/12/01 22:43:15, xunjieli wrote: > On 2015/12/01 18:08:03, eustas wrote: > > PTAL > > FYI. I am going to hold off the review on my part until you add some end-to-end > test in url_request_job_unittest.cc per my previous comment. > > "Could you write a unit test in net/url_request/url_request_job_unittest.cc to > make sure the filter works w.r.t the state machines of url request job? There's > a gzip test which you can use as an example. You can use TEST_MODE_SLOW_READ to > make the server return one byte at a time to trigger > BROTLI_RESULT_NEEDS_MORE_INPUT, and make sure url_request_job.cc handles that > appropriately." Addressed
A few quick comments. I will have time to do a fuller this week. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... File net/filter/brotli_filter.cc (right): https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter.cc:22: BrotliFilter(FilterType type,int buffer_size) nit: can we have constructor before destructor? https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter.cc:35: if (!brotli_state_.get()) This if-block looks weird. If |brotli_state_| is set before the if-block, when can this if-statement be true? Can we get rid of this if-block? https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter.cc:99: next_stream_data_ = NULL; nit: nullptr here. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter.cc:132: DISALLOW_COPY_AND_ASSIGN(BrotliFilter); nit: need to include base/macro.h for DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter.cc:140: return brotli_filter->InitDecoding() ? brotli_filter.release() : NULL; nit: nullptr this line and on the next line. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... File net/filter/brotli_filter.h (right): https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter.h:15: #include "net/base/net_export.h" nit: this is not used. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... File net/filter/brotli_filter_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:6: #include <ostream> Why do we need these? https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:47: ASSERT_TRUE(encoded_buffer_.size() <= kDefaultBufferSize); nit: ASSERT_LE. ASSERT_GT(kDefaultBufferSize, encoded_buffer_.size()); is better since we want to put the expected value first. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:52: // Use filter to decode compressed data, and compare the decoding result with nit: s/decoding/decoded https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:53: // the orginal Data. nit: s/Data/data. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:54: // Parameters: Source and source_len are original data and its size. nit: s/Source/|source| s/source_len/|source_len| Same goes to the other variables in this comment. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:65: ASSERT_TRUE(source_len <= kDefaultBufferSize); ditto https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:66: ASSERT_TRUE(output_buffer_size <= kDefaultBufferSize); ditto https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:85: while (1) { nit: while (true) https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:92: ASSERT_TRUE(code != Filter::FILTER_ERROR); nit: ASSERT_NE https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:103: EXPECT_TRUE(decode_total_data_len == source_len); EXPECT_EQ https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:164: EXPECT_TRUE(decode_size == source_len()); EXPECT_EQ https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:224: EXPECT_TRUE(code == Filter::FILTER_ERROR); nit:EXPECT_EQ https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:249: } // namespace net nit: a blank line before namespace https://codereview.chromium.org/1431723002/diff/200001/net/http/http_transact... File net/http/http_transaction_test_util.h (right): https://codereview.chromium.org/1431723002/diff/200001/net/http/http_transact... net/http/http_transaction_test_util.h:323: void set_network_session(HttpNetworkSession* session) { session_ = session; } I don't think we want to do this, see the comment in the test for an alternative. https://codereview.chromium.org/1431723002/diff/200001/net/url_request/url_re... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/200001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:109: network_layer_.set_network_session(network_session_.get()); These two tests should really follow other URLRequestHttpJobWithMockSocketsTest examples. You can then set the HttpNetworkSession::Params on the TestURLRequestContext. I don't think we want to set a HttpNetworkSession on the MockNetworkLayer.
On 2015/12/02 18:26:31, xunjieli wrote: > A few quick comments. I will have time to do a fuller this week. Sorry, I meant to say that I would have time to do a fuller review later this week.
https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... File net/filter/brotli_filter.cc (right): https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter.cc:22: BrotliFilter(FilterType type,int buffer_size) On 2015/12/02 18:26:30, xunjieli wrote: > nit: can we have constructor before destructor? Of course. Done. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter.cc:35: if (!brotli_state_.get()) On 2015/12/02 18:26:30, xunjieli wrote: > This if-block looks weird. If |brotli_state_| is set before the if-block, when > can this if-statement be true? Can we get rid of this if-block? C-style allocation check. Removed. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter.cc:99: next_stream_data_ = NULL; On 2015/12/02 18:26:30, xunjieli wrote: > nit: nullptr here. Done. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter.cc:132: DISALLOW_COPY_AND_ASSIGN(BrotliFilter); On 2015/12/02 18:26:30, xunjieli wrote: > nit: need to include base/macro.h for DISALLOW_COPY_AND_ASSIGN Done. Thank you. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter.cc:140: return brotli_filter->InitDecoding() ? brotli_filter.release() : NULL; On 2015/12/02 18:26:30, xunjieli wrote: > nit: nullptr this line and on the next line. Fixed everywhere. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... File net/filter/brotli_filter.h (right): https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter.h:15: #include "net/base/net_export.h" On 2015/12/02 18:26:30, xunjieli wrote: > nit: this is not used. Ooops, removed. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... File net/filter/brotli_filter_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/12/02 18:26:31, xunjieli wrote: > nit: 2015. Done. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:6: #include <ostream> On 2015/12/02 18:26:31, xunjieli wrote: > Why do we need these? Copy-pasted from gzip filter unittest... Removed. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:47: ASSERT_TRUE(encoded_buffer_.size() <= kDefaultBufferSize); On 2015/12/02 18:26:31, xunjieli wrote: > nit: ASSERT_LE. > > ASSERT_GT(kDefaultBufferSize, encoded_buffer_.size()); is better since we want > to put the expected value first. Done. Thank you. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:52: // Use filter to decode compressed data, and compare the decoding result with On 2015/12/02 18:26:31, xunjieli wrote: > nit: s/decoding/decoded Done. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:53: // the orginal Data. On 2015/12/02 18:26:31, xunjieli wrote: > nit: s/Data/data. Done. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:54: // Parameters: Source and source_len are original data and its size. On 2015/12/02 18:26:31, xunjieli wrote: > nit: s/Source/|source| > s/source_len/|source_len| > Same goes to the other variables in this comment. Done. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:65: ASSERT_TRUE(source_len <= kDefaultBufferSize); On 2015/12/02 18:26:31, xunjieli wrote: > ditto Done. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:66: ASSERT_TRUE(output_buffer_size <= kDefaultBufferSize); On 2015/12/02 18:26:31, xunjieli wrote: > ditto Done. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:85: while (1) { On 2015/12/02 18:26:30, xunjieli wrote: > nit: while (true) Sorry. Fixed. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:92: ASSERT_TRUE(code != Filter::FILTER_ERROR); On 2015/12/02 18:26:31, xunjieli wrote: > nit: ASSERT_NE Done. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:103: EXPECT_TRUE(decode_total_data_len == source_len); On 2015/12/02 18:26:31, xunjieli wrote: > EXPECT_EQ Done. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:164: EXPECT_TRUE(decode_size == source_len()); On 2015/12/02 18:26:31, xunjieli wrote: > EXPECT_EQ Done. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:224: EXPECT_TRUE(code == Filter::FILTER_ERROR); On 2015/12/02 18:26:31, xunjieli wrote: > nit:EXPECT_EQ Done. https://codereview.chromium.org/1431723002/diff/200001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:249: } // namespace net On 2015/12/02 18:26:31, xunjieli wrote: > nit: a blank line before namespace Fixed https://codereview.chromium.org/1431723002/diff/200001/net/http/http_transact... File net/http/http_transaction_test_util.h (right): https://codereview.chromium.org/1431723002/diff/200001/net/http/http_transact... net/http/http_transaction_test_util.h:323: void set_network_session(HttpNetworkSession* session) { session_ = session; } On 2015/12/02 18:26:31, xunjieli wrote: > I don't think we want to do this, see the comment in the test for an > alternative. Acknowledged. https://codereview.chromium.org/1431723002/diff/200001/net/url_request/url_re... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/200001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:109: network_layer_.set_network_session(network_session_.get()); On 2015/12/02 18:26:31, xunjieli wrote: > These two tests should really follow other URLRequestHttpJobWithMockSocketsTest > examples. > You can then set the HttpNetworkSession::Params on the TestURLRequestContext. I > don't think we want to set a HttpNetworkSession on the MockNetworkLayer. Nice idea. Done!
I think this looks good. I just have a couple of nits and one test suggestion. https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... File net/filter/brotli_filter_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:104: // Parameters: Source and source_len are compressed data and its size. nit: need to update variables in this comment block. https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:143: std::string encoded_buffer_; nit: These two variables should be private, since there are already accessors for them. https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job.cc:612: // Tell the server what compression formats we support. nit: avoid using first person pronouns. Maybe change to "Tell the server what compression formats are supported." https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:650: return AcceptsEncoding(network_delegate_.last_headers(), "br"); Let's not add "last_headers" to TestNetworkDelegate. The test class has too many internal states, and we shouldn't add one unless it's absolutely necessary. I'd suggest an alternative using mock sockets data. In "URLRequestHttpJobWithMockSocketsTest.TestContentLengthSuccessfulRequest", notice that they are using MockWrite and MockRead. We could do something similar. In your "BrotliAdvertisement", we just need to do a mock write with "Accept-Encoding: gzip, deflate, br". The test will then magically make sure that the headers we are writing matches the mock writes. https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:653: void RunRequest(GURL url) { nit: const GURL& https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:664: scoped_ptr<TestURLRequestContext> context_; nit: add "private" before these variables.
First of all, thank you for the changes around #ifdefs and the FilterContext accessor, and my apologies for taking so long to get back to you--I've been swamped this week. This looks pretty good--I have some small points for clarification below, but nothing major. I haven't done a test pass yet, but I wanted to give you some feedback later instead of much later :-}. I'll try and do the test pass on Monday. https://codereview.chromium.org/1431723002/diff/220001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1431723002/diff/220001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1318: return command_line.HasSwitch(switches::kEnableBrotli); If I read this correctly, Brotli will be compiled in on non-iOS platforms, disabled by default, and enabled only via flags or command lines. Is your plan to test with it manually for a bit before doing a finch experiment/default enable? https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... File net/filter/brotli_filter.cc (right): https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... net/filter/brotli_filter.cc:54: FilterStatus ReadFilteredData(char* dest_buffer, int* dest_len) override { When we did the initial review (a long time ago :-}) I remember that it would be useful for Brotli to know when EOF was reached on the input. What's the status of that? Is it waiting on Helen's filter refactor? If so, just for my curiosity, where would the information be used in this code? https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... net/filter/brotli_filter.cc:70: size_t total_out; According to the header file, this value must be initialized to zero? I think I'd like that, even if it isn't going to be used; uninitialized values disturb me. https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... net/filter/brotli_filter.cc:72: BrotliDecompressStream(&available_in, &next_in, &available_out, The header file says "In the current implementation, the function requires that there is enough output buffer size to write all currently processed input, so *available_out must be large enough. " While it doesn't specify how large "large enough" is, I don't see anything confirming that *dest_len is greater than any given size or modifying processing if it is. Is this a bug? https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... net/filter/brotli_filter.cc:93: case BROTLI_RESULT_NEEDS_MORE_INPUT: { How does the underlying Brotli decoding respond if both needs more input and needs more output are true (i.e. its reached the end of the input available to it, and completely filled the output available to it)? https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job.cc:598: if (network_session_params && network_session_params->enable_brotli) { nit: No curly braces needed if both conditional and statement are single line (and its more consistent with surrounding code not to have them). https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job.cc:599: advertise_brotli = !request()->url().SchemeIs(url::kHttpScheme); I *think* you want SchemeIsCryptographic(); that's what I've seen elsewhere for this type of test. If that doesn't look right to you, push back and I'll dig up someone who'll know for sure.
https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... File net/filter/brotli_filter_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:104: // Parameters: Source and source_len are compressed data and its size. On 2015/12/04 15:21:19, xunjieli wrote: > nit: need to update variables in this comment block. Done. https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:143: std::string encoded_buffer_; On 2015/12/04 15:21:19, xunjieli wrote: > nit: These two variables should be private, since there are already accessors > for them. Surely. Done. https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job.cc:612: // Tell the server what compression formats we support. On 2015/12/04 15:21:19, xunjieli wrote: > nit: avoid using first person pronouns. Maybe change to "Tell the server what > compression formats are supported." Nice! Fixed. https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:650: return AcceptsEncoding(network_delegate_.last_headers(), "br"); On 2015/12/04 15:21:19, xunjieli wrote: > Let's not add "last_headers" to TestNetworkDelegate. The test class has > too many internal states, and we shouldn't add one unless it's absolutely > necessary. > > I'd suggest an alternative using mock sockets data. > In "URLRequestHttpJobWithMockSocketsTest.TestContentLengthSuccessfulRequest", > notice that they are using MockWrite and MockRead. We could do something > similar. In your "BrotliAdvertisement", we just need to do a mock write with > "Accept-Encoding: gzip, deflate, br". The test will then magically make sure > that the headers we are writing matches the mock writes. > Great idea! Thank you. Done. https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:653: void RunRequest(GURL url) { On 2015/12/04 15:21:19, xunjieli wrote: > nit: const GURL& Done. https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:664: scoped_ptr<TestURLRequestContext> context_; On 2015/12/04 15:21:19, xunjieli wrote: > nit: add "private" before these variables. Done.
https://codereview.chromium.org/1431723002/diff/220001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1431723002/diff/220001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1318: return command_line.HasSwitch(switches::kEnableBrotli); On 2015/12/04 16:07:52, rdsmith wrote: > If I read this correctly, Brotli will be compiled in on non-iOS platforms, > disabled by default, and enabled only via flags or command lines. Is your plan > to test with it manually for a bit before doing a finch experiment/default > enable? I think we can actually have brotli enabled by default soon (there are not many servers that can serve brotli encoded data). I've chosen disabled-by-default to make this patch more clear (otherwise it would require adding "br" to Accept-Encoding headers in integration tests). https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... File net/filter/brotli_filter.cc (right): https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... net/filter/brotli_filter.cc:54: FilterStatus ReadFilteredData(char* dest_buffer, int* dest_len) override { On 2015/12/04 16:07:52, rdsmith wrote: > When we did the initial review (a long time ago :-}) I remember that it would be > useful for Brotli to know when EOF was reached on the input. What's the status > of that? Is it waiting on Helen's filter refactor? If so, just for my > curiosity, where would the information be used in this code? We've modified decoder to have weaker requirements... Now it doesn't need to know about EOF and pushes to output all the data it has decoded so far. So, we do not depend on Helen's refactoring now... https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... net/filter/brotli_filter.cc:70: size_t total_out; On 2015/12/04 16:07:52, rdsmith wrote: > According to the header file, this value must be initialized to zero? I think > I'd like that, even if it isn't going to be used; uninitialized values disturb > me. No problem. (Ooops, headers are outdated now... currently there is no such requirement, value is overwritten based on decoder internal data). https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... net/filter/brotli_filter.cc:72: BrotliDecompressStream(&available_in, &next_in, &available_out, On 2015/12/04 16:07:52, rdsmith wrote: > The header file says > > "In the current implementation, the function requires that there is enough > output buffer size to write all currently processed input, so > *available_out must be large enough. " > > While it doesn't specify how large "large enough" is, I don't see anything > confirming that *dest_len is greater than any given size or modifying processing > if it is. Is this a bug? This comment is outdated as well. I think it is for pre-March'15 version. Output as small as 1 byte is enough (and we have tests for that). https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... net/filter/brotli_filter.cc:93: case BROTLI_RESULT_NEEDS_MORE_INPUT: { On 2015/12/04 16:07:52, rdsmith wrote: > How does the underlying Brotli decoding respond if both needs more input and > needs more output are true (i.e. its reached the end of the input available to > it, and completely filled the output available to it)? Decoder has its internal ringbuffer (for backward-references). When this buffer is full, the data is copied to destination buffer. If destination buffer can't hold all the data, then Brotli returns NEEDS_MORE_OUTPUT. If there is not enough input to produce more output, decoder pushes all the data produced so far and returns NEEDS_MORE_INPUT (even if the output buffer is full, brotli could still continue decoding to internal ringbuffer). For the decoder there is no situation that it needs more input and more output simultaneously, because it is state machine inside. So if internal ringbuffer is full and there is not enough data to decode further, decoder will ask for more output first (until it is enough) and after that it will request for more input. https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job.cc:598: if (network_session_params && network_session_params->enable_brotli) { On 2015/12/04 16:07:52, rdsmith wrote: > nit: No curly braces needed if both conditional and statement are single line > (and its more consistent with surrounding code not to have them). Done. https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job.cc:599: advertise_brotli = !request()->url().SchemeIs(url::kHttpScheme); On 2015/12/04 16:07:52, rdsmith wrote: > I *think* you want SchemeIsCryptographic(); that's what I've seen elsewhere for > this type of test. If that doesn't look right to you, push back and I'll dig up > someone who'll know for sure. I'm not sure. The reason of this check is to avoid problems with old proxy servers. If proxy server deals well, for example, with ws/wss protocols, then brotli encoding doesn't seem to be a problem. Even "http" scheme check is too strict, because spdy/http2 protocols almost guarantee (IMHO) that nothing bad should happen to the data with non gzip/deflate Content-Encoding.
components/cronet.gypi & net/ lgtm with nits. I am still learning how to review filter code from rdsmith@, so please wait for his feedback as well. Thanks! https://codereview.chromium.org/1431723002/diff/260001/net/url_request/url_re... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/260001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:650: EXPECT_NE(0, request->GetTotalSentBytes()); Why is this 0? It is because it is a GET request? Maybe consider getting rid of this line and add assertions in the test itself. EXPECT_TRUE(request->status().is_success()); EXPECT_EQ(12, request->received_response_content_length()); EXPECT_EQ(CountWriteBytes(writes, arraysize(writes)), request->GetTotalSentBytes()); EXPECT_EQ(CountReadBytes(reads, arraysize(reads)), request->GetTotalReceivedBytes()); https://codereview.chromium.org/1431723002/diff/260001/net/url_request/url_re... File net/url_request/url_request_job_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/260001/net/url_request/url_re... net/url_request/url_request_job_unittest.cc:27: const char kBrotliHelloData[] = Maybe add a comment here. Something like: // Brotli compressed data that contains hello, world! with a newline character.
This is good with the exception that I want to nail down the scheme test for advertising Brotli; everything else is suggestions or non-actionable commentary. https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... File net/filter/brotli_filter.cc (right): https://codereview.chromium.org/1431723002/diff/220001/net/filter/brotli_filt... net/filter/brotli_filter.cc:54: FilterStatus ReadFilteredData(char* dest_buffer, int* dest_len) override { On 2015/12/07 12:30:38, eustas wrote: > On 2015/12/04 16:07:52, rdsmith wrote: > > When we did the initial review (a long time ago :-}) I remember that it would > be > > useful for Brotli to know when EOF was reached on the input. What's the > status > > of that? Is it waiting on Helen's filter refactor? If so, just for my > > curiosity, where would the information be used in this code? > > We've modified decoder to have weaker requirements... > Now it doesn't need to know about EOF and pushes to output all the data it has > decoded so far. > > So, we do not depend on Helen's refactoring now... Is there value in having that information? (Which I guess is equivalent to: Will you be modifying this code after Helen's refactor?) Or is this just as good with the weaker requirements? https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job.cc:599: advertise_brotli = !request()->url().SchemeIs(url::kHttpScheme); On 2015/12/07 12:30:38, eustas wrote: > On 2015/12/04 16:07:52, rdsmith wrote: > > I *think* you want SchemeIsCryptographic(); that's what I've seen elsewhere > for > > this type of test. If that doesn't look right to you, push back and I'll dig > up > > someone who'll know for sure. > > I'm not sure. The reason of this check is to avoid problems with old proxy > servers. > If proxy server deals well, for example, with ws/wss protocols, then brotli > encoding doesn't seem to be a problem. So I'm confused as to why ws: or ftp: (both of which the current check allows) would be ok. I would think that any cleartext protocol allows interference from proxies, whereas any end-to-end cryptographic protocol does not. Thus my suggestion. > > Even "http" scheme check is too strict, because spdy/http2 protocols almost > guarantee (IMHO) that nothing bad should happen to the data with non > gzip/deflate Content-Encoding. I'm confused by this comment; this check is saying that if it's *not* http you'll advertise brotli, which should include spdy2/http2. https://codereview.chromium.org/1431723002/diff/260001/net/filter/brotli_filt... File net/filter/brotli_filter_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/260001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:47: void TearDown() override { PlatformTest::TearDown(); } nit, suggestion: I don't personally feel like putting in an override just to call the base class makes sense, but I could see this as a style thing (i.e. if there's a SetUp(), there should be a TearDown()), so up to you. https://codereview.chromium.org/1431723002/diff/260001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:148: // Basic scenario: decoding gzip data with big enough buffer. nit: Shouldn't this be "decoding *brotli* data ..."? https://codereview.chromium.org/1431723002/diff/260001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:154: char decode_buffer[kDefaultBufferSize]; Suggestion: Put a DCHECK or ASSERT here that kDefaultBufferSize > source_len(). https://codereview.chromium.org/1431723002/diff/260001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:185: // Tests we can decode when caller has small buffer to read out from filter. DecodeWithSmallBuffer tests small filter buffer + large output buffer. This test tests default filter buffer and small output buffer. Most other tests, I believe, test with default filter buffer and large output buffer. How about a test that tests with small filter buffer + small output buffer? It's probably fine (the single element buffer for both filter and output seems like it should cover the same case) but it is an unexplored test option as best I can see. https://codereview.chromium.org/1431723002/diff/260001/net/url_request/url_re... File net/url_request/url_request_job_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/260001/net/url_request/url_re... net/url_request/url_request_job_unittest.cc:313: #if !defined(DISABLE_BROTLI_SUPPORT) I don't think it's relevant to this CL (yet :-}) but I'll note that I consider it a bug that Chrome doesn't fail requests with unsupported content-encoding parameters. See http://crbug.com/567163.
https://codereview.chromium.org/1431723002/diff/260001/net/filter/brotli_filt... File net/filter/brotli_filter_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/260001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:47: void TearDown() override { PlatformTest::TearDown(); } On 2015/12/07 22:27:28, rdsmith wrote: > nit, suggestion: I don't personally feel like putting in an override just to > call the base class makes sense, but I could see this as a style thing (i.e. if > there's a SetUp(), there should be a TearDown()), so up to you. Ooops, didn't notice this (copied from zip filter unittest). You are right, doesn't make much sense to proxy the base class method. https://codereview.chromium.org/1431723002/diff/260001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:148: // Basic scenario: decoding gzip data with big enough buffer. On 2015/12/07 22:27:28, rdsmith wrote: > nit: Shouldn't this be "decoding *brotli* data ..."? Done. https://codereview.chromium.org/1431723002/diff/260001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:154: char decode_buffer[kDefaultBufferSize]; On 2015/12/07 22:27:28, rdsmith wrote: > Suggestion: Put a DCHECK or ASSERT here that kDefaultBufferSize > source_len(). Done. https://codereview.chromium.org/1431723002/diff/260001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:185: // Tests we can decode when caller has small buffer to read out from filter. On 2015/12/07 22:27:28, rdsmith wrote: > DecodeWithSmallBuffer tests small filter buffer + large output buffer. This > test tests default filter buffer and small output buffer. Most other tests, I > believe, test with default filter buffer and large output buffer. How about a > test that tests with small filter buffer + small output buffer? It's probably > fine (the single element buffer for both filter and output seems like it should > cover the same case) but it is an unexplored test option as best I can see. The more tests the better. Added small/small test. https://codereview.chromium.org/1431723002/diff/260001/net/url_request/url_re... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/260001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:650: EXPECT_NE(0, request->GetTotalSentBytes()); On 2015/12/07 16:27:20, xunjieli wrote: > Why is this 0? It is because it is a GET request? Maybe consider getting rid of > this line and add assertions in the test itself. > > EXPECT_TRUE(request->status().is_success()); > EXPECT_EQ(12, request->received_response_content_length()); > EXPECT_EQ(CountWriteBytes(writes, arraysize(writes)), > request->GetTotalSentBytes()); > EXPECT_EQ(CountReadBytes(reads, arraysize(reads)), > request->GetTotalReceivedBytes()); Surely. Done. https://codereview.chromium.org/1431723002/diff/260001/net/url_request/url_re... File net/url_request/url_request_job_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/260001/net/url_request/url_re... net/url_request/url_request_job_unittest.cc:27: const char kBrotliHelloData[] = On 2015/12/07 16:27:20, xunjieli wrote: > Maybe add a comment here. Something like: > // Brotli compressed data that contains hello, world! with a newline character. Done. https://codereview.chromium.org/1431723002/diff/260001/net/url_request/url_re... net/url_request/url_request_job_unittest.cc:313: #if !defined(DISABLE_BROTLI_SUPPORT) On 2015/12/07 22:27:28, rdsmith wrote: > I don't think it's relevant to this CL (yet :-}) but I'll note that I consider > it a bug that Chrome doesn't fail requests with unsupported content-encoding > parameters. See http://crbug.com/567163. Yes, I've noted this also... And one more (speculative) problem - when filter-chain is constructed it may cause a memleak if outer filter is failed to initialise (see PrependFilter invocations).
On 2015/12/07 22:27:28, rdsmith wrote: > > > useful for Brotli to know when EOF was reached on the input. What's the > > Now it doesn't need to know about EOF and pushes to output all the data it has > Is there value in having that information? (Which I guess is equivalent to: > Will you be modifying this code after Helen's refactor?) Or is this just as > good with the weaker requirements? No, now there is no use of EOF in brotli decoder at all. We've moderately upgraded IO to match zlib... And it became easier to integrate it =) > > > I *think* you want SchemeIsCryptographic(); that's what I've seen elsewhere > > I'm not sure. The reason of this check is to avoid problems with old proxy > So I'm confused as to why ws: or ftp: (both of which the current check allows) > would be ok. I would think that any cleartext protocol allows interference from > proxies, whereas any end-to-end cryptographic protocol does not. Thus my > suggestion. If I remember correct, the problem is not with interference, but with incorrect interference, when proxy servers do something wrong (e.g. cache file with one encoding and send it to client even if encodings doesn't match, or, maybe, sends wrong headers). WS, FTP, Spdy protocols are more interactive and there is less chance that proxy would like to modify the conversation. > > Even "http" scheme check is too strict, because spdy/http2 protocols almost > I'm confused by this comment; this check is saying that if it's *not* http > you'll advertise brotli, which should include spdy2/http2. I've tried to say that URLRequestJob might be a wrong place to decide about encoding. At that level there is no knowledge about the protocol, only the scheme is known at the moment...
On 2015/12/08 11:44:45, eustas wrote: > On 2015/12/07 22:27:28, rdsmith wrote: > > > > I *think* you want SchemeIsCryptographic(); that's what I've seen > elsewhere > > > I'm not sure. The reason of this check is to avoid problems with old proxy > > So I'm confused as to why ws: or ftp: (both of which the current check allows) > > would be ok. I would think that any cleartext protocol allows interference > from > > proxies, whereas any end-to-end cryptographic protocol does not. Thus my > > suggestion. > > If I remember correct, the problem is not with interference, but with incorrect > interference, > when proxy servers do something wrong (e.g. cache file with one encoding and > send it > to client even if encodings doesn't match, or, maybe, sends wrong headers). > WS, FTP, Spdy protocols are more interactive and there is less chance that proxy > would > like to modify the conversation. FTP isn't at all interactive. I also think that if a proxy *can* do something wrong, it (eventually :-}) will, so I'd rather make this check be whether or not an intermediate *can* do something. We also are moving to restricting many new features to HTTPS and equivalent (which is what I take SchemeIsCryptographic() to mean) so this would be in alignment with that plan. Are you aware of any contexts in which using SchemeIsCryptographic() will hurt contexts in which you want Brotli used? (We're deprecating SPDY in favor of HTTP2, which will use the HTTPS scheme, so I'm not worried about it.) The only one I can think of is ws (as opposed to wss), but that seems like enough of a corner case that I don't think it's worth allowing (especially since proxies can still interfere). I also think it'll violate the principle of least astonishment; restricting to cryptographic protocols is relatively simple to explain. Allowing for anything besides HTTP seems like it would be more confusing. > > > Even "http" scheme check is too strict, because spdy/http2 protocols almost > > I'm confused by this comment; this check is saying that if it's *not* http > > you'll advertise brotli, which should include spdy2/http2. > > I've tried to say that URLRequestJob might be a wrong place to decide about > encoding. > At that level there is no knowledge about the protocol, only the scheme is known > at the moment... (Assuming you mean URLRequestHTTPJob, which I think of as somewhat better than URLRequestJob for this kind of stuff.) We don't have a lot of options beyond URLRequestHTTPJob, and my understanding is that if we upgrade from HTTP to HTTPS it will generally be via redirect that spawns a new request. HTTPS may be established over HTTP1.1/TLS, HTTP/2, or QUIC, but all of those are fine for preventing intermediates modifying the stream.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1431723002/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job.cc:599: advertise_brotli = !request()->url().SchemeIs(url::kHttpScheme); On 2015/12/07 22:27:28, rdsmith wrote: > On 2015/12/07 12:30:38, eustas wrote: > > On 2015/12/04 16:07:52, rdsmith wrote: > > > I *think* you want SchemeIsCryptographic(); that's what I've seen elsewhere > > for > > > this type of test. If that doesn't look right to you, push back and I'll > dig > > up > > > someone who'll know for sure. > > > > I'm not sure. The reason of this check is to avoid problems with old proxy > > servers. > > If proxy server deals well, for example, with ws/wss protocols, then brotli > > encoding doesn't seem to be a problem. > > So I'm confused as to why ws: or ftp: (both of which the current check allows) > would be ok. I would think that any cleartext protocol allows interference from > proxies, whereas any end-to-end cryptographic protocol does not. Thus my > suggestion. +1 - I agree with Randy here. !SchemeIs(url::kHttpScheme) smells like an anti-pattern here, and security has been working to remove such ad-hoc checks in favour of SchemeIsCryptographic Further, when we consider what other user agents may look for with implementing, it's far more problematic to suggest a scheme-by-scheme whitelist as opposed to simply stating 'secure origins'.
> We're deprecating SPDY in favor of HTTP2, which will use the > HTTPS scheme. Didn't know about this. This fact eliminates all my worries. BTW, already switched to "SchemeIsCryptographic" in last patchset =)
eustas@chromium.org changed reviewers: + agrieve@chromium.org
agrieve@chromium.org: Please review changes in build/common.gypi
On 2015/12/09 13:14:27, eustas wrote: > mailto:agrieve@chromium.org: Please review changes in build/common.gypi build/ lgtm
net/ LGTM. Thanks for your patience!
https://codereview.chromium.org/1431723002/diff/280001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1431723002/diff/280001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1358: return command_line.HasSwitch(switches::kEnableBrotli); Why did you go the route of double-flags? Typically we do one or the other - and transition appropriately. Do you anticipate needing to enable this by policy? Why or why not? Do you anticipate needing to enable this on ChromeOS and Android? Why or why not? Do you have any framework for examining the error rates for places that enable Brotli vs those that don't? I'm concerned that this may not be the right thing; the QUIC code already has too many command-line options and are inconsistent with Chrome's general policy about command-line flags, so I wouldn't use that code as your model. 1) If you expect to need this on Android or ChromeOS by developers during testing, then you should be making sure that the prefs system is being used, so that you can flag that pref via about:flags 2) If you expect to control this via Finch (and you probably should, at the least for the A/B analysis of network errors w/ and w/o this being offered), then you should probably be using the base::Feature API Note that 1 & 2 are not mutually exclusive.
On 2015/12/09 18:10:11, Ryan Sleevi wrote: > https://codereview.chromium.org/1431723002/diff/280001/chrome/browser/io_thre... > File chrome/browser/io_thread.cc (right): > > https://codereview.chromium.org/1431723002/diff/280001/chrome/browser/io_thre... > chrome/browser/io_thread.cc:1358: return > command_line.HasSwitch(switches::kEnableBrotli); > Why did you go the route of double-flags? Typically we do one or the other - and > transition appropriately. > Do you anticipate needing to enable this by policy? Why or why not? > Do you anticipate needing to enable this on ChromeOS and Android? Why or why > not? > Do you have any framework for examining the error rates for places that enable > Brotli vs those that don't? > > I'm concerned that this may not be the right thing; the QUIC code already has > too many command-line options and are inconsistent with Chrome's general policy > about command-line flags, so I wouldn't use that code as your model. > > 1) If you expect to need this on Android or ChromeOS by developers during > testing, then you should be making sure that the prefs system is being used, so > that you can flag that pref via about:flags > 2) If you expect to control this via Finch (and you probably should, at the > least for the A/B analysis of network errors w/ and w/o this being offered), > then you should probably be using the base::Feature API Agree that this is the right way to set up the configuration. It will match the typical style you see of on/off/default in about:flags. > > Note that 1 & 2 are not mutually exclusive.
https://codereview.chromium.org/1431723002/diff/280001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1431723002/diff/280001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1358: return command_line.HasSwitch(switches::kEnableBrotli); > 1) If you expect to need this on Android or ChromeOS by developers during > testing, then you should be making sure that the prefs system is being used, so > that you can flag that pref via about:flags > 2) If you expect to control this via Finch (and you probably should, at the > least for the A/B analysis of network errors w/ and w/o this being offered), > then you should probably be using the base::Feature API > > Note that 1 & 2 are not mutually exclusive. Well, I think I need someone's help/advise to implement it. To use a base:Feature/FieldTrial I need to declare it somewhere (and both about_flags and io_thread must be able to refer this place). What would you recommend?
> 1) If you expect to need this on Android or ChromeOS by developers during > testing, then you should be making sure that the prefs system is being used, so > that you can flag that pref via about:flags As I see "spdy" pref is propagated to net subsystem via static field. Should we do the same, or there is a better way of doing this?
On 2015/12/10 16:19:24, eustas wrote: > https://codereview.chromium.org/1431723002/diff/280001/chrome/browser/io_thre... > File chrome/browser/io_thread.cc (right): > > https://codereview.chromium.org/1431723002/diff/280001/chrome/browser/io_thre... > chrome/browser/io_thread.cc:1358: return > command_line.HasSwitch(switches::kEnableBrotli); > > 1) If you expect to need this on Android or ChromeOS by developers during > > testing, then you should be making sure that the prefs system is being used, > so > > that you can flag that pref via about:flags > > 2) If you expect to control this via Finch (and you probably should, at the > > least for the A/B analysis of network errors w/ and w/o this being offered), > > then you should probably be using the base::Feature API > > > > Note that 1 & 2 are not mutually exclusive. > > Well, I think I need someone's help/advise to implement it. > To use a base:Feature/FieldTrial I need to declare it somewhere (and both > about_flags and io_thread must be able to refer this place). What would you > recommend? I haven't done this myself. It was suggested that we could create a net/features/net_features.cc/h to put the flag. That would mirror //content and other components. E.g. content/public/common/content_features.cc components/password_manager/core/common/password_manager_features.cc WDYT?
On 2015/12/11 16:03:22, xunjieli wrote: > > Well, I think I need someone's help/advise to implement it. > > To use a base:Feature/FieldTrial I need to declare it somewhere (and both > > about_flags and io_thread must be able to refer this place). What would you > > recommend? > > I haven't done this myself. It was suggested that we could create a > net/features/net_features.cc/h to put the flag. > That would mirror //content and other components. E.g. > content/public/common/content_features.cc > components/password_manager/core/common/password_manager_features.cc > WDYT? Helen: Any reason you preferred that over the more preferable net/url_request or net/http alternatives suggested? I don't have strong opinions, but it seems like we should avoid creating new top-level directories unless we can demonstrate there's significant (design) need. I was just posing it as the nuclear option if we could find no better place.
On 2015/12/11 16:13:23, Ryan Sleevi wrote: > On 2015/12/11 16:03:22, xunjieli wrote: > > > Well, I think I need someone's help/advise to implement it. > > > To use a base:Feature/FieldTrial I need to declare it somewhere (and both > > > about_flags and io_thread must be able to refer this place). What would you > > > recommend? > > > > I haven't done this myself. It was suggested that we could create a > > net/features/net_features.cc/h to put the flag. > > That would mirror //content and other components. E.g. > > content/public/common/content_features.cc > > components/password_manager/core/common/password_manager_features.cc > > WDYT? > > Helen: Any reason you preferred that over the more preferable net/url_request or > net/http alternatives suggested? I don't have strong opinions, but it seems like > we should avoid creating new top-level directories unless we can demonstrate > there's significant (design) need. I was just posing it as the nuclear option if > we could find no better place. Thanks for following up on this. I misread the suggestion. Eustas, please follow Sleevi's advice to choose either net/url_request or net/http to put the features file.
> Thanks for following up on this. I misread the suggestion. Eustas, please
follow
> Sleevi's advice to choose either net/url_request or net/http to put the
features
> file.
Placed feature to http/http_features.{cc,h}
lgtm with nits https://codereview.chromium.org/1431723002/diff/300001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1431723002/diff/300001/chrome/browser/about_f... chrome/browser/about_flags.cc:2030: #endif nit: suggest adding a comment at the end. #endif // !defined(DISABLE_BROTILI_SUPPORT) https://codereview.chromium.org/1431723002/diff/300001/net/http/http_features.cc File net/http/http_features.cc (right): https://codereview.chromium.org/1431723002/diff/300001/net/http/http_features... net/http/http_features.cc:11: // Enables brotli "Accept-Encooding" advertising and "Content-Encoding" support. nit: typo in Encoding https://codereview.chromium.org/1431723002/diff/300001/net/http/http_features... net/http/http_features.cc:11: // Enables brotli "Accept-Encooding" advertising and "Content-Encoding" support. Could you also link ietf draft here? http://www.ietf.org/id/draft-alakuijala-brotli https://codereview.chromium.org/1431723002/diff/300001/net/http/http_features.h File net/http/http_features.h (right): https://codereview.chromium.org/1431723002/diff/300001/net/http/http_features... net/http/http_features.h:20: // Checks if brotli encoding feature is enabled AND supported. Is there a reason that AND is capitalized?
https://codereview.chromium.org/1431723002/diff/300001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1431723002/diff/300001/chrome/browser/about_f... chrome/browser/about_flags.cc:2030: #endif On 2015/12/14 16:02:27, xunjieli wrote: > nit: suggest adding a comment at the end. > > #endif // !defined(DISABLE_BROTILI_SUPPORT) Done. https://codereview.chromium.org/1431723002/diff/300001/net/http/http_features.cc File net/http/http_features.cc (right): https://codereview.chromium.org/1431723002/diff/300001/net/http/http_features... net/http/http_features.cc:11: // Enables brotli "Accept-Encooding" advertising and "Content-Encoding" support. On 2015/12/14 16:02:27, xunjieli wrote: > nit: typo in Encoding Done. https://codereview.chromium.org/1431723002/diff/300001/net/http/http_features... net/http/http_features.cc:11: // Enables brotli "Accept-Encooding" advertising and "Content-Encoding" support. On 2015/12/14 16:02:27, xunjieli wrote: > Could you also link ietf draft here? > http://www.ietf.org/id/draft-alakuijala-brotli Done. https://codereview.chromium.org/1431723002/diff/300001/net/http/http_features.h File net/http/http_features.h (right): https://codereview.chromium.org/1431723002/diff/300001/net/http/http_features... net/http/http_features.h:20: // Checks if brotli encoding feature is enabled AND supported. On 2015/12/14 16:02:27, xunjieli wrote: > Is there a reason that AND is capitalized? To emphasise that not only feature value is taken into account. Fixed.
chrome/browser/net LGTM One thing confuses me though - should the new brotli feature flag be at chrome/ layer rather than net/ layer? it looks like there were recommendations earlier in the code review to place the new Brotli Feature in the net/ subdirectory (don't really care about net/http etc.). However, this is only really accessed at the chrome layer (about_flags, and io_thread). I think if net/ were directly accessing it than placing it in the net/ layer makes sense. But I think the approach taken of using HttpNetworkSession::Params to configure the behavior rather than the Feature is probably the right call as we can do this on a per-URLRequestContext basis. Not going to block the CL on this, but something that feels a bit strange to me.
Chris: I'm torn on this. Really, the layering question is one of whether //net should be doing FieldTrials. If we say "No, //net should not do FieldTrials" - then I agree, we should uplift the base::Feature[List] up to the layer that should (whether //content or //chrome) However, if we say "Yes, //net should do FieldTrials", then we should converge on base::Feature[List] as the preferred way to handle all ways of interacting with that field trial (including pref handling), and that means exporting it as part of //net. There are some other design issues here that I missed when taking a cursory pass, so I'd like to highlight them. https://codereview.chromium.org/1431723002/diff/320001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1431723002/diff/320001/build/common.gypi#newc... build/common.gypi:601: 'disable_brotli_support%': 0, There is strong resistance towards adding system-wide flags like this. Can you explain more why this is necessary, and whether you see this as short-term or long-term? https://codereview.chromium.org/1431723002/diff/320001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1431723002/diff/320001/chrome/browser/about_f... chrome/browser/about_flags.cc:2024: #if !defined(DISABLE_BROTLI_SUPPORT) BUG: Please review https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5c4ySpPsV14/JJfAe... #if BUILDFLAG(DISABLE_BROTLI_SUPPORT) https://codereview.chromium.org/1431723002/diff/320001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/1431723002/diff/320001/components/cronet.gypi... components/cronet.gypi:160: 'DISABLE_BROTLI_SUPPORT=1', Why is this done just for this target? https://codereview.chromium.org/1431723002/diff/320001/net/filter/brotli_filt... File net/filter/brotli_filter.cc (right): https://codereview.chromium.org/1431723002/diff/320001/net/filter/brotli_filt... net/filter/brotli_filter.cc:15: #if !defined(DISABLE_BROTLI_SUPPORT) #if !BUILDFLAG(DISABLE_BROTLI_SUPPORT) https://codereview.chromium.org/1431723002/diff/320001/net/filter/brotli_filt... File net/filter/brotli_filter_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/320001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:55: int source_len, https://www.chromium.org/developers/coding-style#TOC-Types "use size_t for object and allocation sizes, boject counts, array and pointer offsets, vector indices, and so on" https://codereview.chromium.org/1431723002/diff/320001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:231: TEST_F(BrotliUnitTest, DecodeMissingData) { Why don't any of these tests need to be conditionally enabled/disabled? https://codereview.chromium.org/1431723002/diff/320001/net/filter/filter.cc File net/filter/filter.cc (right): https://codereview.chromium.org/1431723002/diff/320001/net/filter/filter.cc#n... net/filter/filter.cc:42: const char kBrotli[] = "br"; Why don't you keep the style of the surrounding section - by indenting? https://codereview.chromium.org/1431723002/diff/320001/net/filter/filter.h File net/filter/filter.h (right): https://codereview.chromium.org/1431723002/diff/320001/net/filter/filter.h#ne... net/filter/filter.h:235: friend class BrotliFilter; ^ this strikes me as an anti-pattern, as it represents circular layering. Could you explain more? https://codereview.chromium.org/1431723002/diff/320001/net/url_request/url_re... File net/url_request/url_request_job_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/320001/net/url_request/url_re... net/url_request/url_request_job_unittest.cc:317: #endif What's the expected result if brotli is disabled? Why run this test at all when it is?
https://codereview.chromium.org/1431723002/diff/320001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1431723002/diff/320001/build/common.gypi#newc... build/common.gypi:601: 'disable_brotli_support%': 0, On 2015/12/17 01:04:40, Ryan Sleevi wrote: > There is strong resistance towards adding system-wide flags like this. > > Can you explain more why this is necessary, and whether you see this as > short-term or long-term? This flag was added in response to a request to make it possible completely disable brotli in a build. So I've decided to mimic FTP and File support. I'm OK to remove it (it was planned as a short-term, until feature is fully launched). https://codereview.chromium.org/1431723002/diff/320001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1431723002/diff/320001/chrome/browser/about_f... chrome/browser/about_flags.cc:2024: #if !defined(DISABLE_BROTLI_SUPPORT) On 2015/12/17 01:04:40, Ryan Sleevi wrote: > BUG: Please review > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5c4ySpPsV14/JJfAe... > > #if BUILDFLAG(DISABLE_BROTLI_SUPPORT) All 3 exmaples of buildflag look simple - I don't see a way to change the value of the flag for concrete target (e.g. for cronet_small). Is it possible? https://codereview.chromium.org/1431723002/diff/320001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/1431723002/diff/320001/components/cronet.gypi... components/cronet.gypi:160: 'DISABLE_BROTLI_SUPPORT=1', On 2015/12/17 01:04:40, Ryan Sleevi wrote: > Why is this done just for this target? Probably I have missed something... What other targets should exclude brotli support? brotli support is disabled for cronet_static_small to avoid overhead brought by brotli decoder (~150kb of code/data). https://codereview.chromium.org/1431723002/diff/320001/net/filter/brotli_filt... File net/filter/brotli_filter_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/320001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:55: int source_len, On 2015/12/17 01:04:40, Ryan Sleevi wrote: > https://www.chromium.org/developers/coding-style#TOC-Types > > "use size_t for object and allocation sizes, boject counts, array and pointer > offsets, vector indices, and so on" I also prefer to use size_t where appropriate, but Filter class has "int" type everywhere. If I turn source_len to size_t it would require adding a bunch of static_casts https://codereview.chromium.org/1431723002/diff/320001/net/filter/brotli_filt... net/filter/brotli_filter_unittest.cc:231: TEST_F(BrotliUnitTest, DecodeMissingData) { On 2015/12/17 01:04:40, Ryan Sleevi wrote: > Why don't any of these tests need to be conditionally enabled/disabled? The whole BrotliUnitTest is excluded when brotli support is disabled. FilterFactory returns nullptr in this case -> nothing to test. https://codereview.chromium.org/1431723002/diff/320001/net/filter/filter.cc File net/filter/filter.cc (right): https://codereview.chromium.org/1431723002/diff/320001/net/filter/filter.cc#n... net/filter/filter.cc:42: const char kBrotli[] = "br"; On 2015/12/17 01:04:40, Ryan Sleevi wrote: > Why don't you keep the style of the surrounding section - by indenting? presubmit is unhappy and "git cl format net" always corrects back. Fixed. https://codereview.chromium.org/1431723002/diff/320001/net/filter/filter.h File net/filter/filter.h (right): https://codereview.chromium.org/1431723002/diff/320001/net/filter/filter.h#ne... net/filter/filter.h:235: friend class BrotliFilter; On 2015/12/17 01:04:40, Ryan Sleevi wrote: > ^ this strikes me as an anti-pattern, as it represents circular layering. Could > you explain more? For some reason InitBuffer is a private method. I've baked its invocation into CreateBrotliFilter to have the least amount of added code to filter.cc Rolled back. https://codereview.chromium.org/1431723002/diff/320001/net/url_request/url_re... File net/url_request/url_request_job_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/320001/net/url_request/url_re... net/url_request/url_request_job_unittest.cc:317: #endif On 2015/12/17 01:04:40, Ryan Sleevi wrote: > What's the expected result if brotli is disabled? Why run this test at all when > it is? Ideally it would be "request failed, no data" (see https://code.google.com/p/chromium/issues/detail?id=567163) Currently it is unfiltered data. I've moved more expects inside of #if and added TODO.
I'm unfortunately going to have to put a Not LGTM on this until we can get the build flag sorted out. If you're not sure what steps to take, Brett or I should be able to help you out; the aforementioned email thread should explain. As Brett notes in the email, "when adding a new feature flag, please don't add it globally" It also doesn't work with how you're building cronet_small, so if that's the only reason for it, then we should talk. https://codereview.chromium.org/1431723002/diff/320001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1431723002/diff/320001/build/common.gypi#newc... build/common.gypi:601: 'disable_brotli_support%': 0, On 2015/12/17 16:44:54, eustas wrote: > On 2015/12/17 01:04:40, Ryan Sleevi wrote: > > There is strong resistance towards adding system-wide flags like this. > > > > Can you explain more why this is necessary, and whether you see this as > > short-term or long-term? > > This flag was added in response to a request to make it possible completely > disable brotli in a build. > So I've decided to mimic FTP and File support. > I'm OK to remove it (it was planned as a short-term, until feature is fully > launched). Right, we're generally trying not to add build flags like that, since every permutation adds to complexity and testing. For example, it's unclear which is "supported" or not, what bots we have building either, etc. https://codereview.chromium.org/1431723002/diff/320001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1431723002/diff/320001/chrome/browser/about_f... chrome/browser/about_flags.cc:2024: #if !defined(DISABLE_BROTLI_SUPPORT) On 2015/12/17 16:44:54, eustas wrote: > On 2015/12/17 01:04:40, Ryan Sleevi wrote: > > BUG: Please review > > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5c4ySpPsV14/JJfAe... > > > > #if BUILDFLAG(DISABLE_BROTLI_SUPPORT) > > All 3 exmaples of buildflag look simple - I don't see a way to change the value > of the flag for concrete target (e.g. for cronet_small). Is it possible? That's not possible today either. That's part of why it's a BUG - what you're trying to do (defining at the top-most target whether or not a feature is enabled) is explicitly not supported.
On 2015/12/17 22:49:33, Ryan Sleevi wrote: > I'm unfortunately going to have to put a Not LGTM on this until we can get the > build flag sorted out. Removed build flag. Now brotli is excluded only for net_small target to avoid burden on cronet.
PING
PTAL
Ping
Much better! Apologies for the holiday delays. I think the big question that remains is, with this cleanup, whether or not it still makes sense to have http_features; it seems like this can get lifted up to the content flags in //content, now that the HttpSession simply takes it as a bool. Did I miss something obvious when reviewing? https://codereview.chromium.org/1431723002/diff/440001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1431723002/diff/440001/net/BUILD.gn#newcode462 net/BUILD.gn:462: # Same as net, but with ICU, file, ftp, and websocket support stripped. Update comment? https://codereview.chromium.org/1431723002/diff/440001/net/DEPS File net/DEPS (right): https://codereview.chromium.org/1431723002/diff/440001/net/DEPS#newcode61 net/DEPS:61: "+third_party/brotli", If you're looking for parity with base/i18n and third_party/icu, then you should also update lines 14-17 to indicate that it's intentional that only a single file is allowed to depend on brotli. https://codereview.chromium.org/1431723002/diff/440001/net/filter/brotli_filt... File net/filter/brotli_filter.cc (right): https://codereview.chromium.org/1431723002/diff/440001/net/filter/brotli_filt... net/filter/brotli_filter.cc:30: brotli_state_.reset(new BrotliState); Does this actually need to be a pointer? It seems like you could make BrotliState a member variable of this class, and just leave InitDecoding / Cleanup to do the BrotliStateInit/BrotliStateCleanup https://codereview.chromium.org/1431723002/diff/440001/net/filter/brotli_filt... net/filter/brotli_filter.cc:69: int bytes_written = *dest_len - available_out; DANGER: You're mixing signed and unsigned types in math, and using a third-party library. While it may be safe, it can often cause subtle bugs if the third-party library ends up changing API behaviour or contract. It may be wise to use the base/safe_numerics we have and use the appropriate checked types, as an defense in depth when mixing these. For example, I think we want to avoid 'decompression bomb' type attacks where the attacker is able to return a >int sized bit of data. It also gives you strong guards against bad calls to ReadFilteredData, where dest_len might be a negative value (which would be developer error, but hey, mistakes happen) https://codereview.chromium.org/1431723002/diff/440001/net/filter/brotli_filt... net/filter/brotli_filter.cc:77: // Fall through. This comment doesn't seem correct; you break on line 75. Should you move line 73 to 94, and then actually let it fall through to the default case? https://codereview.chromium.org/1431723002/diff/440001/net/filter/brotli_filt... net/filter/brotli_filter.cc:105: } Should this be a switch, rather than a conditional? For example, when reading this, it's ambiguous whether or not there are more statuses (there are; FILTER_NEED_MORE_DATA and FILTER_OK) or what the 'expected' value of decoding_status_ should be. https://codereview.chromium.org/1431723002/diff/440001/net/filter/brotli_filt... File net/filter/brotli_filter.h (right): https://codereview.chromium.org/1431723002/diff/440001/net/filter/brotli_filt... net/filter/brotli_filter.h:10: // for sample usage. It seems like lines 5-10 belong as class-level documentation in the .cc; strictly speaking, from the API contract that this header file provides, it doesn't seem necessary to say any of this, and it may be coupling implementation and interface https://codereview.chromium.org/1431723002/diff/440001/net/filter/brotli_filt... net/filter/brotli_filter.h:19: Filter* CreateBrotliFilter(Filter::FilterType type_id); Document? https://codereview.chromium.org/1431723002/diff/440001/net/filter/filter.cc File net/filter/filter.cc (right): https://codereview.chromium.org/1431723002/diff/440001/net/filter/filter.cc#n... net/filter/filter.cc:363: } no braces here, but a newline between 363 (well, 362) and 364 wouldn't hurt. https://codereview.chromium.org/1431723002/diff/440001/net/url_request/url_re... File net/url_request/url_request_job_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/440001/net/url_request/url_re... net/url_request/url_request_job_unittest.cc:312: base::MessageLoop::current()->Run(); I realize you're keeping with existing style, but I think it'd be more advisable here to use base::RunLoop().Run() [until Idle?]
I have no strong opinion about the best place for feature declaration -> moved it to content. https://codereview.chromium.org/1431723002/diff/440001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1431723002/diff/440001/net/BUILD.gn#newcode462 net/BUILD.gn:462: # Same as net, but with ICU, file, ftp, and websocket support stripped. On 2015/12/28 19:30:53, Ryan Sleevi wrote: > Update comment? Done. https://codereview.chromium.org/1431723002/diff/440001/net/DEPS File net/DEPS (right): https://codereview.chromium.org/1431723002/diff/440001/net/DEPS#newcode61 net/DEPS:61: "+third_party/brotli", On 2015/12/28 19:30:53, Ryan Sleevi wrote: > If you're looking for parity with base/i18n and third_party/icu, then you should > also update lines 14-17 to indicate that it's intentional that only a single > file is allowed to depend on brotli. Done. https://codereview.chromium.org/1431723002/diff/440001/net/filter/brotli_filt... File net/filter/brotli_filter.cc (right): https://codereview.chromium.org/1431723002/diff/440001/net/filter/brotli_filt... net/filter/brotli_filter.cc:30: brotli_state_.reset(new BrotliState); On 2015/12/28 19:30:53, Ryan Sleevi wrote: > Does this actually need to be a pointer? It seems like you could make > BrotliState a member variable of this class, and just leave InitDecoding / > Cleanup to do the BrotliStateInit/BrotliStateCleanup No need. It just moved from previous patches. Done. https://codereview.chromium.org/1431723002/diff/440001/net/filter/brotli_filt... net/filter/brotli_filter.cc:69: int bytes_written = *dest_len - available_out; On 2015/12/28 19:30:53, Ryan Sleevi wrote: > DANGER: You're mixing signed and unsigned types in math, and using a third-party > library. While it may be safe, it can often cause subtle bugs if the third-party > library ends up changing API behaviour or contract. > > It may be wise to use the base/safe_numerics we have and use the appropriate > checked types, as an defense in depth when mixing these. For example, I think we > want to avoid 'decompression bomb' type attacks where the attacker is able to > return a >int sized bit of data. It also gives you strong guards against bad > calls to ReadFilteredData, where dest_len might be a negative value (which would > be developer error, but hey, mistakes happen) Good idea. Done. https://codereview.chromium.org/1431723002/diff/440001/net/filter/brotli_filt... net/filter/brotli_filter.cc:77: // Fall through. On 2015/12/28 19:30:53, Ryan Sleevi wrote: > This comment doesn't seem correct; you break on line 75. Should you move line 73 > to 94, and then actually let it fall through to the default case? I meant that "case BROTLI_RESULT_NEEDS_MORE_OUTPUT:" falls through to "case BROTLI_RESULT_SUCCESS: {" But combining error & default also looks good. https://codereview.chromium.org/1431723002/diff/440001/net/filter/brotli_filt... net/filter/brotli_filter.cc:105: } On 2015/12/28 19:30:53, Ryan Sleevi wrote: > Should this be a switch, rather than a conditional? > > For example, when reading this, it's ambiguous whether or not there are more > statuses (there are; FILTER_NEED_MORE_DATA and FILTER_OK) or what the 'expected' > value of decoding_status_ should be. Backed this conditional to previous switch; now it is easier to track state transitions. https://codereview.chromium.org/1431723002/diff/440001/net/filter/brotli_filt... File net/filter/brotli_filter.h (right): https://codereview.chromium.org/1431723002/diff/440001/net/filter/brotli_filt... net/filter/brotli_filter.h:10: // for sample usage. On 2015/12/28 19:30:53, Ryan Sleevi wrote: > It seems like lines 5-10 belong as class-level documentation in the .cc; > strictly speaking, from the API contract that this header file provides, it > doesn't seem necessary to say any of this, and it may be coupling implementation > and interface Surely. https://codereview.chromium.org/1431723002/diff/440001/net/filter/brotli_filt... net/filter/brotli_filter.h:19: Filter* CreateBrotliFilter(Filter::FilterType type_id); On 2015/12/28 19:30:53, Ryan Sleevi wrote: > Document? Done. https://codereview.chromium.org/1431723002/diff/440001/net/filter/filter.cc File net/filter/filter.cc (right): https://codereview.chromium.org/1431723002/diff/440001/net/filter/filter.cc#n... net/filter/filter.cc:363: } On 2015/12/28 19:30:53, Ryan Sleevi wrote: > no braces here, but a newline between 363 (well, 362) and 364 wouldn't hurt. Fixed. https://codereview.chromium.org/1431723002/diff/440001/net/url_request/url_re... File net/url_request/url_request_job_unittest.cc (right): https://codereview.chromium.org/1431723002/diff/440001/net/url_request/url_re... net/url_request/url_request_job_unittest.cc:312: base::MessageLoop::current()->Run(); On 2015/12/28 19:30:53, Ryan Sleevi wrote: > I realize you're keeping with existing style, but I think it'd be more advisable > here to use base::RunLoop().Run() [until Idle?] Yes, looks better. Done.
eustas@chromium.org changed reviewers: + jam@chromium.org
jam@ : please take a look at content/public/common
On 2015/12/29 13:19:52, eustas wrote: > jam@ : please take a look at content/public/common lgtm I initially thought that the feature flag should be in /net, but now I read the whole conversation about it :) It still seems odd that it's in content, since content doesn't use it. It seems like it should be in chrome/ since that's the only layer that uses it. but I won't block the cl on that. lgtm
LGTM, % fixing the safe_numerics CC'ing jschuh because I had to check the safe-numerics usage; there's a bug there, but it should be trivially fixable. https://codereview.chromium.org/1431723002/diff/460001/net/filter/brotli_filt... File net/filter/brotli_filter.cc (right): https://codereview.chromium.org/1431723002/diff/460001/net/filter/brotli_filt... net/filter/brotli_filter.cc:44: return Filter::FILTER_ERROR; If this ever happens, isn't it a programmer error? That seems like a CHECK() is fine, meaning size_t output_buffer_size = base::checked_cast<size_t>(*dest_len); should be safe https://codereview.chromium.org/1431723002/diff/460001/net/filter/brotli_filt... net/filter/brotli_filter.cc:64: base::checked_cast<int>(output_buffer_size - available_out); So this isn't actually safe (or at least, jschuh agreed it looked wrong/dangerous) You could do size_t safe_bytes_written = base::CheckedNumeric<size_t>(output_buffer_size); safe_bytes_written -= available_out; int bytes_written = base::checked_cast<int>(safe_bytes_written.ValueOrDie()); That will cover the case where available_out > output_buffer_size (a range underflow), as well as downcasting to int (a range overflow)
https://codereview.chromium.org/1431723002/diff/460001/net/filter/brotli_filt... File net/filter/brotli_filter.cc (right): https://codereview.chromium.org/1431723002/diff/460001/net/filter/brotli_filt... net/filter/brotli_filter.cc:44: return Filter::FILTER_ERROR; On 2015/12/29 20:59:11, Ryan Sleevi wrote: > If this ever happens, isn't it a programmer error? That seems like a CHECK() is > fine, meaning > > size_t output_buffer_size = base::checked_cast<size_t>(*dest_len); > > should be safe Surely. https://codereview.chromium.org/1431723002/diff/460001/net/filter/brotli_filt... net/filter/brotli_filter.cc:64: base::checked_cast<int>(output_buffer_size - available_out); On 2015/12/29 20:59:11, Ryan Sleevi wrote: > So this isn't actually safe (or at least, jschuh agreed it looked > wrong/dangerous) > > You could do > size_t safe_bytes_written = base::CheckedNumeric<size_t>(output_buffer_size); > safe_bytes_written -= available_out; > int bytes_written = base::checked_cast<int>(safe_bytes_written.ValueOrDie()); > > > That will cover the case where available_out > output_buffer_size (a range > underflow), as well as downcasting to int (a range overflow) Done. I've also added explicit decompressor contract checks (that available_in/out could only decrease) and checked_cast for stream_data_len_ assignment (because it is also int).
The CQ bit was checked by eustas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, agrieve@chromium.org, rdsmith@chromium.org, xunjieli@chromium.org, cbentzel@chromium.org, rsleevi@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/1431723002/#ps480001 (title: "More safe conversions")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1431723002/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1431723002/480001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by eustas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbentzel@chromium.org, rkaplow@chromium.org, rsleevi@chromium.org, jam@chromium.org, agrieve@chromium.org, rdsmith@chromium.org, xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/1431723002/#ps500001 (title: "Fixed path in net.gyp")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1431723002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1431723002/500001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by eustas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbentzel@chromium.org, rkaplow@chromium.org, rsleevi@chromium.org, jam@chromium.org, agrieve@chromium.org, rdsmith@chromium.org, xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/1431723002/#ps520001 (title: "Exclude brotli_filter_unittest on ios - needs to read input data files")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1431723002/520001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1431723002/520001
Message was sent while issue was closed.
Description was changed from ========== Add brotli content-encoding filter. BUG=472009 ========== to ========== Add brotli content-encoding filter. BUG=472009 ==========
Message was sent while issue was closed.
Committed patchset #27 (id:520001)
Message was sent while issue was closed.
Committed patchset #27 (id:520001)
Message was sent while issue was closed.
Description was changed from ========== Add brotli content-encoding filter. BUG=472009 ========== to ========== Add brotli content-encoding filter. BUG=472009 Committed: https://crrev.com/fbec913fc94fbf68bfbb3e9d46449d2cc8b31e10 Cr-Commit-Position: refs/heads/master@{#367153} ==========
Message was sent while issue was closed.
Patchset 27 (id:??) landed as https://crrev.com/fbec913fc94fbf68bfbb3e9d46449d2cc8b31e10 Cr-Commit-Position: refs/heads/master@{#367153}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
This has memory errors: https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28...
Message was sent while issue was closed.
On 2015/12/30 22:54:28, Nico wrote: > This has memory errors: > https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28... This one is actually the same as in issue 542575 (see https://codereview.chromium.org/1467203002). I'm going to update suppressions soon. |
