Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
400323002:
Refactor the captive portal code to move from the ssl_blocking_page class to the ssl_error_classific (Closed)
|
Created:
6 years, 5 months ago by radhikabhar Modified:
6 years, 3 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRefactor the captive portal code to move from the ssl_blocking_page class to the ssl_error_classification class.
A function is also added to check the type of the network the user is connected to.
BUG=395295
Patch Set 1 #Patch Set 2 : Removed extraneous code #Patch Set 3 : Added comments #
Total comments: 20
Patch Set 4 : Addressed comments #
Total comments: 11
Patch Set 5 : Rebase Update #Patch Set 6 : Addressed Comments #
Total comments: 2
Patch Set 7 : Added function call #
Total comments: 3
Patch Set 8 : Removed Web_Contents() #
Total comments: 8
Patch Set 9 : Updates #
Total comments: 2
Patch Set 10 : Changes #Patch Set 11 : Rebase-update #Patch Set 12 : Call the function from the Observe function #
Total comments: 1
Patch Set 13 : Moved the histogram stuff #
Total comments: 8
Patch Set 14 : Addressed Comments #Patch Set 15 : Rebase-Update #Patch Set 16 : Addressed Comments #Patch Set 17 : Removed arguments from functions #
Total comments: 7
Patch Set 18 : Addressed Comments #
Total comments: 1
Patch Set 19 : Fixed typo #
Total comments: 9
Patch Set 20 : Addressed comments #
Total comments: 2
Patch Set 21 : Addressed Comments #Patch Set 22 : Rebase-Update #
Total comments: 5
Patch Set 23 : Fixed typo #Patch Set 24 : Rebase-Update #Patch Set 25 : Addressed comments for the histogram.xml file #
Total comments: 8
Patch Set 26 : Addressed Comments #
Total comments: 5
Patch Set 27 : Addressed Comments #Patch Set 28 : Fixed compilation errors #Patch Set 29 : Rebase-Update #Patch Set 30 : Rebase-Update #Messages
Total messages: 85 (0 generated)
@meacer - Can you please take a look at the captive portal code. I also added an histogram only for the Captive portal and made you as the owner. @felt - Can you please take a look at the function CalculateScoreEnvironmetns() on line 189?
FYI: it looks like some of the changes from 376333003 have slipped into this CL. https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:119: CAPTIVE_PORTAL_DETECTION_ENABLED, // Captive Portal errors moved to Can you rename them like: DEPRECATED_CAPTIVE_PORTAL_DETECTION_ENABLED, https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:244: SSLErrorInfo::CERT_DATE_INVALID) not a single line, so it needs { } (if i missed this in the other CL, it needs it there too) https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.cc:38: enum SSLBlockingPageCaptivePortal { SSLInterstitialCauseCaptivePortal maybe? https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.cc:65: UMA_HISTOGRAM_ENUMERATION("interstitial.ssl.captiveportal", interstitial.ssl.captive_portal I think would be in line with how they do the naming
https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:119: CAPTIVE_PORTAL_DETECTION_ENABLED, // Captive Portal errors moved to I think it makes sense to put captive portal stuff into a new enum. https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.cc:201: type == net::NetworkChangeNotifier::CONNECTION_4G) Multiline ifs should have braces {} https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.h:23: const::net::X509Certificate& cert); You don't need :: before net here, do you? const net::X509Certificate should be OK. https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.h:34: void CaptivePortalDetect(bool record_histograms, Maybe I'm missing something but I can't see the implementation for this method in ssl_error_classification.cc? https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.h:42: void RecordUMAStatisticsCaptivePortals(bool overridable); nit: Maybe rename to RecordCaptivePortalUMAStatistics? https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.h:57: content::WebContents& web_contents_; I think you should make this a pointer. The majority of the code keeps web_contents as a pointer.
https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:119: CAPTIVE_PORTAL_DETECTION_ENABLED, // Captive Portal errors moved to On 2014/07/18 23:53:42, felt wrote: > Can you rename them like: > > DEPRECATED_CAPTIVE_PORTAL_DETECTION_ENABLED, Done. https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:119: CAPTIVE_PORTAL_DETECTION_ENABLED, // Captive Portal errors moved to On 2014/07/19 00:06:45, Mustafa Emre Acer wrote: > I think it makes sense to put captive portal stuff into a new enum. I have already created a new enum in the ssl_error_classification and in the histogram.xml - should I just delete the captive portal stuff from this file as well as remove it from the enum in the histogram.xml file? https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:244: SSLErrorInfo::CERT_DATE_INVALID) On 2014/07/18 23:53:42, felt wrote: > not a single line, so it needs { } > (if i missed this in the other CL, it needs it there too) Done. https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.cc:38: enum SSLBlockingPageCaptivePortal { On 2014/07/18 23:53:42, felt wrote: > SSLInterstitialCauseCaptivePortal maybe? Done. https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.h:23: const::net::X509Certificate& cert); On 2014/07/19 00:06:45, Mustafa Emre Acer wrote: > You don't need :: before net here, do you? > > const net::X509Certificate should be OK. Done. https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.h:34: void CaptivePortalDetect(bool record_histograms, On 2014/07/19 00:06:45, Mustafa Emre Acer wrote: > Maybe I'm missing something but I can't see the implementation for this method > in ssl_error_classification.cc? My bad. I had this function previously then deleted it as I no longer needed it. Forgot to delete it here. https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.h:42: void RecordUMAStatisticsCaptivePortals(bool overridable); On 2014/07/19 00:06:45, Mustafa Emre Acer wrote: > nit: Maybe rename to RecordCaptivePortalUMAStatistics? Done.
https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:119: CAPTIVE_PORTAL_DETECTION_ENABLED, // Captive Portal errors moved to On 2014/07/21 16:05:11, radhikabhar wrote: > On 2014/07/19 00:06:45, Mustafa Emre Acer wrote: > > I think it makes sense to put captive portal stuff into a new enum. > > I have already created a new enum in the ssl_error_classification and in the > histogram.xml - should I just delete the captive portal stuff from this file as > well as remove it from the enum in the histogram.xml file? I think you have done it the correct way now. Msutafa might have missed that you created the new enum. We can't delete the captive portal entries in the enum or else it will screw up the UMA dashboards. All we can do is mark them as deprecated (which it looks like you've already done).
> I think you have done it the correct way now. Msutafa might have missed that you > created the new enum. We can't delete the captive portal entries in the enum or > else it will screw up the UMA dashboards. All we can do is mark them as > deprecated (which it looks like you've already done). Yes, sorry I missed it. https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:126: DEPRECATED_CAPTIVE_PORTAL_DETECTED_OVERRIDABLE, histograms.xml have entries for the deprecated enums. I think you'll need to mark them as deprecated as well. https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:671: const content::NotificationDetails& details) { I don't think this is needed anymore, it just records some bools for UMA and that logic is already handled in ssl_error_classification. Looks like you could remove this method and all captive portal related bools from this class? https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_e... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.cc:165: extra line https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_e... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.h:37: void RecordUMAStatistics(bool overridable); void RecordUMAStatistics(bool overridable) const ? https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.h:38: void RecordCaptivePortalUMAStatistics(bool overridable); const here too.
https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_e... File chrome/browser/ssl/ssl_error_classification_unittest.cc (right): https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification_unittest.cc:22: virtual void SetUp() OVERRIDE { I patched your test in locally and ran it. Your test requires a MessageLoopForIO backing the IO thread (the default TestThreadBundle option is to have one message loop backing all the different threads), so you need to call SetThreadBundleOptions(content::TestBrowserThreadBundle::REAL_IO_THREAD) in the constructor for your test fixture. This way, you get a real MessageLoopForIO backing the IO thread (rather than a MessageLoopForUI) and the CHECKs don't get triggered.
https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.cc:65: UMA_HISTOGRAM_ENUMERATION("interstitial.ssl.captiveportal", On 2014/07/18 23:53:42, felt wrote: > interstitial.ssl.captive_portal I think would be in line with how they do the > naming Done. https://codereview.chromium.org/400323002/diff/30001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.cc:201: type == net::NetworkChangeNotifier::CONNECTION_4G) On 2014/07/19 00:06:45, Mustafa Emre Acer wrote: > Multiline ifs should have braces {} Done. https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:126: DEPRECATED_CAPTIVE_PORTAL_DETECTED_OVERRIDABLE, On 2014/07/21 18:45:14, Mustafa Emre Acer wrote: > histograms.xml have entries for the deprecated enums. I think you'll need to > mark them as deprecated as well. Done. https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:671: const content::NotificationDetails& details) { On 2014/07/21 18:45:15, Mustafa Emre Acer wrote: > I don't think this is needed anymore, it just records some bools for UMA and > that logic is already handled in ssl_error_classification. Looks like you could > remove this method and all captive portal related bools from this class? > Done. https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_e... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.cc:165: On 2014/07/21 18:45:15, Mustafa Emre Acer wrote: > extra line Done. https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_e... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.h:37: void RecordUMAStatistics(bool overridable); On 2014/07/21 18:45:15, Mustafa Emre Acer wrote: > void RecordUMAStatistics(bool overridable) const ? Done. https://codereview.chromium.org/400323002/diff/90001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_classification.h:38: void RecordCaptivePortalUMAStatistics(bool overridable); On 2014/07/21 18:45:15, Mustafa Emre Acer wrote: > const here too. Done.
https://codereview.chromium.org/400323002/diff/280001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/280001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:88: content::Source<Profile>(profile)); I think you are missing a call to RecordCaptivePortalUMAStatistics here.
https://codereview.chromium.org/400323002/diff/280001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/280001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:88: content::Source<Profile>(profile)); On 2014/07/24 00:22:54, Mustafa Emre Acer wrote: > I think you are missing a call to RecordCaptivePortalUMAStatistics here. I added the function call in ssl_blocking_page.cc on line 325. Is it better to add the call to recorCaptivePortalUMAStatistics here or is it fine in the ssl_blocking_page.cc?
Lgtm https://codereview.chromium.org/400323002/diff/300001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/400323002/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:326: overridable_ && !strict_enforcement_); Maybe you can add a public method to SSLErrorClassification such as RecordUMA which calls RecordSSLUMAStatistics and RecordCaptivePortalUMAStatistics, and then call ssl_error_classification.RecordUMA() here? That way you don't have to check for ENABLE_CAPTIVE_PORTAL_DETECTION here, and you can do the SSLErrorInfo::NetErrorToErrorType(cert_error_)==SSLErrorInfo::CERT_DATE_INVALID check in the error handler as well. I don't have a strong preference though.
https://codereview.chromium.org/400323002/diff/300001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification_unittest.cc (right): https://codereview.chromium.org/400323002/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification_unittest.cc:28: web_contents_.reset(CreateTestWebContents()); CRVHTH::SetUp() already creates a web contents. Do you need a second one or can you just the default one?
https://codereview.chromium.org/400323002/diff/300001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification_unittest.cc (right): https://codereview.chromium.org/400323002/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification_unittest.cc:28: web_contents_.reset(CreateTestWebContents()); On 2014/07/24 00:46:40, dcheng (OOO) wrote: > CRVHTH::SetUp() already creates a web contents. Do you need a second one or can > you just the default one? Done.
On 2014/07/24 at 00:52:47, radhikabhar wrote: > https://codereview.chromium.org/400323002/diff/300001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_error_classification_unittest.cc (right): > > https://codereview.chromium.org/400323002/diff/300001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification_unittest.cc:28: web_contents_.reset(CreateTestWebContents()); > On 2014/07/24 00:46:40, dcheng (OOO) wrote: > > CRVHTH::SetUp() already creates a web contents. Do you need a second one or can > > you just the default one? > > Done. thanks. lgtm
https://codereview.chromium.org/400323002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:205: if (captive_portal_detected_) captive_portal_detected_ won't have been set yet unless ::Observe triggered. Does this need to wait to execute until ::Observe? or should there be a way of updating this when ::Observe runs? https://codereview.chromium.org/400323002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/400323002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:37: void RecordUMAStatistics(bool overridable) const; why is this method signature changing?
https://codereview.chromium.org/400323002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:205: if (captive_portal_detected_) On 2014/07/24 01:12:33, felt wrote: > captive_portal_detected_ won't have been set yet unless ::Observe triggered. > Does this need to wait to execute until ::Observe? or should there be a way of > updating this when ::Observe runs? Or wait 2 seconds to calculate this score? :-) But that's a good point. If you want to use this bool, you'll probably want to wait until Observe runs, or weigh this score with captive_portal_probe_completed_.
https://codereview.chromium.org/400323002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:205: if (captive_portal_detected_) On 2014/07/24 01:17:26, Mustafa Emre Acer wrote: > On 2014/07/24 01:12:33, felt wrote: > > captive_portal_detected_ won't have been set yet unless ::Observe triggered. > > Does this need to wait to execute until ::Observe? or should there be a way of > > updating this when ::Observe runs? > > Or wait 2 seconds to calculate this score? :-) > > But that's a good point. If you want to use this bool, you'll probably want to > wait until Observe runs, or weigh this score with > captive_portal_probe_completed_. Doesn't Observe run when we call the registrar.Add function? https://codereview.chromium.org/400323002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/400323002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:37: void RecordUMAStatistics(bool overridable) const; On 2014/07/24 01:12:33, felt wrote: > why is this method signature changing? This was done according to @meacer's comment
https://codereview.chromium.org/400323002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:205: if (captive_portal_detected_) On 2014/07/24 01:25:17, radhikabhar wrote: > On 2014/07/24 01:17:26, Mustafa Emre Acer wrote: > > On 2014/07/24 01:12:33, felt wrote: > > > captive_portal_detected_ won't have been set yet unless ::Observe triggered. > > > Does this need to wait to execute until ::Observe? or should there be a way > of > > > updating this when ::Observe runs? > > > > Or wait 2 seconds to calculate this score? :-) > > > > But that's a good point. If you want to use this bool, you'll probably want to > > wait until Observe runs, or weigh this score with > > captive_portal_probe_completed_. > > Doesn't Observe run when we call the registrar.Add function? > Hmm, I think I wanted to say "wait until captive_portal_probe_completed_ is set".
https://codereview.chromium.org/400323002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:205: if (captive_portal_detected_) On 2014/07/24 01:25:17, radhikabhar wrote: > On 2014/07/24 01:17:26, Mustafa Emre Acer wrote: > > On 2014/07/24 01:12:33, felt wrote: > > > captive_portal_detected_ won't have been set yet unless ::Observe triggered. > > > Does this need to wait to execute until ::Observe? or should there be a way > of > > > updating this when ::Observe runs? > > > > Or wait 2 seconds to calculate this score? :-) > > > > But that's a good point. If you want to use this bool, you'll probably want to > > wait until Observe runs, or weigh this score with > > captive_portal_probe_completed_. > > Doesn't Observe run when we call the registrar.Add function? > Observe should run when the notification comes, which might be a while after registrar.add.
https://codereview.chromium.org/400323002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:205: if (captive_portal_detected_) On 2014/07/24 01:51:08, felt wrote: > On 2014/07/24 01:25:17, radhikabhar wrote: > > On 2014/07/24 01:17:26, Mustafa Emre Acer wrote: > > > On 2014/07/24 01:12:33, felt wrote: > > > > captive_portal_detected_ won't have been set yet unless ::Observe > triggered. > > > > Does this need to wait to execute until ::Observe? or should there be a > way > > of > > > > updating this when ::Observe runs? > > > > > > Or wait 2 seconds to calculate this score? :-) > > > > > > But that's a good point. If you want to use this bool, you'll probably want > to > > > wait until Observe runs, or weigh this score with > > > captive_portal_probe_completed_. > > > > Doesn't Observe run when we call the registrar.Add function? > > > > Observe should run when the notification comes, which might be a while after > registrar.add. @felt - Will this be correct logic - if !captive_portal_probe_completed_ then wait for 2 seconds and then check whether captive_portal_detected?
> @felt - Will this be correct logic - if !captive_portal_probe_completed_ then > wait for 2 seconds and then check whether captive_portal_detected? Well, there's no guarantee that anything will happen if 2 seconds have passed. As an alternative, you could record this one in the ~SSLBlockingPage destructor perhaps... since that's the latest point in time you can check. https://codereview.chromium.org/400323002/diff/340001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:205: if (captive_portal_probe_completed_) { if (captive_portal_probe_complete_ && captive_portal_detected_)
https://codereview.chromium.org/400323002/diff/340001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:205: if (captive_portal_probe_completed_) { On 2014/07/25 00:25:22, felt wrote: > if (captive_portal_probe_complete_ && captive_portal_detected_) Done.
On 2014/07/25 16:35:32, radhikabhar wrote: > https://codereview.chromium.org/400323002/diff/340001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_error_classification.cc (right): > > https://codereview.chromium.org/400323002/diff/340001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.cc:205: if > (captive_portal_probe_completed_) { > On 2014/07/25 00:25:22, felt wrote: > > if (captive_portal_probe_complete_ && captive_portal_detected_) > > Done. did you miss my other comment suggesting that you could use the destructor?
On 2014/07/26 01:19:38, felt wrote: > On 2014/07/25 16:35:32, radhikabhar wrote: > > > https://codereview.chromium.org/400323002/diff/340001/chrome/browser/ssl/ssl_... > > File chrome/browser/ssl/ssl_error_classification.cc (right): > > > > > https://codereview.chromium.org/400323002/diff/340001/chrome/browser/ssl/ssl_... > > chrome/browser/ssl/ssl_error_classification.cc:205: if > > (captive_portal_probe_completed_) { > > On 2014/07/25 00:25:22, felt wrote: > > > if (captive_portal_probe_complete_ && captive_portal_detected_) > > > > Done. > > did you miss my other comment suggesting that you could use the destructor? Should I call the function for checking the type of network in the destructor of the ~SSLBlockingPage? I was planning on calling that function in the function CalculateSeverityScore for every type of error and adding the weight to the score but this would require being before the destructor. Or do we just want to record in UMA the type of the network for now?
On 2014/07/27 22:13:01, radhikabhar wrote: > On 2014/07/26 01:19:38, felt wrote: > > On 2014/07/25 16:35:32, radhikabhar wrote: > > > > > > https://codereview.chromium.org/400323002/diff/340001/chrome/browser/ssl/ssl_... > > > File chrome/browser/ssl/ssl_error_classification.cc (right): > > > > > > > > > https://codereview.chromium.org/400323002/diff/340001/chrome/browser/ssl/ssl_... > > > chrome/browser/ssl/ssl_error_classification.cc:205: if > > > (captive_portal_probe_completed_) { > > > On 2014/07/25 00:25:22, felt wrote: > > > > if (captive_portal_probe_complete_ && captive_portal_detected_) > > > > > > Done. > > > > did you miss my other comment suggesting that you could use the destructor? > > Should I call the function for checking the type of network in the destructor of > the ~SSLBlockingPage? I was planning on calling that function in the function > CalculateSeverityScore for every type of error and adding the weight to the > score but this would require being before the destructor. Or do we just want to > record in UMA the type of the network for now? The captive portal part isn't particularly meaningful unless you calculate it during destruction. Can you outline the options that you have, and the pros and cons of each?
On 2014/07/28 17:53:49, felt wrote: > On 2014/07/27 22:13:01, radhikabhar wrote: > > On 2014/07/26 01:19:38, felt wrote: > > > On 2014/07/25 16:35:32, radhikabhar wrote: > > > > > > > > > > https://codereview.chromium.org/400323002/diff/340001/chrome/browser/ssl/ssl_... > > > > File chrome/browser/ssl/ssl_error_classification.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/400323002/diff/340001/chrome/browser/ssl/ssl_... > > > > chrome/browser/ssl/ssl_error_classification.cc:205: if > > > > (captive_portal_probe_completed_) { > > > > On 2014/07/25 00:25:22, felt wrote: > > > > > if (captive_portal_probe_complete_ && captive_portal_detected_) > > > > > > > > Done. > > > > > > did you miss my other comment suggesting that you could use the destructor? > > > > Should I call the function for checking the type of network in the destructor > of > > the ~SSLBlockingPage? I was planning on calling that function in the function > > CalculateSeverityScore for every type of error and adding the weight to the > > score but this would require being before the destructor. Or do we just want > to > > record in UMA the type of the network for now? > > The captive portal part isn't particularly meaningful unless you calculate it > during destruction. Can you outline the options that you have, and the pros and > cons of each? @felt - Can you please take a look at the flow of the code and see if it makes sense? I also added a Histogram to record the severity score and made you as the owner.
https://codereview.chromium.org/400323002/diff/400001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:282: InvalidDateSeverityScore(); Since the function for CalculateScoreEnvironments is only going to be called either from InvaildDateSeverityScore or InvalidNameSeverityScore, it made more sense to call those functions here instead of Updating the score.
On 2014/07/28 23:02:36, radhikabhar wrote: > https://codereview.chromium.org/400323002/diff/400001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_error_classification.cc (right): > > https://codereview.chromium.org/400323002/diff/400001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.cc:282: InvalidDateSeverityScore(); > Since the function for CalculateScoreEnvironments is only going to be called > either from InvaildDateSeverityScore or InvalidNameSeverityScore, it made more > sense to call those functions here instead of Updating the score. Can you move the histogram stuff into a separate CL please?
On 2014/07/28 23:56:26, felt wrote: > On 2014/07/28 23:02:36, radhikabhar wrote: > > > https://codereview.chromium.org/400323002/diff/400001/chrome/browser/ssl/ssl_... > > File chrome/browser/ssl/ssl_error_classification.cc (right): > > > > > https://codereview.chromium.org/400323002/diff/400001/chrome/browser/ssl/ssl_... > > chrome/browser/ssl/ssl_error_classification.cc:282: > InvalidDateSeverityScore(); > > Since the function for CalculateScoreEnvironments is only going to be called > > either from InvaildDateSeverityScore or InvalidNameSeverityScore, it made more > > sense to call those functions here instead of Updating the score. > > Can you move the histogram stuff into a separate CL please? Just the severity score histogram stuff, I mean. The histogram code related to captive portals should stay.
On 2014/07/28 23:57:13, felt wrote: > On 2014/07/28 23:56:26, felt wrote: > > On 2014/07/28 23:02:36, radhikabhar wrote: > > > > > > https://codereview.chromium.org/400323002/diff/400001/chrome/browser/ssl/ssl_... > > > File chrome/browser/ssl/ssl_error_classification.cc (right): > > > > > > > > > https://codereview.chromium.org/400323002/diff/400001/chrome/browser/ssl/ssl_... > > > chrome/browser/ssl/ssl_error_classification.cc:282: > > InvalidDateSeverityScore(); > > > Since the function for CalculateScoreEnvironments is only going to be called > > > either from InvaildDateSeverityScore or InvalidNameSeverityScore, it made > more > > > sense to call those functions here instead of Updating the score. > > > > Can you move the histogram stuff into a separate CL please? > > Just the severity score histogram stuff, I mean. The histogram code related to > captive portals should stay. Done.
I'm confused about how this is supposed to work now, left a few comments. https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:328: #elif I'm confused about what's going on here: * Why is InvalidDateSeverityScore() only be checked if captive portal detection is disabled? * There is no conditional after the #elif Did you perhaps mean to move the #endif up from line 333 to line 328? https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:332: } Also, this if-statement seems redundant to the code on lines 310-322? https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:133: severity_date_score += kServerWeight * kNotYetValidWeight; This no longer has a return value, so what's the point of the method now? https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:191: float SSLErrorClassification::CalculateScoreEnvironments() const { I'm confused, I still don't see this being invoked anywhere.
https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:328: #elif On 2014/07/29 01:47:53, felt wrote: > I'm confused about what's going on here: > * Why is InvalidDateSeverityScore() only be checked if captive portal detection > is disabled? > * There is no conditional after the #elif > > Did you perhaps mean to move the #endif up from line 333 to line 328? * My logic was- #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION Call InvalidCommonNameSeverityScore in Observe() #else Call InvalidCommonNameSeverityScore #endif * Correct - It should only be else https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:332: } On 2014/07/29 01:47:53, felt wrote: > Also, this if-statement seems redundant to the code on lines 310-322? I was thinking way ahead into the CL for the common name. when that Cl lands then I have to call either InvalidDateSeverityScore or InvalidCommonNameSeverityScore, that's why the check out here, but it is not needed right now. removed it. https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:133: severity_date_score += kServerWeight * kNotYetValidWeight; On 2014/07/29 01:47:53, felt wrote: > This no longer has a return value, so what's the point of the method now? Again, I was thinking way ahead here when I have the histogram CL for the severity score here. Then I will record the score right here and I didn't need the severity score anywhere else so I don't need to return the score. https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:191: float SSLErrorClassification::CalculateScoreEnvironments() const { On 2014/07/29 01:47:53, felt wrote: > I'm confused, I still don't see this being invoked anywhere. This function only makes sense for the CERT_COMMON_NAME_INVALID error. So as soon as my CL lands for the common name errors I will add a call to this function in the InvalidCommonNameSeverityScore() function. There are too many dependencies on the other CL's. Maybe I should wait for the common name CL to land before landing this CL.
On 2014/07/29 17:56:56, radhikabhar wrote: > https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_blocking_page.cc:328: #elif > On 2014/07/29 01:47:53, felt wrote: > > I'm confused about what's going on here: > > * Why is InvalidDateSeverityScore() only be checked if captive portal > detection > > is disabled? > > * There is no conditional after the #elif > > > > Did you perhaps mean to move the #endif up from line 333 to line 328? > * My logic was- > #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION > Call InvalidCommonNameSeverityScore in Observe() > #else > Call InvalidCommonNameSeverityScore > #endif > > * Correct - It should only be else > > https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_blocking_page.cc:332: } > On 2014/07/29 01:47:53, felt wrote: > > Also, this if-statement seems redundant to the code on lines 310-322? > > I was thinking way ahead into the CL for the common name. when that Cl lands > then I have to call either InvalidDateSeverityScore or > InvalidCommonNameSeverityScore, that's why the check out here, but it is not > needed right now. removed it. > > https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_error_classification.cc (right): > > https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.cc:133: severity_date_score += > kServerWeight * kNotYetValidWeight; > On 2014/07/29 01:47:53, felt wrote: > > This no longer has a return value, so what's the point of the method now? > > Again, I was thinking way ahead here when I have the histogram CL for the > severity score here. Then I will record the score right here and I didn't need > the severity score anywhere else so I don't need to return the score. > > https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.cc:191: float > SSLErrorClassification::CalculateScoreEnvironments() const { > On 2014/07/29 01:47:53, felt wrote: > > I'm confused, I still don't see this being invoked anywhere. > > This function only makes sense for the CERT_COMMON_NAME_INVALID error. So as > soon as my CL lands for the common name errors I will add a call to this > function in the InvalidCommonNameSeverityScore() function. > > There are too many dependencies on the other CL's. Maybe I should wait for the > common name CL to land before landing this CL. Yeah, I am getting confused. Please wait for the common name CL to land, then rebase and ping this thread again for me to finish the review.
On 2014/07/29 23:16:36, felt wrote: > On 2014/07/29 17:56:56, radhikabhar wrote: > > > https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... > > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > > > > https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... > > chrome/browser/ssl/ssl_blocking_page.cc:328: #elif > > On 2014/07/29 01:47:53, felt wrote: > > > I'm confused about what's going on here: > > > * Why is InvalidDateSeverityScore() only be checked if captive portal > > detection > > > is disabled? > > > * There is no conditional after the #elif > > > > > > Did you perhaps mean to move the #endif up from line 333 to line 328? > > * My logic was- > > #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION > > Call InvalidCommonNameSeverityScore in Observe() > > #else > > Call InvalidCommonNameSeverityScore > > #endif > > > > * Correct - It should only be else > > > > > https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... > > chrome/browser/ssl/ssl_blocking_page.cc:332: } > > On 2014/07/29 01:47:53, felt wrote: > > > Also, this if-statement seems redundant to the code on lines 310-322? > > > > I was thinking way ahead into the CL for the common name. when that Cl lands > > then I have to call either InvalidDateSeverityScore or > > InvalidCommonNameSeverityScore, that's why the check out here, but it is not > > needed right now. removed it. > > > > > https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... > > File chrome/browser/ssl/ssl_error_classification.cc (right): > > > > > https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... > > chrome/browser/ssl/ssl_error_classification.cc:133: severity_date_score += > > kServerWeight * kNotYetValidWeight; > > On 2014/07/29 01:47:53, felt wrote: > > > This no longer has a return value, so what's the point of the method now? > > > > Again, I was thinking way ahead here when I have the histogram CL for the > > severity score here. Then I will record the score right here and I didn't need > > the severity score anywhere else so I don't need to return the score. > > > > > https://codereview.chromium.org/400323002/diff/420001/chrome/browser/ssl/ssl_... > > chrome/browser/ssl/ssl_error_classification.cc:191: float > > SSLErrorClassification::CalculateScoreEnvironments() const { > > On 2014/07/29 01:47:53, felt wrote: > > > I'm confused, I still don't see this being invoked anywhere. > > > > This function only makes sense for the CERT_COMMON_NAME_INVALID error. So as > > soon as my CL lands for the common name errors I will add a call to this > > function in the InvalidCommonNameSeverityScore() function. > > > > There are too many dependencies on the other CL's. Maybe I should wait for the > > common name CL to land before landing this CL. > > Yeah, I am getting confused. Please wait for the common name CL to land, then > rebase and ping this thread again for me to finish the review. @felt - Can you PTAL after the rebase update?
https://codereview.chromium.org/400323002/diff/560001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/400323002/diff/560001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:308: ssl_error_classification.InvalidDateSeverityScore(); It would make sense to specify in a comment that this is in the destructor because InvalidDateSeverityScore and InvalidCommonNameSeverityScore depend on knowing whether captive portal detection happened before the user made a decision. https://codereview.chromium.org/400323002/diff/560001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:309: break; With that being said, does it make sense to factor in captive portals in for date errors? It's extremely unlikely that a captive portal would cause a certificate to appear expired... I would expect it to be a name mismatch error. https://codereview.chromium.org/400323002/diff/560001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/560001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:109: captive_portal_detection_enabled_ = captive_portal_service ->enabled(); lines 109 and 110 both have extra spaces before the ->
https://codereview.chromium.org/400323002/diff/560001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/400323002/diff/560001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:308: ssl_error_classification.InvalidDateSeverityScore(); On 2014/08/07 23:17:10, felt wrote: > It would make sense to specify in a comment that this is in the destructor > because InvalidDateSeverityScore and InvalidCommonNameSeverityScore depend on > knowing whether captive portal detection happened before the user made a > decision. Done. https://codereview.chromium.org/400323002/diff/560001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:309: break; On 2014/08/07 23:17:10, felt wrote: > With that being said, does it make sense to factor in captive portals in for > date errors? It's extremely unlikely that a captive portal would cause a > certificate to appear expired... I would expect it to be a name mismatch error. No it doesn't. I just added it here for consistency sake because it doesn't matter when the date invalid error is called. Should I move it? https://codereview.chromium.org/400323002/diff/560001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/400323002/diff/560001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:109: captive_portal_detection_enabled_ = captive_portal_service ->enabled(); On 2014/08/07 23:17:10, felt wrote: > lines 109 and 110 both have extra spaces before the -> Done.
lgtm https://codereview.chromium.org/400323002/diff/560001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/400323002/diff/560001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:309: break; On 2014/08/07 23:25:22, radhikabhar wrote: > On 2014/08/07 23:17:10, felt wrote: > > With that being said, does it make sense to factor in captive portals in for > > date errors? It's extremely unlikely that a captive portal would cause a > > certificate to appear expired... I would expect it to be a name mismatch > error. > > No it doesn't. I just added it here for consistency sake because it doesn't > matter when the date invalid error is called. Should I move it? I suppose it doesn't hurt.
https://codereview.chromium.org/400323002/diff/580001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/400323002/diff/580001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:299: // destrcutor because they depend on knowing whether captive portal detection destructor, not destrcutor
On 2014/08/07 23:31:43, felt wrote: > https://codereview.chromium.org/400323002/diff/580001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > https://codereview.chromium.org/400323002/diff/580001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_blocking_page.cc:299: // destrcutor because they depend > on knowing whether captive portal detection > destructor, not destrcutor @palmer - Can you PTAL?
The bug and the CL description need to be more informative. https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:89: DEPRECATED_CAPTIVE_PORTAL_DETECTION_ENABLED, // Captive Portal errors moved Put this comment on 1 line, above line 89. https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:21: // This class calculates the severity scores for the different type of SSL Now that it extends |NotificationObserver|, this class is no longer that simple. Document what else it does. https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:41: // A function which calculates the severity score when the ssl error is As before, be more concise in these comments (for this function and for |InvalidCommonNameSeverityScore|). The sentences are not super grammatical English. https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:45: void InvalidDateSeverityScore() const; How can these functions be all of: void, const, and meaningful? https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:48: // when the SSL error is |CERT_COMMON_NAME_INVALID|, calcualates a score Typo: should be "calculates"
https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:89: DEPRECATED_CAPTIVE_PORTAL_DETECTION_ENABLED, // Captive Portal errors moved On 2014/08/07 23:48:37, Chromium Palmer wrote: > Put this comment on 1 line, above line 89. Done. https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:21: // This class calculates the severity scores for the different type of SSL On 2014/08/07 23:48:37, Chromium Palmer wrote: > Now that it extends |NotificationObserver|, this class is no longer that simple. > Document what else it does. Done. https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:41: // A function which calculates the severity score when the ssl error is On 2014/08/07 23:48:37, Chromium Palmer wrote: > As before, be more concise in these comments (for this function and for > |InvalidCommonNameSeverityScore|). > > The sentences are not super grammatical English. Done. https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:45: void InvalidDateSeverityScore() const; On 2014/08/07 23:48:37, Chromium Palmer wrote: > How can these functions be all of: void, const, and meaningful? Done.
On 2014/08/08 00:19:44, radhikabhar wrote: > https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_blocking_page.cc:89: > DEPRECATED_CAPTIVE_PORTAL_DETECTION_ENABLED, // Captive Portal errors moved > On 2014/08/07 23:48:37, Chromium Palmer wrote: > > Put this comment on 1 line, above line 89. > > Done. > > https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_error_classification.h (right): > > https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.h:21: // This class calculates the > severity scores for the different type of SSL > On 2014/08/07 23:48:37, Chromium Palmer wrote: > > Now that it extends |NotificationObserver|, this class is no longer that > simple. > > Document what else it does. > > Done. > > https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.h:41: // A function which calculates > the severity score when the ssl error is > On 2014/08/07 23:48:37, Chromium Palmer wrote: > > As before, be more concise in these comments (for this function and for > > |InvalidCommonNameSeverityScore|). > > > > The sentences are not super grammatical English. > > Done. > > https://codereview.chromium.org/400323002/diff/600001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.h:45: void > InvalidDateSeverityScore() const; > On 2014/08/07 23:48:37, Chromium Palmer wrote: > > How can these functions be all of: void, const, and meaningful? > > Done. @palmer - Do you want anything specific to be done because of the increase in the complexity of this class or is only documentation good?
https://codereview.chromium.org/400323002/diff/640001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification_unittest.cc (right): https://codereview.chromium.org/400323002/diff/640001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification_unittest.cc:36: } Another drive by: if the only thing needed is to call the superclass implementation, then this test fixture doesn't need to override SetUp() or TearDown().
https://codereview.chromium.org/400323002/diff/640001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification_unittest.cc (right): https://codereview.chromium.org/400323002/diff/640001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification_unittest.cc:36: } On 2014/08/11 19:37:08, dcheng (OOO) wrote: > Another drive by: if the only thing needed is to call the superclass > implementation, then this test fixture doesn't need to override SetUp() or > TearDown(). Done.
lgtm
On 2014/08/11 22:50:59, Chromium Palmer wrote: > lgtm @mpearson - Could you please give owners approval for histogram.xml?
On 2014/08/12 00:49:15, radhikabhar wrote: > On 2014/08/11 22:50:59, Chromium Palmer wrote: > > lgtm > > @mpearson - Could you please give owners approval for histogram.xml? @mpearson - Friendly ping, could you please give owners approval for histogram.xml
Sorry for the delay; yesterday was a mess. --mark https://codereview.chromium.org/400323002/diff/680001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/400323002/diff/680001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9958: + <summary>Record possible states of captive portals.</summary> Histogram descriptions should clearly state when the histogram is emitted to (network request received? etc.). Also, please briefly mention what a captive portal is. https://codereview.chromium.org/400323002/diff/680001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48676: + In cases of overlap, we emit to only one bucket. I thought you already submitted this fragment. Please rebase. https://codereview.chromium.org/400323002/diff/680001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48722: + (DEPRECATED_CAPTIVE_PORTAL_DETECTED_OVERRIDABLE)"/> Do you have replacements for these deprecated values? If so, you add code to emit them and add their values to the enum.
https://codereview.chromium.org/400323002/diff/680001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/400323002/diff/680001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48676: + In cases of overlap, we emit to only one bucket. On 2014/08/13 20:07:56, Mark P wrote: > I thought you already submitted this fragment. Please rebase. Done. https://codereview.chromium.org/400323002/diff/680001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48722: + (DEPRECATED_CAPTIVE_PORTAL_DETECTED_OVERRIDABLE)"/> On 2014/08/13 20:07:56, Mark P wrote: > Do you have replacements for these deprecated values? If so, you add code to > emit them and add their values to the enum. Done.
https://codereview.chromium.org/400323002/diff/900001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/400323002/diff/900001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10240: + whenever a ssl interstitial page is displayed and captive portal detection If I'm reading the enum correctly, these value overlap. I think you should explicitly point that out here perhaps add the remark (possibly multiple times to different buckets) immediately after "is emitted" https://codereview.chromium.org/400323002/diff/900001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10243: + Internet normally. nit: lowercase internet https://codereview.chromium.org/400323002/diff/900001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48928: + label="Received a captive portal result This is not "Received a captive portal result", right? That would actually imply the "result" is a captive portal. If so, that's the same as bucket 6. (We're on a captive portal and know it.) Did you mean received the result of attempting to decide if page is a captive portal? ditto on 3. https://codereview.chromium.org/400323002/diff/900001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:49362: + (DEPRECATED_CAPTIVE_PORTAL_DETECTED_OVERRIDABLE)"/> On 2014/08/14 18:16:29, radhikabhar wrote: > On 2014/08/13 20:07:56, Mark P wrote: > > Do you have replacements for these deprecated values? If so, you add code to > > emit them and add their values to the enum. > > Done. You didn't do anything. By done, so that mean you don't have replacements for these deprecated values?
https://codereview.chromium.org/400323002/diff/900001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/400323002/diff/900001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10240: + whenever a ssl interstitial page is displayed and captive portal detection On 2014/08/14 19:57:27, Mark P wrote: > If I'm reading the enum correctly, these value overlap. > I think you should explicitly point that out here > perhaps add the remark > (possibly multiple times to different buckets) > immediately after "is emitted" Done. https://codereview.chromium.org/400323002/diff/900001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10243: + Internet normally. On 2014/08/14 19:57:27, Mark P wrote: > nit: lowercase internet Done. https://codereview.chromium.org/400323002/diff/900001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48928: + label="Received a captive portal result On 2014/08/14 19:57:27, Mark P wrote: > This is not "Received a captive portal result", right? That would actually > imply the "result" is a captive portal. If so, that's the same as bucket 6. > (We're on a captive portal and know it.) > Did you mean received the result of attempting to decide if page is a captive > portal? > > ditto on 3. Done. https://codereview.chromium.org/400323002/diff/900001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:49362: + (DEPRECATED_CAPTIVE_PORTAL_DETECTED_OVERRIDABLE)"/> On 2014/08/14 19:57:27, Mark P wrote: > On 2014/08/14 18:16:29, radhikabhar wrote: > > On 2014/08/13 20:07:56, Mark P wrote: > > > Do you have replacements for these deprecated values? If so, you add code > to > > > emit them and add their values to the enum. > > > > Done. > > You didn't do anything. By done, so that mean you don't have replacements for > these deprecated values? I created a new histogram for these deprecated values which is SSLCaptivePortal. I don't have any replacement in this histogram but in the SSLCaptivePortal Histogram.
minor bits; once you fix them feel free to submit. lgtm https://codereview.chromium.org/400323002/diff/940001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/400323002/diff/940001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48926: + error page (CAPTIVE_PORTAL_DETECTION_ENABLED_OVERRIDABLE)"/> nit: add as content: This is a subset of CAPTIVE_PORTAL_DETECTION_ENABLED (bucket 1), the only difference is that it is for overridable errors. https://codereview.chromium.org/400323002/diff/940001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48937: + This is the same as CAPTIVE_PORTAL_PROBE_COMPLETED (bucket 2), the only nit: the same as -> a subset of https://codereview.chromium.org/400323002/diff/940001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48948: + (CAPTIVE_PORTAL_DETECTED_OVERRIDABLE)"/> nit: add the analogous subset of description here. https://codereview.chromium.org/400323002/diff/940001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:49368: + (DEPRECATED_CAPTIVE_PORTAL_DETECTED_OVERRIDABLE)"/> On 2014/08/14 21:29:41, radhikabhar wrote: > On 2014/08/14 19:57:27, Mark P wrote: > > On 2014/08/14 18:16:29, radhikabhar wrote: > > > On 2014/08/13 20:07:56, Mark P wrote: > > > > Do you have replacements for these deprecated values? If so, you add code > > to > > > > emit them and add their values to the enum. > > > > > > Done. > > > > You didn't do anything. By done, so that mean you don't have replacements for > > these deprecated values? > > I created a new histogram for these deprecated values which is SSLCaptivePortal. > I don't have any replacement in this histogram but in the SSLCaptivePortal > Histogram. Does the histogram that uses this enum need to be deprecated?
https://codereview.chromium.org/400323002/diff/940001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/400323002/diff/940001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:49368: + (DEPRECATED_CAPTIVE_PORTAL_DETECTED_OVERRIDABLE)"/> On 2014/08/14 22:18:35, Mark P wrote: > On 2014/08/14 21:29:41, radhikabhar wrote: > > On 2014/08/14 19:57:27, Mark P wrote: > > > On 2014/08/14 18:16:29, radhikabhar wrote: > > > > On 2014/08/13 20:07:56, Mark P wrote: > > > > > Do you have replacements for these deprecated values? If so, you add > code > > > to > > > > > emit them and add their values to the enum. > > > > > > > > Done. > > > > > > You didn't do anything. By done, so that mean you don't have replacements > for > > > these deprecated values? > > > > I created a new histogram for these deprecated values which is > SSLCaptivePortal. > > I don't have any replacement in this histogram but in the SSLCaptivePortal > > Histogram. > > Does the histogram that uses this enum need to be deprecated? No because it includes other buckets which are still in use (Only the captive portal stuff has been deprecated)
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/400323002/960001
The CQ bit was unchecked by radhikabhar@chromium.org
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/400323002/980001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/400323002/980001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/400323002/910007
The CQ bit was unchecked by radhikabhar@chromium.org
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/400323002/910007
The CQ bit was unchecked by radhikabhar@chromium.org
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/400323002/101...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: 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/42357) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/47560) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/400323002/103...
The CQ bit was unchecked by radhikabhar@chromium.org
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/400323002/105...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
Message was sent while issue was closed.
On 2014/08/15 23:35:38, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > android_clang_dbg on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) I'm taking over this CL to finish it, at https://codereview.chromium.org/516373003 |
