Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
211983002:
Add logging for distinguishing between captive portals with HTTP and HTTPS login pages (Closed)
|
Created:
6 years, 9 months ago by meacer Modified:
6 years, 9 months ago CC:
chromium-reviews, jar (doing other things), asvitkine+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd logging for distinguishing between captive portals with HTTP and HTTPS login pages
BUG=349737
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260237
Patch Set 1 #
Total comments: 2
Patch Set 2 : Record the landing url for captive portal probe instead of just the https status #
Total comments: 4
Patch Set 3 : mmenke comments #
Total comments: 4
Patch Set 4 : mmenke comment #Patch Set 5 : namespace fix #
Total comments: 2
Patch Set 6 : Style fix #
Total comments: 4
Patch Set 7 : isherman comments #
Total comments: 2
Patch Set 8 : Histogram naming #Patch Set 9 : Fix builds #
Messages
Total messages: 28 (0 generated)
Hi Matt, can you please take a look? Thanks.
https://codereview.chromium.org/211983002/diff/1/chrome/browser/captive_porta... File chrome/browser/captive_portal/captive_portal_detector.h (right): https://codereview.chromium.org/211983002/diff/1/chrome/browser/captive_porta... chrome/browser/captive_portal/captive_portal_detector.h:48: bool is_response_https; This should be was_redirected_to_https, since the initial response is always HTTP. However, I think it would make more sense to just record the final URL, and let the consumer decide what to do with it - this is more flexible for future logic (Could, for instance, open the new tab at this URL, rather than following the redirects)
https://codereview.chromium.org/211983002/diff/1/chrome/browser/captive_porta... File chrome/browser/captive_portal/captive_portal_detector.h (right): https://codereview.chromium.org/211983002/diff/1/chrome/browser/captive_porta... chrome/browser/captive_portal/captive_portal_detector.h:48: bool is_response_https; On 2014/03/26 14:46:08, mmenke wrote: > This should be was_redirected_to_https, since the initial response is always > HTTP. > > However, I think it would make more sense to just record the final URL, and let > the consumer decide what to do with it - this is more flexible for future logic > (Could, for instance, open the new tab at this URL, rather than following the > redirects) Done.
https://codereview.chromium.org/211983002/diff/70002/chrome/browser/captive_p... File chrome/browser/captive_portal/captive_portal_service.cc (right): https://codereview.chromium.org/211983002/diff/70002/chrome/browser/captive_p... chrome/browser/captive_portal/captive_portal_service.cc:222: RESULT_COUNT + 2); The +2 strikes me as fragile. If this histogram isn't going to be long lived, I'm fine with this, but otherwise, should probably do something more robust, like having another enum and mapping results manually. https://codereview.chromium.org/211983002/diff/70002/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/211983002/diff/70002/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1755: + <owner>mmenke@chromium.org</owner> Should I really be the owner of this? If we were recording the results in the captive_portal files, I'd be fine with that, but given that this is living in the SSL interstitial code, I don't think I'm the right person.
https://codereview.chromium.org/211983002/diff/70002/chrome/browser/captive_p... File chrome/browser/captive_portal/captive_portal_service.cc (right): https://codereview.chromium.org/211983002/diff/70002/chrome/browser/captive_p... chrome/browser/captive_portal/captive_portal_service.cc:222: RESULT_COUNT + 2); On 2014/03/26 17:23:21, mmenke wrote: > The +2 strikes me as fragile. If this histogram isn't going to be long lived, > I'm fine with this, but otherwise, should probably do something more robust, > like having another enum and mapping results manually. I guess we can keep it for some time, so I extracted it into something a bit more robust. https://codereview.chromium.org/211983002/diff/70002/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/211983002/diff/70002/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1755: + <owner>mmenke@chromium.org</owner> On 2014/03/26 17:23:21, mmenke wrote: > Should I really be the owner of this? If we were recording the results in the > captive_portal files, I'd be fine with that, but given that this is living in > the SSL interstitial code, I don't think I'm the right person. Sure, I can be the owner. I added you since I thought you were the initial implementor.
https://codereview.chromium.org/211983002/diff/90001/chrome/browser/captive_p... File chrome/browser/captive_portal/captive_portal_service.cc (right): https://codereview.chromium.org/211983002/diff/90001/chrome/browser/captive_p... chrome/browser/captive_portal/captive_portal_service.cc:42: }; This should be in the anonymous namespace. https://codereview.chromium.org/211983002/diff/90001/chrome/browser/captive_p... chrome/browser/captive_portal/captive_portal_service.cc:90: bool is_https = results.landing_url.SchemeIs("https"); Should use a switch on results.result. It's more code, but if you don't leave a default case, compilation will fail if a new result is added - that's my main concern here.
https://codereview.chromium.org/211983002/diff/90001/chrome/browser/captive_p... File chrome/browser/captive_portal/captive_portal_service.cc (right): https://codereview.chromium.org/211983002/diff/90001/chrome/browser/captive_p... chrome/browser/captive_portal/captive_portal_service.cc:42: }; On 2014/03/26 18:05:36, mmenke wrote: > This should be in the anonymous namespace. Done. https://codereview.chromium.org/211983002/diff/90001/chrome/browser/captive_p... chrome/browser/captive_portal/captive_portal_service.cc:90: bool is_https = results.landing_url.SchemeIs("https"); On 2014/03/26 18:05:36, mmenke wrote: > Should use a switch on results.result. It's more code, but if you don't leave a > default case, compilation will fail if a new result is added - that's my main > concern here. Done.
LGTM. https://codereview.chromium.org/211983002/diff/130001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_service.cc (right): https://codereview.chromium.org/211983002/diff/130001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_service.cc:101: : DETECTION_RESULT_BEHIND_CAPTIVE_PORTAL; optional nit: It's more common on chrome to put binary operators on the same line as the first argument than the second. I assume this applies to trinary operators as well.
Ilya, can you please take a look at histograms.xml? Thanks. https://codereview.chromium.org/211983002/diff/130001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_service.cc (right): https://codereview.chromium.org/211983002/diff/130001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_service.cc:101: : DETECTION_RESULT_BEHIND_CAPTIVE_PORTAL; On 2014/03/26 19:11:27, mmenke wrote: > optional nit: It's more common on chrome to put binary operators on the same > line as the first argument than the second. I assume this applies to trinary > operators as well. I looked this up when writing the code, and it seems to be a mix: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ref... puts the operator in separate lines. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... puts them in the same line.
On 2014/03/26 19:22:48, Mustafa Emre Acer wrote: > Ilya, can you please take a look at histograms.xml? Thanks. > > https://codereview.chromium.org/211983002/diff/130001/chrome/browser/captive_... > File chrome/browser/captive_portal/captive_portal_service.cc (right): > > https://codereview.chromium.org/211983002/diff/130001/chrome/browser/captive_... > chrome/browser/captive_portal/captive_portal_service.cc:101: : > DETECTION_RESULT_BEHIND_CAPTIVE_PORTAL; > On 2014/03/26 19:11:27, mmenke wrote: > > optional nit: It's more common on chrome to put binary operators on the same > > line as the first argument than the second. I assume this applies to trinary > > operators as well. > > I looked this up when writing the code, and it seems to be a mix: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ref... > puts the operator in separate lines. > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > puts them in the same line. Putting them on the first line looks to be about 8 times more common in src/chrome, if you believe these crude searches and file counts as opposed to line counts: https://code.google.com/p/chromium/codesearch#search/&q=%5C%20%5C?$%20file:sr... https://code.google.com/p/chromium/codesearch#search/&q=%5C%20%5C%20%5C%20%5C...
> Putting them on the first line looks to be about 8 times more common in > src/chrome, if you believe these crude searches and file counts as opposed to > line counts: Done.
Please first move the existing histogram definition over from the internal file, so that the diff relative to the existing code is clearer. https://codereview.chromium.org/211983002/diff/150001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_service.cc (right): https://codereview.chromium.org/211983002/diff/150001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_service.cc:31: // in histograms.xml. Please also document that this enum should be append-only. https://codereview.chromium.org/211983002/diff/150001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/211983002/diff/150001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29801: + </int> nit: Unless you're really attached to mouse-over tooltip text, I'd recommend having either the human-readable text (prefered, IMO) or the ENUM_VALUE_NAME text, but not both. Up to you, though.
> Please first move the existing histogram definition over from the internal file, > so that the diff relative to the existing code is clearer. Not sure I understand which definition you are referring to. Could you clarify? https://codereview.chromium.org/211983002/diff/150001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_service.cc (right): https://codereview.chromium.org/211983002/diff/150001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_service.cc:31: // in histograms.xml. On 2014/03/27 00:32:52, Ilya Sherman wrote: > Please also document that this enum should be append-only. Done. https://codereview.chromium.org/211983002/diff/150001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/211983002/diff/150001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29801: + </int> On 2014/03/27 00:32:52, Ilya Sherman wrote: > nit: Unless you're really attached to mouse-over tooltip text, I'd recommend > having either the human-readable text (prefered, IMO) or the ENUM_VALUE_NAME > text, but not both. Up to you, though. I removed the human readable text as the enums would be more familiar by this point (this is an existing histogram). A quick code search of the enums will bring up the comments with the human readable text.
On 2014/03/27 00:52:51, Mustafa Emre Acer wrote: > > Please first move the existing histogram definition over from the internal > file, > > so that the diff relative to the existing code is clearer. > > Not sure I understand which definition you are referring to. Could you clarify? You're modifying an existing histogram, but the diff makes it look like you're adding a new one. That's because the existing histogram is in //tools/histograms/histograms.xml (in src-internal), whereas you're modifying //tools/metrics/histograms/histograms.xml (in the public repo). I'm asking you to move the existing histogram out of src-internal first, then land these changes.
On 2014/03/27 00:54:38, Ilya Sherman wrote: > On 2014/03/27 00:52:51, Mustafa Emre Acer wrote: > > > Please first move the existing histogram definition over from the internal > > file, > > > so that the diff relative to the existing code is clearer. > > > > Not sure I understand which definition you are referring to. Could you > clarify? > > You're modifying an existing histogram, but the diff makes it look like you're > adding a new one. That's because the existing histogram is in > //tools/histograms/histograms.xml (in src-internal), whereas you're modifying > //tools/metrics/histograms/histograms.xml (in the public repo). I'm asking you > to move the existing histogram out of src-internal first, then land these > changes. Ah thanks, I wasn't aware of the internal version of the histogram. Sent a CL to src-internal.
histograms lgtm https://codereview.chromium.org/211983002/diff/160001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/211983002/diff/160001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29804: + label="DETECTION_RESULT_BEHIND_CAPTIVE_PORTAL_HTTPS_LANDING_URL"/> optional nit: Maybe omit the "DETECTION_RESULT_" prefix for all of these?
https://codereview.chromium.org/211983002/diff/160001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/211983002/diff/160001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29804: + label="DETECTION_RESULT_BEHIND_CAPTIVE_PORTAL_HTTPS_LANDING_URL"/> On 2014/03/27 19:00:53, Ilya Sherman wrote: > optional nit: Maybe omit the "DETECTION_RESULT_" prefix for all of these? Done.
The CQ bit was checked by meacer@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/meacer@chromium.org/211983002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
The CQ bit was checked by meacer@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/meacer@chromium.org/211983002/190001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, base_unittests, base_unittests_swarm, browser_tests, browser_tests_swarm, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests, interactive_ui_tests, interactive_ui_tests_swarm, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, net_unittests, net_unittests_swarm, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, unit_tests, unit_tests_swarm, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by meacer@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/meacer@chromium.org/211983002/190001
Message was sent while issue was closed.
Change committed as 260237 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
