Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
361083002:
Small cleanups to ported QuicServer and dependencies (Closed)
DescriptionSmall cleanups to ported QuicServer and dependencies
Based on review comments from https://codereview.chromium.org/337093003/
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282275
Patch Set 1 #
Total comments: 6
Patch Set 2 : Bring back quic_tools (formerly quic_ported_server) #
Total comments: 8
Patch Set 3 : Go back to using a reference for QuicInMemoryCache::Response::headers() #Patch Set 4 : Rebase onto upstream #Messages
Total messages: 17 (0 generated)
Review comments on patch set 1: Seems good. I'd like to take a closer look tomorrow. https://codereview.chromium.org/361083002/diff/1/net/BUILD.gn File net/BUILD.gn (left): https://codereview.chromium.org/361083002/diff/1/net/BUILD.gn#oldcode1024 net/BUILD.gn:1024: source_set("quic_tools") { Why are you deleting this? https://codereview.chromium.org/361083002/diff/1/net/net.gypi File net/net.gypi (right): https://codereview.chromium.org/361083002/diff/1/net/net.gypi#newcode899 net/net.gypi:899: 'quic/quic_in_memory_cache.h', These should go into some target that is only used by net_unittests. Similarly for quic/quic_server.* and quic/quic_spdy_server_stream.*. https://codereview.chromium.org/361083002/diff/1/net/quic/quic_server.h File net/quic/quic_server.h (right): https://codereview.chromium.org/361083002/diff/1/net/quic/quic_server.h#newco... net/quic/quic_server.h:32: class NET_EXPORT_PRIVATE QuicServer { This should be unnecessary if QuicServer is part of a target used only by net_unittests.
Let's discuss this tomorrow. https://codereview.chromium.org/361083002/diff/1/net/BUILD.gn File net/BUILD.gn (left): https://codereview.chromium.org/361083002/diff/1/net/BUILD.gn#oldcode1024 net/BUILD.gn:1024: source_set("quic_tools") { On 2014/07/02 01:01:40, wtc wrote: > > Why are you deleting this? It's the equivalent to quic_ported_server in net.gyp https://codereview.chromium.org/361083002/diff/1/net/net.gypi File net/net.gypi (right): https://codereview.chromium.org/361083002/diff/1/net/net.gypi#newcode899 net/net.gypi:899: 'quic/quic_in_memory_cache.h', On 2014/07/02 01:01:41, wtc wrote: > > These should go into some target that is only used by net_unittests. > > Similarly for quic/quic_server.* and quic/quic_spdy_server_stream.*. That's a good point. Unfortunately, currently, QuicDispatcher depends on QuicSpdyServerStream, which depends on QuicInMemoryCache. This comes back to making QuicDispatcher more general.
I've brought back quic_tools (formerly called quic_ported_server). Now the CL is quite simple. PTAL
Thanks for cleaning this up! https://codereview.chromium.org/361083002/diff/20001/net/quic/quic_in_memory_... File net/quic/quic_in_memory_cache.h (right): https://codereview.chromium.org/361083002/diff/20001/net/quic/quic_in_memory_... net/quic/quic_in_memory_cache.h:47: const HttpResponseHeaders* headers() const { return headers_.get(); } Why did this change? https://codereview.chromium.org/361083002/diff/20001/net/quic/quic_spdy_serve... File net/quic/quic_spdy_server_stream.cc (right): https://codereview.chromium.org/361083002/diff/20001/net/quic/quic_spdy_serve... net/quic/quic_spdy_server_stream.cc:35: if (data_len > INT_MAX) { Is INT_MAX the right type here, since int *might* be 64 bits? (What does the internal code do?)
Patch set 2 LGTM. I am no longer in favor of changing the return type of headers() from a reference to a pointer, unless it comes with a null pointer check. https://codereview.chromium.org/361083002/diff/1/net/quic/quic_in_memory_cache.h File net/quic/quic_in_memory_cache.h (right): https://codereview.chromium.org/361083002/diff/1/net/quic/quic_in_memory_cach... net/quic/quic_in_memory_cache.h:21: NET_EXPORT_PRIVATE extern base::FilePath::StringType* The order probably doesn't matter, but please put "extern" before "NET_EXPORT_PRIVATE". See the Microsoft example code at http://msdn.microsoft.com/en-us/library/y4h7bcy6.aspx Update: I see that you've reverted the two changes here (adding NET_EXPORT_PRIVATE and changing g_quic_in_memory_cache_dir to a pointer). https://codereview.chromium.org/361083002/diff/20001/net/quic/quic_spdy_serve... File net/quic/quic_spdy_server_stream.cc (right): https://codereview.chromium.org/361083002/diff/20001/net/quic/quic_spdy_serve... net/quic/quic_spdy_server_stream.cc:35: if (data_len > INT_MAX) { On 2014/07/02 19:31:31, Ryan Hamilton wrote: > Is INT_MAX the right type here, since int *might* be 64 bits? (What does the > internal code do?) This protects the cast of 'data_len' to 'int' on line 42. https://codereview.chromium.org/361083002/diff/20001/net/quic/quic_spdy_serve... net/quic/quic_spdy_server_stream.cc:42: while (read_buf_->RemainingCapacity() < (int)data_len) { Please take the opportunity to change this to static_cast<int>(data_len) https://codereview.chromium.org/361083002/diff/20001/net/quic/quic_spdy_serve... net/quic/quic_spdy_server_stream.cc:132: SendHeadersAndBody(*response->headers(), response->body()); Hmm... If we are still dereferencing response->headers() without checking for a null pointer, then the change of response->headers() from a reference to a pointer isn't useful.
PTAL https://codereview.chromium.org/361083002/diff/20001/net/quic/quic_in_memory_... File net/quic/quic_in_memory_cache.h (right): https://codereview.chromium.org/361083002/diff/20001/net/quic/quic_in_memory_... net/quic/quic_in_memory_cache.h:47: const HttpResponseHeaders* headers() const { return headers_.get(); } On 2014/07/02 19:31:31, Ryan Hamilton wrote: > Why did this change? Because headers_ might be null. However, I'm undoing the change (as Wan-Teh suggested) because in that case, the caller will already have looked at the response_type() and not call headers() anyway. https://codereview.chromium.org/361083002/diff/20001/net/quic/quic_spdy_serve... File net/quic/quic_spdy_server_stream.cc (right): https://codereview.chromium.org/361083002/diff/20001/net/quic/quic_spdy_serve... net/quic/quic_spdy_server_stream.cc:42: while (read_buf_->RemainingCapacity() < (int)data_len) { On 2014/07/02 19:39:08, wtc wrote: > > Please take the opportunity to change this to static_cast<int>(data_len) Done. https://codereview.chromium.org/361083002/diff/20001/net/quic/quic_spdy_serve... net/quic/quic_spdy_server_stream.cc:132: SendHeadersAndBody(*response->headers(), response->body()); On 2014/07/02 19:39:08, wtc wrote: > > Hmm... If we are still dereferencing response->headers() without checking for a > null pointer, then the change of response->headers() from a reference to a > pointer isn't useful. Alright, I've undone the change.
Patch set 3 LGTM.
The CQ bit was checked by dmziegler@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmziegler@chromium.org/361083002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26724) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/31159)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26739)
The CQ bit was checked by dmziegler@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmziegler@chromium.org/361083002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 282275 |
