Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
123383002:
Enable SDCH support over HTTPS. (Closed)
|
Created:
6 years, 11 months ago by mef Modified:
6 years, 11 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnable SDCH support over HTTPS if --enable-sdch=2 switch is present.
BUG=313716
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243957
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244217
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address jar's comments. #Patch Set 3 : Add SdchFilter unit tests. #
Total comments: 5
Patch Set 4 : Sync to r243587 #Patch Set 5 : Address codereview comments. #
Messages
Total messages: 16 (0 generated)
Hi Jim, please take a look. It appears to use SDCH over HTTPS for searches on google.com. thanks, -m
Mostly just naming and comment nits. https://codereview.chromium.org/123383002/diff/1/chrome/browser/chrome_browse... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/123383002/diff/1/chrome/browser/chrome_browse... chrome/browser/chrome_browser_main.cc:1393: // Disable SDCH filtering if switches::kEnableSdch is 0. Nit: Change comment to something more like: // SDCH options via switches::kEnableSdch include: https://codereview.chromium.org/123383002/diff/1/chrome/browser/chrome_browse... chrome/browser/chrome_browser_main.cc:1395: const int kSdchEnabled = 1; nit: kSdchOverHttpEnabled https://codereview.chromium.org/123383002/diff/1/chrome/browser/chrome_browse... chrome/browser/chrome_browser_main.cc:1396: const int kSdchOverSecureEnabled = 2; nit: kSdchOverBothHttpAndHttpsEnabled https://codereview.chromium.org/123383002/diff/1/chrome/browser/chrome_browse... chrome/browser/chrome_browser_main.cc:1403: if (sdch_enabled == kSdchOverSecureEnabled) nit: else if https://codereview.chromium.org/123383002/diff/1/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/123383002/diff/1/net/base/sdch_manager.cc#new... net/base/sdch_manager.cc:68: 4. The request is not an HTTPS request. nit: please update the comment with something like: [We can now override (ignore) item (4) only when we have explicitly enabled HTTPS support]. https://codereview.chromium.org/123383002/diff/1/net/base/sdch_manager.h File net/base/sdch_manager.h (right): https://codereview.chromium.org/123383002/diff/1/net/base/sdch_manager.h#newc... net/base/sdch_manager.h:255: static void EnableSdchOverSecureSupport(bool enabled); nit: This is already in the SdchManager class... so a better name might be: EnableSecureSchemeSupport https://codereview.chromium.org/123383002/diff/1/net/base/sdch_manager.h#newc... net/base/sdch_manager.h:257: static bool sdch_over_secure_enabled() { return g_sdch_over_secure_enabled_; } nit: secure_scheme_supported make this a const method. https://codereview.chromium.org/123383002/diff/1/net/base/sdch_manager.h#newc... net/base/sdch_manager.h:352: // Support SDCH compression over secure connection, by advertising in headers. nit: Better might be g_secure_scheme_support_ better comment: Support SDCH compression for HTTPS requests and responses. When supported, HTTPS applicable dictionaries MUST have been acquired securely via HTTPS.
Hi Jim, thanks for your comments, PTAL. https://codereview.chromium.org/123383002/diff/1/chrome/browser/chrome_browse... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/123383002/diff/1/chrome/browser/chrome_browse... chrome/browser/chrome_browser_main.cc:1393: // Disable SDCH filtering if switches::kEnableSdch is 0. On 2014/01/03 18:35:13, jar wrote: > Nit: Change comment to something more like: > > // SDCH options via switches::kEnableSdch include: Done. https://codereview.chromium.org/123383002/diff/1/chrome/browser/chrome_browse... chrome/browser/chrome_browser_main.cc:1395: const int kSdchEnabled = 1; On 2014/01/03 18:35:13, jar wrote: > nit: kSdchOverHttpEnabled Done. https://codereview.chromium.org/123383002/diff/1/chrome/browser/chrome_browse... chrome/browser/chrome_browser_main.cc:1396: const int kSdchOverSecureEnabled = 2; On 2014/01/03 18:35:13, jar wrote: > nit: kSdchOverBothHttpAndHttpsEnabled Done. https://codereview.chromium.org/123383002/diff/1/chrome/browser/chrome_browse... chrome/browser/chrome_browser_main.cc:1403: if (sdch_enabled == kSdchOverSecureEnabled) On 2014/01/03 18:35:13, jar wrote: > nit: > else if Done. https://codereview.chromium.org/123383002/diff/1/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/123383002/diff/1/net/base/sdch_manager.cc#new... net/base/sdch_manager.cc:68: 4. The request is not an HTTPS request. On 2014/01/03 18:35:13, jar wrote: > nit: please update the comment with something like: > > [We can now override (ignore) item (4) only when we have explicitly enabled > HTTPS support]. Done. https://codereview.chromium.org/123383002/diff/1/net/base/sdch_manager.h File net/base/sdch_manager.h (right): https://codereview.chromium.org/123383002/diff/1/net/base/sdch_manager.h#newc... net/base/sdch_manager.h:255: static void EnableSdchOverSecureSupport(bool enabled); On 2014/01/03 18:35:13, jar wrote: > nit: This is already in the SdchManager class... so a better name might be: > > EnableSecureSchemeSupport > Done. https://codereview.chromium.org/123383002/diff/1/net/base/sdch_manager.h#newc... net/base/sdch_manager.h:257: static bool sdch_over_secure_enabled() { return g_sdch_over_secure_enabled_; } On 2014/01/03 18:35:13, jar wrote: > nit: secure_scheme_supported Done. > make this a const method. It's static, so it can't be const. https://codereview.chromium.org/123383002/diff/1/net/base/sdch_manager.h#newc... net/base/sdch_manager.h:352: // Support SDCH compression over secure connection, by advertising in headers. On 2014/01/03 18:35:13, jar wrote: > nit: Better might be g_secure_scheme_support_ > > better comment: > > Support SDCH compression for HTTPS requests and responses. When supported, HTTPS > applicable dictionaries MUST have been acquired securely via HTTPS. Done.
Thanks, the changes all LG. Please add test(s) to net/base/sdch_filter_unittest.cc for the new functionality. Specifically, have tests that relate to the HTTPS blockade, showing that SDCH works on HTTPS when properly configured. You should also test that that the dictionary *must* be fetched via HTTPS for it to be used with HTTPS content.
On 2014/01/03 20:58:35, jar wrote: > Thanks, the changes all LG. Excellent! > Please add test(s) to net/base/sdch_filter_unittest.cc for the new > functionality. Sure, I didn't realize that sdch_filter_unittest also contains sdch_manager tests. Would it make sense to separate them out into sdch_manager_unittest.cc? > Specifically, have tests that relate to the HTTPS blockade, showing that SDCH > works on HTTPS when properly configured. Any suggestions on how to test that? Does it need a new SdchFilter test, or SdchManager tests would be enough? > You should also test that that the dictionary *must* be fetched via HTTPS for it > to be used with HTTPS content. Done.
On 2014/01/06 21:13:45, mef wrote: > On 2014/01/03 20:58:35, jar wrote: > > Thanks, the changes all LG. > Excellent! > > > Please add test(s) to net/base/sdch_filter_unittest.cc for the new > > functionality. > Sure, I didn't realize that sdch_filter_unittest also contains sdch_manager > tests. > Would it make sense to separate them out into sdch_manager_unittest.cc? > > > Specifically, have tests that relate to the HTTPS blockade, showing that SDCH > > works on HTTPS when properly configured. > Any suggestions on how to test that? Does it need a new SdchFilter test, or > SdchManager tests would be enough? > > > You should also test that that the dictionary *must* be fetched via HTTPS for > it > > to be used with HTTPS content. > Done. If you were confused by the lack of an sdch_manager_unittest file, it sounds like a good idea to refactor some of the tests into such, and include your new test there. If you don't think it is too confusing, you can just use the sdch_filter_unittest.cc I think the refactor is probably the better plan.... and I expect there to be needs for more tests over time as this module grows.
Hi Jim, PTAL. I've added some tests for HTTP/HTTPS dictionary advertisement and usage. Couple of questions: - There seem to be a lot of repetitive code in those tests. Should I factor it out? - Do I need to add tests for actual usage of SdchFilter (like BasicDictionary test) over HTTPS? I didn't change the filter code... I'd prefer to refactor sdch_filter_unittest as a separate CL. thanks, -m
Hi Lei, could you OWNER-review chrome/browser/chrome_browser_main.cc? I've added new value to --enable-sdch command line switch. thanks, -m
lgtm with jar's blessings. https://codereview.chromium.org/123383002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/123383002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1393: // SDCH options via switches::kEnableSdch include: nit: you can declare these inside the if() block below since there are only used in there. https://codereview.chromium.org/123383002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1399: base::StringToInt(parsed_command_line().GetSwitchValueASCII( If you care about 'imperfect conversions' as defined in base/strings/string_number_conversions.h for StringToInt(), then you'd want to check the return value.
LGTM % nit below. https://codereview.chromium.org/123383002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/123383002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1393: // SDCH options via switches::kEnableSdch include: On 2014/01/08 19:42:23, Lei Zhang wrote: > nit: you can declare these inside the if() block below since there are only used > in there. +1
Thanks! Jim, just to confirm - does your LGTM cover unit tests as well? https://codereview.chromium.org/123383002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/123383002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1393: // SDCH options via switches::kEnableSdch include: On 2014/01/08 19:42:23, Lei Zhang wrote: > nit: you can declare these inside the if() block below since there are only used > in there. Done. https://codereview.chromium.org/123383002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1399: base::StringToInt(parsed_command_line().GetSwitchValueASCII( On 2014/01/08 19:42:23, Lei Zhang wrote: > If you care about 'imperfect conversions' as defined in > base/strings/string_number_conversions.h for StringToInt(), then you'd want to > check the return value. Done.
On 2014/01/09 16:23:53, mef wrote: > Thanks! > > Jim, just to confirm - does your LGTM cover unit tests as well? > > https://codereview.chromium.org/123383002/diff/170001/chrome/browser/chrome_b... > File chrome/browser/chrome_browser_main.cc (right): > > https://codereview.chromium.org/123383002/diff/170001/chrome/browser/chrome_b... > chrome/browser/chrome_browser_main.cc:1393: // SDCH options via > switches::kEnableSdch include: > On 2014/01/08 19:42:23, Lei Zhang wrote: > > nit: you can declare these inside the if() block below since there are only > used > > in there. > > Done. > > https://codereview.chromium.org/123383002/diff/170001/chrome/browser/chrome_b... > chrome/browser/chrome_browser_main.cc:1399: > base::StringToInt(parsed_command_line().GetSwitchValueASCII( > On 2014/01/08 19:42:23, Lei Zhang wrote: > > If you care about 'imperfect conversions' as defined in > > base/strings/string_number_conversions.h for StringToInt(), then you'd want to > > check the return value. > > Done. Yes, the LGTM was specifically made in light of the unit tests. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/123383002/310001
Message was sent while issue was closed.
Change committed as 243957
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/123383002/310001
Message was sent while issue was closed.
Change committed as 244217 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
