Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1035023002:
Adding the Finch code for the certificate error reporter experiment (Closed)
DescriptionAdding the Finch code for the certificate errors reporter experiment
BUG=461589
Committed: https://crrev.com/a9e9187b29a86e140021533b4a0d76da3bfbe386
Cr-Commit-Position: refs/heads/master@{#323870}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Rename and move the Finch helper function #Patch Set 3 : Finch experiment for certificiate error reporter. #
Total comments: 2
Patch Set 4 : Finch support for unittests #
Total comments: 3
Patch Set 5 : stark's comments #
Total comments: 15
Patch Set 6 : Fix unittests #
Total comments: 6
Patch Set 7 : Fix logic bug and add test for invalid Finch param value #
Total comments: 6
Patch Set 8 : Refactor Finch config #
Total comments: 20
Patch Set 9 : felt's comments #
Total comments: 4
Patch Set 10 : felt's comments #Patch Set 11 : fix conflicts #
Total comments: 4
Patch Set 12 : fix resolution mix-up #
Messages
Total messages: 65 (15 generated)
fahl@chromium.org changed reviewers: + estark@chromium.org, felt@chromium.org, yiyaoliu@google.com
How do Finch experiments work when running tests? Can you try (or did you already try) running browser tests? (Build browser_tests and run `out/Debug/browser_tests --gtest_filter="*HTTPS*Report*" to run the cert reporting tests.) https://codereview.chromium.org/1035023002/diff/1/chrome/browser/ssl/ssl_bloc... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/1/chrome/browser/ssl/ssl_bloc... chrome/browser/ssl/ssl_blocking_page.cc:83: bool SendFraudulentCertificateChain() { Maybe put this down with the other helper functions (starting at line 108)? So that constants and helper functions are grouped together. https://codereview.chromium.org/1035023002/diff/1/chrome/browser/ssl/ssl_bloc... chrome/browser/ssl/ssl_blocking_page.cc:83: bool SendFraudulentCertificateChain() { I think we should name the experiment |CertificateErrorReporter| or |ReportCertificateErrors| or |ReportSSLCertificateErrors| (and then this function would be |ShouldReportCertificateErrors| or something), because "fraudulent" seems to refer to pinning violations in the code. For example ChromeFraudulentCertificateReporter specifically reports pinning violations on Google properties, whereas CertificateErrorReporter is the more general certificate-reporting class that is used to report SSL errors here. https://codereview.chromium.org/1035023002/diff/1/chrome/browser/ssl/ssl_bloc... chrome/browser/ssl/ssl_blocking_page.cc:668: if (certificate_report_callback_for_testing_.is_null()) This bit can go inside the `if` statement as well (to avoid an extra DoNothing when not in the experiment). Not a big deal though. https://codereview.chromium.org/1035023002/diff/1/chrome/browser/ssl/ssl_bloc... chrome/browser/ssl/ssl_blocking_page.cc:671: if (SendFraudulentCertificateChain()) I would use curly braces since the body spans more than one line (consistent with style in line 121-125).
https://codereview.chromium.org/1035023002/diff/1/chrome/browser/ssl/ssl_bloc... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/1/chrome/browser/ssl/ssl_bloc... chrome/browser/ssl/ssl_blocking_page.cc:83: bool SendFraudulentCertificateChain() { On 2015/03/27 01:22:02, emily wrote: > Maybe put this down with the other helper functions (starting at line 108)? So > that constants and helper functions are grouped together. Sure, I just put it there so no one forgets to remove the code one the experiment expired. I found an experiment that expired in August last year and still is in the code... :)
https://codereview.chromium.org/1035023002/diff/1/chrome/browser/ssl/ssl_bloc... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/1/chrome/browser/ssl/ssl_bloc... chrome/browser/ssl/ssl_blocking_page.cc:83: bool SendFraudulentCertificateChain() { On 2015/03/27 01:22:02, emily wrote: > Maybe put this down with the other helper functions (starting at line 108)? So > that constants and helper functions are grouped together. Done. https://codereview.chromium.org/1035023002/diff/1/chrome/browser/ssl/ssl_bloc... chrome/browser/ssl/ssl_blocking_page.cc:668: if (certificate_report_callback_for_testing_.is_null()) On 2015/03/27 01:22:01, emily wrote: > This bit can go inside the `if` statement as well (to avoid an extra DoNothing > when not in the experiment). Not a big deal though. Done. https://codereview.chromium.org/1035023002/diff/1/chrome/browser/ssl/ssl_bloc... chrome/browser/ssl/ssl_blocking_page.cc:671: if (SendFraudulentCertificateChain()) On 2015/03/27 01:22:01, emily wrote: > I would use curly braces since the body spans more than one line (consistent > with style in line 121-125). Done.
On 2015/03/27 01:22:02, emily wrote: > How do Finch experiments work when running tests? Can you try (or did you > already try) running browser tests? (Build browser_tests and run > `out/Debug/browser_tests --gtest_filter="*HTTPS*Report*" to run the cert > reporting tests.) You'll need to explicitly the Finch experiment as enabled or disabled (whatever you want for the test). You can do this by setting a command line flag for the test that enables/disables the experiment, whichever you like. > > https://codereview.chromium.org/1035023002/diff/1/chrome/browser/ssl/ssl_bloc... > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > https://codereview.chromium.org/1035023002/diff/1/chrome/browser/ssl/ssl_bloc... > chrome/browser/ssl/ssl_blocking_page.cc:83: bool > SendFraudulentCertificateChain() { > Maybe put this down with the other helper functions (starting at line 108)? So > that constants and helper functions are grouped together. > > https://codereview.chromium.org/1035023002/diff/1/chrome/browser/ssl/ssl_bloc... > chrome/browser/ssl/ssl_blocking_page.cc:83: bool > SendFraudulentCertificateChain() { > I think we should name the experiment |CertificateErrorReporter| or > |ReportCertificateErrors| or |ReportSSLCertificateErrors| (and then this > function would be |ShouldReportCertificateErrors| or something), because > "fraudulent" seems to refer to pinning violations in the code. For example > ChromeFraudulentCertificateReporter specifically reports pinning violations on > Google properties, whereas CertificateErrorReporter is the more general > certificate-reporting class that is used to report SSL errors here. > > https://codereview.chromium.org/1035023002/diff/1/chrome/browser/ssl/ssl_bloc... > chrome/browser/ssl/ssl_blocking_page.cc:668: if > (certificate_report_callback_for_testing_.is_null()) > This bit can go inside the `if` statement as well (to avoid an extra DoNothing > when not in the experiment). Not a big deal though. > > https://codereview.chromium.org/1035023002/diff/1/chrome/browser/ssl/ssl_bloc... > chrome/browser/ssl/ssl_blocking_page.cc:671: if > (SendFraudulentCertificateChain()) > I would use curly braces since the body spans more than one line (consistent > with style in line 121-125).
"After talking to Emily we decided to show all users the checkbox, but make the decision whether the certificate chain should be sent in the background. This way we provide a consistent user experience for all users. The other option would be to use Finch to decide whether to show the checkbox. Would anyone prefer that option?" For future reference, you should put questions like this in the body of the first message that you send out. The description should be a description of how the CL works, not open questions etc. So can you please update the description by removing the question? To answer the question though, I envisioned this Finch flag working a little differently. -- We'd discussed (in the design doc) the idea that the cert reporter will send back a certain % of reports. This lets us collect lots on Canary, but we can throttle down Stable until we have a good pipeline in place that needs the full firehose. -- We should have a condition that completely kills the feature altogether (even removing the checkbox). That way if our feature is not approved to launch, or whatever else happens... we are able to pull it back. -- You can take a look at how Joel implemented the remember-cert-error-decisions flag. I think it would be a good template.
On 2015/03/27 04:51:33, felt wrote:
> "After talking to Emily we decided to show all users the checkbox, but make
the
> decision whether the certificate chain should be sent in the background. This
> way we provide a consistent user experience for all users. The other option
> would be to use Finch to decide whether to show the checkbox. Would anyone
> prefer that option?"
>
> For future reference, you should put questions like this in the body of the
> first message that you send out. The description should be a description of
how
> the CL works, not open questions etc. So can you please update the description
> by removing the question?
>
> To answer the question though, I envisioned this Finch flag working a little
> differently.
> -- We'd discussed (in the design doc) the idea that the cert reporter will
send
> back a certain % of reports. This lets us collect lots on Canary, but we can
> throttle down Stable until we have a good pipeline in place that needs the
full
> firehose.
Ohhh I think maybe I don't understand how Finch works at all... Is the idea that
we should have three groups ("Send", "Don't send", "Don't send and don't even
show the checkbox") and we dynamically set the percentage of users in each
group? I was under the impression that if we set 1% in the Finch config, then
|FindFullName("ReportCertificateErrors")| would return "Default" for 1% and
empty string or something else for the rest (which in retrospect doesn't make a
whole lot of sense, and sounds like it's not how it works).
> -- We should have a condition that completely kills the feature altogether
(even
> removing the checkbox). That way if our feature is not approved to launch, or
> whatever else happens... we are able to pull it back.
> -- You can take a look at how Joel implemented the
remember-cert-error-decisions
> flag. I think it would be a good template.
Thanks, that's helpful.
On 2015/03/27 06:09:07, emily wrote:
> On 2015/03/27 04:51:33, felt wrote:
> > "After talking to Emily we decided to show all users the checkbox, but make
> the
> > decision whether the certificate chain should be sent in the background.
This
> > way we provide a consistent user experience for all users. The other option
> > would be to use Finch to decide whether to show the checkbox. Would anyone
> > prefer that option?"
> >
> > For future reference, you should put questions like this in the body of the
> > first message that you send out. The description should be a description of
> how
> > the CL works, not open questions etc. So can you please update the
description
> > by removing the question?
> >
> > To answer the question though, I envisioned this Finch flag working a little
> > differently.
> > -- We'd discussed (in the design doc) the idea that the cert reporter will
> send
> > back a certain % of reports. This lets us collect lots on Canary, but we can
> > throttle down Stable until we have a good pipeline in place that needs the
> full
> > firehose.
>
> Ohhh I think maybe I don't understand how Finch works at all... Is the idea
that
> we should have three groups ("Send", "Don't send", "Don't send and don't even
> show the checkbox") and we dynamically set the percentage of users in each
> group? I was under the impression that if we set 1% in the Finch config, then
> |FindFullName("ReportCertificateErrors")| would return "Default" for 1% and
> empty string or something else for the rest (which in retrospect doesn't make
a
> whole lot of sense, and sounds like it's not how it works).
There are two ways to do it:
1) We have three groups (Send, Don't send, Completely off) and dynamically set
how many people are in each group.
2) We have two groups (Send, Completely off) and send a parameter that controls
a probability for sending.
I was originally thinking #2 would be the way to go, but now that you mention it
I suppose that #1 is fine too.
With that being said you still want to name the groups explicitly, not just look
for "Default", like:
[psuedocode so you get the gist]
if (FindFullName("ReportCertErors") == "Send") {
show = true;
SendReports();
} else if (FindFullName("ReportCertErrors") == "ShowButDontSend") {
show = true;
} else {
show = false;
}
>
> > -- We should have a condition that completely kills the feature altogether
> (even
> > removing the checkbox). That way if our feature is not approved to launch,
or
> > whatever else happens... we are able to pull it back.
> > -- You can take a look at how Joel implemented the
> remember-cert-error-decisions
> > flag. I think it would be a good template.
>
> Thanks, that's helpful.
On 2015/03/26 23:51:37, fahl wrote: After talking to Emily we decided to show all users the checkbox, but make the decision whether the certificate chain should be sent in the background. This way we provide a consistent user experience for all users. The other option would be to use Finch to decide whether to show the checkbox. Would anyone prefer that option?
fahl@chromium.org changed reviewers: - yiyaoliu@google.com
felt@: you were going to send an example of the command-line flag to set a finch config so that we can use that in the cert reporter browser tests. https://codereview.chromium.org/1035023002/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:110: if (base::CommandLine::ForCurrentProcess()->HasSwitch( I believe we can remove this command-line flag altogether now, because the default behavior with finch alone will be to not show the checkbox and not send reports. Not sure if that has to be done in this CL though.
https://codereview.chromium.org/1035023002/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:110: if (base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/03/27 23:23:41, emily wrote: > I believe we can remove this command-line flag altogether now, because the > default behavior with finch alone will be to not show the checkbox and not send > reports. > > Not sure if that has to be done in this CL though. I just left it there to not break the unittests.
On 2015/03/27 23:23:41, emily wrote: > felt@: you were going to send an example of the command-line flag to set a finch > config so that we can use that in the cert reporter browser tests. > > https://codereview.chromium.org/1035023002/diff/40001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > https://codereview.chromium.org/1035023002/diff/40001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_blocking_page.cc:110: if > (base::CommandLine::ForCurrentProcess()->HasSwitch( > I believe we can remove this command-line flag altogether now, because the > default behavior with finch alone will be to not show the checkbox and not send > reports. > > Not sure if that has to be done in this CL though. You can see an example of how to do it for tests here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/cap...
On 2015/03/28 01:44:09, felt wrote: > On 2015/03/27 23:23:41, emily wrote: > > felt@: you were going to send an example of the command-line flag to set a > finch > > config so that we can use that in the cert reporter browser tests. > > > > > https://codereview.chromium.org/1035023002/diff/40001/chrome/browser/ssl/ssl_... > > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > > > > https://codereview.chromium.org/1035023002/diff/40001/chrome/browser/ssl/ssl_... > > chrome/browser/ssl/ssl_blocking_page.cc:110: if > > (base::CommandLine::ForCurrentProcess()->HasSwitch( > > I believe we can remove this command-line flag altogether now, because the > > default behavior with finch alone will be to not show the checkbox and not > send > > reports. > > > > Not sure if that has to be done in this CL though. > > You can see an example of how to do it for tests here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/cap... I followed the example for the captive portal tests. Using the kForceFieldTrials approach works fine for just passing in a groupname. However, it seems there is no support for also passing in a parameter value for a group name. It seems we have two options here: (1) always manually parse the groupname value and include the parameter value in the groupname (2) use a special flag for the unittests as we do for now Adrienne: are you aware of a workaround for this? In case not which option would you prefer? I'd say having a special flag for unittests is a bit more explicit and safe than manually parsing the groupname.
On 2015/03/30 21:37:45, fahl wrote: > On 2015/03/28 01:44:09, felt wrote: > > On 2015/03/27 23:23:41, emily wrote: > > > felt@: you were going to send an example of the command-line flag to set a > > finch > > > config so that we can use that in the cert reporter browser tests. > > > > > > > > > https://codereview.chromium.org/1035023002/diff/40001/chrome/browser/ssl/ssl_... > > > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > > > > > > > > https://codereview.chromium.org/1035023002/diff/40001/chrome/browser/ssl/ssl_... > > > chrome/browser/ssl/ssl_blocking_page.cc:110: if > > > (base::CommandLine::ForCurrentProcess()->HasSwitch( > > > I believe we can remove this command-line flag altogether now, because the > > > default behavior with finch alone will be to not show the checkbox and not > > send > > > reports. > > > > > > Not sure if that has to be done in this CL though. > > > > You can see an example of how to do it for tests here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/cap... > > I followed the example for the captive portal tests. Using the kForceFieldTrials > approach works fine for just passing in a groupname. However, it seems there is > no support for also passing in a parameter value for a group name. It seems we > have two options here: > > (1) always manually parse the groupname value and include the parameter value in > the groupname > (2) use a special flag for the unittests as we do for now > > Adrienne: are you aware of a workaround for this? In case not which option would > you prefer? I'd say having a special flag for unittests is a bit more explicit > and safe than manually parsing the groupname. Hmm you might want to ping finch-team@ to double check first. If it ends up that there really isn't one, we can do a separate flag for unittests.
Patchset #4 (id:60001) has been deleted
I added Finch parameter support for the experiment and removed the old command line flag (switches::kEnableInvalidCertCollection) for that. However, the flag is still used in other parts of the unit tests. Should we leave it like that or work towards removing the command line flag entirely?
I think we should get rid of the kEnableInvalidCerts flag entirely, to get rid of the extra complexity (I'm afraid that we'll forget to test some combination of flag and finch config and end up accidentally sending reports when we shouldn't be). See my comments inline about how to adapt the tests to test finch config instead of the command-line flag; let me know if it's unclear. https://codereview.chromium.org/1035023002/diff/80001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1035023002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:1150: base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors", I think this code should appear on the 4 tests below as well (all the SSLUITestWithExtendedReporting tests). In fact, I think you could just remove the command-line flag entirely and then replace line 527 with this code (and make the function |SetUp| instead of |SetUpCommandLine|). https://codereview.chromium.org/1035023002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:1191: TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN, This test is supposed to test that reports don't get sent when the user doesn't see the checkbox. So I think this test should be adapted to include code like lines 1150-1155, except assigning the user to the DontShowAndDontSend group. We should then duplicate this test that's exactly the same, excepting assigning the user to ShowAndPossiblySend with a parameter of 0. It would be even better to additionally test that the checkbox doesn't actually appear when the user is in the DontShowAndDontSend group (using something like https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/int...).
https://codereview.chromium.org/1035023002/diff/80001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1035023002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:1191: TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN, On 2015/04/01 20:47:43, emily wrote: > We should then duplicate this test that's exactly the same, excepting assigning > the user to ShowAndPossiblySend with a parameter of 0. Sorry, I failed at English on that sentence... let me rephrase: We should then duplicate this test to have one that's exactly the same, except assigning the user to ShowAndPossiblySend with a parameter of 0.
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
Hmm, the tests don't look quite right to me. Here are the cases that I think we should test: * User is shown checkbox, checks it, and report is sent when finch param is 1.0. This is correctly covered by lines 1125-1154 right now. * User is shown checkbox, does not check it, and report is not sent, even with a finch param of 1.0. This is almost covered by lines 1160-1183 right now, but see inline comment. * User is shown checkbox, checks it, and report is not sent when finch param is -1.0. I don't think we are covering this anywhere right now. * User is not shown checkbox and report is not sent, even if the opt-in preference is set. This is almost covered by lines 1193-1202 right now, but see inline comment. https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:126: return base::FieldTrialList::FindFullName("ReportCertificateErrors") How about doing the incognito check in here? https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:488: ShowCertificateReporterCheckbox(); If you do the incognito check in ShowCertificateReporterCheckbox, then this can just be |bool show = ShowCertificateReporterCheckbox()|. https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:674: if (!ReportCertificateErrors() || I think this should just be: if (!ShowCertificateReporterCheckbox()) return; (assuming you move the incognito check into ShowCertificateReporterCheckbox) The reason is that I think we want to record UMA stats for how many users are opted in to reporting (line 684), even if the user doesn't actually send this particular report. https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:1164: params["possibly_send"] = "-1.0"; I think this should be -1.0, not 1.0. Same for the test below, which is why I think you can move this setup code into SSLUITestWithExtendedReporting::SetUp instead of repeating it in each test. Why should it be 1.0? This test and the one below it are testing that a report never gets sent if the user does not check the checkbox. Suppose we had a bug where we were sending reports even if the checkbox wasn't checked; as is, this test wouldn't catch that bug because the Finch param is set to -1.0 (never send reports). So I think these two tests should have params of 1.0, and then we should have an additional test that tests that reports never get sent when the param is -1.0, even if the user is opted in. This additional test would look like this: // // set param to -1.0 so reports never get sent // TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN, SSL_INTERSTITIAL_PROCEED, CERT_REPORT_NOT_EXPECTED, browser()); https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:1181: params["possibly_send"] = "-1.0"; should be 1.0 as above https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:1199: CertificateReporting::EXTENDED_REPORTING_DO_NOT_OPT_IN, I think this should be OPT_IN. i.e. we want to test that even if the user is opted in (because they were previously in ShowAndPossiblySend, for example), the report does not get sent. https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:1211: TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN, Should this test be deleted? It doesn't seem to make sense because the Finch config is not fixed, so we don't know what the outcome will be.
On 2015/04/02 01:19:51, emily wrote: > Hmm, the tests don't look quite right to me. Here are the cases that I think we > should test: > > * User is shown checkbox, checks it, and report is sent when finch param is 1.0. > This is correctly covered by lines 1125-1154 right now. > > * User is shown checkbox, does not check it, and report is not sent, even with a > finch param of 1.0. This is almost covered by lines 1160-1183 right now, but see > inline comment. > > * User is shown checkbox, checks it, and report is not sent when finch param is > -1.0. I don't think we are covering this anywhere right now. > > * User is not shown checkbox and report is not sent, even if the opt-in > preference is set. This is almost covered by lines 1193-1202 right now, but see > inline comment. > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_blocking_page.cc:126: return > base::FieldTrialList::FindFullName("ReportCertificateErrors") > How about doing the incognito check in here? > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_blocking_page.cc:488: ShowCertificateReporterCheckbox(); > If you do the incognito check in ShowCertificateReporterCheckbox, then this can > just be |bool show = ShowCertificateReporterCheckbox()|. > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_blocking_page.cc:674: if (!ReportCertificateErrors() || > I think this should just be: > > if (!ShowCertificateReporterCheckbox()) > return; > > (assuming you move the incognito check into ShowCertificateReporterCheckbox) > > The reason is that I think we want to record UMA stats for how many users are > opted in to reporting (line 684), even if the user doesn't actually send this > particular report. > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:1164: params["possibly_send"] = "-1.0"; > I think this should be -1.0, not 1.0. Same for the test below, which is why I > think you can move this setup code into SSLUITestWithExtendedReporting::SetUp > instead of repeating it in each test. > > Why should it be 1.0? This test and the one below it are testing that a report > never gets sent if the user does not check the checkbox. Suppose we had a bug > where we were sending reports even if the checkbox wasn't checked; as is, this > test wouldn't catch that bug because the Finch param is set to -1.0 (never send > reports). > > So I think these two tests should have params of 1.0, and then we should have an > additional test that tests that reports never get sent when the param is -1.0, > even if the user is opted in. This additional test would look like this: > > // > // set param to -1.0 so reports never get sent > // > TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN, > SSL_INTERSTITIAL_PROCEED, CERT_REPORT_NOT_EXPECTED, browser()); > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:1181: params["possibly_send"] = "-1.0"; > should be 1.0 as above > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:1199: > CertificateReporting::EXTENDED_REPORTING_DO_NOT_OPT_IN, > I think this should be OPT_IN. i.e. we want to test that even if the user is > opted in (because they were previously in ShowAndPossiblySend, for example), the > report does not get sent. > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:1211: > TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN, > Should this test be deleted? It doesn't seem to make sense because the Finch > config is not fixed, so we don't know what the outcome will be. You are totally right, the current tests coverage is not sufficient :) I just modeled the "Send Report" output as a function with the four inputs "Checkbox Shown", "Checkbox Checked", "Finch Param" and "Incognito" and "Send Report" as output (https://docs.google.com/spreadsheets/d/1zy1WoESiF2axCrRrvIiaZjEdV_pHVWbNoG6MD...) and it turns out that we currently have no test that takes all four inputs. Should I implement all 16 tests so we get 100% coverage or would that be too much?
https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:674: if (!ReportCertificateErrors() || On 2015/04/02 01:19:51, emily wrote: > I think this should just be: > > if (!ShowCertificateReporterCheckbox()) > return; > > (assuming you move the incognito check into ShowCertificateReporterCheckbox) > > The reason is that I think we want to record UMA stats for how many users are > opted in to reporting (line 684), even if the user doesn't actually send this > particular report. Done. https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:1211: TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN, On 2015/04/02 01:19:51, emily wrote: > Should this test be deleted? It doesn't seem to make sense because the Finch > config is not fixed, so we don't know what the outcome will be. Acknowledged.
On 2015/04/02 02:01:07, fahl wrote: > On 2015/04/02 01:19:51, emily wrote: > > Hmm, the tests don't look quite right to me. Here are the cases that I think > we > > should test: > > > > * User is shown checkbox, checks it, and report is sent when finch param is > 1.0. > > This is correctly covered by lines 1125-1154 right now. > > > > * User is shown checkbox, does not check it, and report is not sent, even with > a > > finch param of 1.0. This is almost covered by lines 1160-1183 right now, but > see > > inline comment. > > > > * User is shown checkbox, checks it, and report is not sent when finch param > is > > -1.0. I don't think we are covering this anywhere right now. > > > > * User is not shown checkbox and report is not sent, even if the opt-in > > preference is set. This is almost covered by lines 1193-1202 right now, but > see > > inline comment. > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > chrome/browser/ssl/ssl_blocking_page.cc:126: return > > base::FieldTrialList::FindFullName("ReportCertificateErrors") > > How about doing the incognito check in here? > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > chrome/browser/ssl/ssl_blocking_page.cc:488: > ShowCertificateReporterCheckbox(); > > If you do the incognito check in ShowCertificateReporterCheckbox, then this > can > > just be |bool show = ShowCertificateReporterCheckbox()|. > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > chrome/browser/ssl/ssl_blocking_page.cc:674: if (!ReportCertificateErrors() || > > I think this should just be: > > > > if (!ShowCertificateReporterCheckbox()) > > return; > > > > (assuming you move the incognito check into ShowCertificateReporterCheckbox) > > > > The reason is that I think we want to record UMA stats for how many users are > > opted in to reporting (line 684), even if the user doesn't actually send this > > particular report. > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > chrome/browser/ssl/ssl_browser_tests.cc:1164: params["possibly_send"] = > "-1.0"; > > I think this should be -1.0, not 1.0. Same for the test below, which is why I > > think you can move this setup code into SSLUITestWithExtendedReporting::SetUp > > instead of repeating it in each test. > > > > Why should it be 1.0? This test and the one below it are testing that a report > > never gets sent if the user does not check the checkbox. Suppose we had a bug > > where we were sending reports even if the checkbox wasn't checked; as is, this > > test wouldn't catch that bug because the Finch param is set to -1.0 (never > send > > reports). > > > > So I think these two tests should have params of 1.0, and then we should have > an > > additional test that tests that reports never get sent when the param is -1.0, > > even if the user is opted in. This additional test would look like this: > > > > // > > // set param to -1.0 so reports never get sent > > // > > TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN, > > SSL_INTERSTITIAL_PROCEED, CERT_REPORT_NOT_EXPECTED, browser()); > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > chrome/browser/ssl/ssl_browser_tests.cc:1181: params["possibly_send"] = > "-1.0"; > > should be 1.0 as above > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > chrome/browser/ssl/ssl_browser_tests.cc:1199: > > CertificateReporting::EXTENDED_REPORTING_DO_NOT_OPT_IN, > > I think this should be OPT_IN. i.e. we want to test that even if the user is > > opted in (because they were previously in ShowAndPossiblySend, for example), > the > > report does not get sent. > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > chrome/browser/ssl/ssl_browser_tests.cc:1211: > > TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN, > > Should this test be deleted? It doesn't seem to make sense because the Finch > > config is not fixed, so we don't know what the outcome will be. > > You are totally right, the current tests coverage is not sufficient :) > > I just modeled the "Send Report" output as a function with the four inputs > "Checkbox Shown", "Checkbox Checked", "Finch Param" and "Incognito" and "Send > Report" as output > (https://docs.google.com/spreadsheets/d/1zy1WoESiF2axCrRrvIiaZjEdV_pHVWbNoG6MD...) > and it turns out that we currently have no test that takes all four inputs. > > Should I implement all 16 tests so we get 100% coverage or would that be too > much? Sorry, the non-incognito tests take the right number of inputs. However, still some input combinations are missing. In case you agree I'd implement all 16 tests so we have 100% coverage.
On 2015/04/02 02:25:07, fahl wrote: > On 2015/04/02 02:01:07, fahl wrote: > > On 2015/04/02 01:19:51, emily wrote: > > > Hmm, the tests don't look quite right to me. Here are the cases that I think > > we > > > should test: > > > > > > * User is shown checkbox, checks it, and report is sent when finch param is > > 1.0. > > > This is correctly covered by lines 1125-1154 right now. > > > > > > * User is shown checkbox, does not check it, and report is not sent, even > with > > a > > > finch param of 1.0. This is almost covered by lines 1160-1183 right now, but > > see > > > inline comment. > > > > > > * User is shown checkbox, checks it, and report is not sent when finch param > > is > > > -1.0. I don't think we are covering this anywhere right now. > > > > > > * User is not shown checkbox and report is not sent, even if the opt-in > > > preference is set. This is almost covered by lines 1193-1202 right now, but > > see > > > inline comment. > > > > > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > > > > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > > chrome/browser/ssl/ssl_blocking_page.cc:126: return > > > base::FieldTrialList::FindFullName("ReportCertificateErrors") > > > How about doing the incognito check in here? > > > > > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > > chrome/browser/ssl/ssl_blocking_page.cc:488: > > ShowCertificateReporterCheckbox(); > > > If you do the incognito check in ShowCertificateReporterCheckbox, then this > > can > > > just be |bool show = ShowCertificateReporterCheckbox()|. > > > > > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > > chrome/browser/ssl/ssl_blocking_page.cc:674: if (!ReportCertificateErrors() > || > > > I think this should just be: > > > > > > if (!ShowCertificateReporterCheckbox()) > > > return; > > > > > > (assuming you move the incognito check into ShowCertificateReporterCheckbox) > > > > > > The reason is that I think we want to record UMA stats for how many users > are > > > opted in to reporting (line 684), even if the user doesn't actually send > this > > > particular report. > > > > > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > > > > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > > chrome/browser/ssl/ssl_browser_tests.cc:1164: params["possibly_send"] = > > "-1.0"; > > > I think this should be -1.0, not 1.0. Same for the test below, which is why > I > > > think you can move this setup code into > SSLUITestWithExtendedReporting::SetUp > > > instead of repeating it in each test. > > > > > > Why should it be 1.0? This test and the one below it are testing that a > report > > > never gets sent if the user does not check the checkbox. Suppose we had a > bug > > > where we were sending reports even if the checkbox wasn't checked; as is, > this > > > test wouldn't catch that bug because the Finch param is set to -1.0 (never > > send > > > reports). > > > > > > So I think these two tests should have params of 1.0, and then we should > have > > an > > > additional test that tests that reports never get sent when the param is > -1.0, > > > even if the user is opted in. This additional test would look like this: > > > > > > // > > > // set param to -1.0 so reports never get sent > > > // > > > TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN, > > > SSL_INTERSTITIAL_PROCEED, CERT_REPORT_NOT_EXPECTED, browser()); > > > > > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > > chrome/browser/ssl/ssl_browser_tests.cc:1181: params["possibly_send"] = > > "-1.0"; > > > should be 1.0 as above > > > > > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > > chrome/browser/ssl/ssl_browser_tests.cc:1199: > > > CertificateReporting::EXTENDED_REPORTING_DO_NOT_OPT_IN, > > > I think this should be OPT_IN. i.e. we want to test that even if the user is > > > opted in (because they were previously in ShowAndPossiblySend, for example), > > the > > > report does not get sent. > > > > > > > > > https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... > > > chrome/browser/ssl/ssl_browser_tests.cc:1211: > > > TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN, > > > Should this test be deleted? It doesn't seem to make sense because the Finch > > > config is not fixed, so we don't know what the outcome will be. > > > > You are totally right, the current tests coverage is not sufficient :) > > > > I just modeled the "Send Report" output as a function with the four inputs > > "Checkbox Shown", "Checkbox Checked", "Finch Param" and "Incognito" and "Send > > Report" as output > > > (https://docs.google.com/spreadsheets/d/1zy1WoESiF2axCrRrvIiaZjEdV_pHVWbNoG6MD...) > > and it turns out that we currently have no test that takes all four inputs. > > > > Should I implement all 16 tests so we get 100% coverage or would that be too > > much? > > Sorry, the non-incognito tests take the right number of inputs. However, still > some input combinations are missing. In case you agree I'd implement all 16 > tests so we have 100% coverage. Eh, I don't think we need to test all possible combinations. For example, if we have a test that ShowAndPossiblySend/param=1.0/user opted-in/incognito doesn't send reports, I don't think we need to test DontShowAndDontSend in incognito mode, because we're unlikely to introduce a bug in which DontShowAndDontSend sends reports in incognito but ShowAndPossiblySend doesn't. I think the most important cases should be sufficient (the ones I listed above, plus not sending when opted-in in incognito). That is, Test that opt-in works as expected: * ShowAndPossiblySend/param=1.0/user opts in => report gets sent * ShowAndPossiblySend/param=1.0/user does not opt in => report does not get sent Test that Finch param works as expected: * ShowAndPossiblySend/param=-1.0/user opts in => report does not get sent Test that DontShowAndDontSend works as expected: * DontShowAndDontSend/user is opted in => report does not get sent Reports are not sent in incognito * ShowAndPossiblySend/param=1.0/user opts in => report does not get sent These are the cases I think would be most disastrous if we broke and are most likely to catch bugs.
https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:1221: TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN, this one also needs a finch param (I think it should be ShowAndPossiblySend with a param of 1.0)
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
I still could move the Finch specific code from the test method bodies to a test global SetUp method. However, since the Finch code is different for different tests and leaving it in the tests makes it a bit easier to read the tests, I think we also could leave the Finch code where it is now. estark, what do you think? https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:1164: params["possibly_send"] = "-1.0"; On 2015/04/02 01:19:51, emily wrote: > I think this should be -1.0, not 1.0. Same for the test below, which is why I > think you can move this setup code into SSLUITestWithExtendedReporting::SetUp > instead of repeating it in each test. > > Why should it be 1.0? This test and the one below it are testing that a report > never gets sent if the user does not check the checkbox. Suppose we had a bug > where we were sending reports even if the checkbox wasn't checked; as is, this > test wouldn't catch that bug because the Finch param is set to -1.0 (never send > reports). > > So I think these two tests should have params of 1.0, and then we should have an > additional test that tests that reports never get sent when the param is -1.0, > even if the user is opted in. This additional test would look like this: > > // > // set param to -1.0 so reports never get sent > // > TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN, > SSL_INTERSTITIAL_PROCEED, CERT_REPORT_NOT_EXPECTED, browser()); Done. https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:1181: params["possibly_send"] = "-1.0"; On 2015/04/02 01:19:51, emily wrote: > should be 1.0 as above Done. https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:1199: CertificateReporting::EXTENDED_REPORTING_DO_NOT_OPT_IN, On 2015/04/02 01:19:51, emily wrote: > I think this should be OPT_IN. i.e. we want to test that even if the user is > opted in (because they were previously in ShowAndPossiblySend, for example), the > report does not get sent. Done. https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:1211: TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN, On 2015/04/02 01:19:51, emily wrote: > Should this test be deleted? It doesn't seem to make sense because the Finch > config is not fixed, so we don't know what the outcome will be. Done. https://codereview.chromium.org/1035023002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:1221: TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN, On 2015/04/02 03:22:23, emily wrote: > this one also needs a finch param (I think it should be ShowAndPossiblySend with > a param of 1.0) Done.
Tests look good! Logic bug inline though. https://codereview.chromium.org/1035023002/diff/200001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/200001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:115: LOG(ERROR) << "RandValue: " << base::RandDouble(); any reason to leave this in? https://codereview.chromium.org/1035023002/diff/200001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:116: if (base::StringToDouble(param, &possiblySend)) This logic looks funny to me... |ReportCertificateErrors(false)| will return true if |StringToDouble| returns false. Also I think this function is more complicated than it needs to be. We call |ShowCertificateReporterCheckbox| immediately before calling |ReportCertificateErrors| so I don't think we need to check the group name and incognito again. Maybe something like this would be cleaner and more correct: // Returns whether the user should report a given certificate error, assuming that they are opted in to extended reporting and are being shown the checkbox on the interstitial. bool ShouldReportCertificateError(bool in_incognito) { DCHECK(ShouldShowCertificateReporterCheckbox(in_incognito)); const std::string param = ... if (param.compare("") != 0) { double possiblySend; if (base::StringToDouble(param, &possiblySend)) return base::RandDouble() <= possiblySend; } return false; } And to be on the safe side we should perhaps add a regression test that sets a parameter to an invalid value ("abc"?) and checks that the report doesn't get sent in that case. https://codereview.chromium.org/1035023002/diff/200001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1035023002/diff/200001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:1129: std::map<std::string, std::string> params; you could also factor this out into a helper function that takes the param value as an argument, but I don't have a strong opinion either way
Patchset #7 (id:210006) has been deleted
https://codereview.chromium.org/1035023002/diff/200001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/200001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:115: LOG(ERROR) << "RandValue: " << base::RandDouble(); On 2015/04/02 18:33:34, emily wrote: > any reason to leave this in? Done. https://codereview.chromium.org/1035023002/diff/200001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:116: if (base::StringToDouble(param, &possiblySend)) On 2015/04/02 18:33:34, emily wrote: > This logic looks funny to me... |ReportCertificateErrors(false)| will return > true if |StringToDouble| returns false. Also I think this function is more > complicated than it needs to be. We call |ShowCertificateReporterCheckbox| > immediately before calling |ReportCertificateErrors| so I don't think we need to > check the group name and incognito again. > > Maybe something like this would be cleaner and more correct: > > // Returns whether the user should report a given certificate error, assuming > that they are opted in to extended reporting and are being shown the checkbox on > the interstitial. > bool ShouldReportCertificateError(bool in_incognito) { > DCHECK(ShouldShowCertificateReporterCheckbox(in_incognito)); > const std::string param = ... > if (param.compare("") != 0) { > double possiblySend; > if (base::StringToDouble(param, &possiblySend)) > return base::RandDouble() <= possiblySend; > } > return false; > } > > And to be on the safe side we should perhaps add a regression test that sets a > parameter to an invalid value ("abc"?) and checks that the report doesn't get > sent in that case. Done. https://codereview.chromium.org/1035023002/diff/200001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1035023002/diff/200001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:1129: std::map<std::string, std::string> params; On 2015/04/02 18:33:34, emily wrote: > you could also factor this out into a helper function that takes the param value > as an argument, but I don't have a strong opinion either way Done.
LGTM except for two small style things inline. https://codereview.chromium.org/1035023002/diff/230001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1035023002/diff/230001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:479: std::map<std::string, std::string> SetFinchPossiblySendParam( As a matter of style, I don't think we're generally supposed to pass around objects by value (both the return value and the |value| argument here). Why not do something like: void SetCertReportingFinchConfig(const std::string& group_name, const std::string& param_value) { base::FieldTrialList::CreateFieldTrial(...); // initialize |params| here variations::AssociateVariationParams(...); } https://codereview.chromium.org/1035023002/diff/230001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:1135: base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors", nit: the "ReportCertificateErrors" and group name strings ("ShowAndPossiblySend", "DontShowAndDontSend") should be constants. They should probably be constants in ssl_blocking_page.cc too, come to think of it... so maybe declare the constants in ssl_blocking_page.h and #include them to use here?
https://codereview.chromium.org/1035023002/diff/230001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/230001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:673: web_contents()->GetBrowserContext()->IsOffTheRecord())) Sorry, one more nit here, I think the body should have curly braces since the condition spans more than one line (following the style of line 240)
Made the Finch config strings constants and moved the actual experiment configuration to a helper function as suggested. https://codereview.chromium.org/1035023002/diff/230001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/230001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:673: web_contents()->GetBrowserContext()->IsOffTheRecord())) On 2015/04/02 20:01:32, emily wrote: > Sorry, one more nit here, I think the body should have curly braces since the > condition spans more than one line (following the style of line 240) Done. https://codereview.chromium.org/1035023002/diff/230001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1035023002/diff/230001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:479: std::map<std::string, std::string> SetFinchPossiblySendParam( On 2015/04/02 19:55:56, emily wrote: > As a matter of style, I don't think we're generally supposed to pass around > objects by value (both the return value and the |value| argument here). Why not > do something like: > > void SetCertReportingFinchConfig(const std::string& group_name, const > std::string& param_value) { > base::FieldTrialList::CreateFieldTrial(...); > // initialize |params| here > variations::AssociateVariationParams(...); > } Done. https://codereview.chromium.org/1035023002/diff/230001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:1135: base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors", On 2015/04/02 19:55:56, emily wrote: > nit: the "ReportCertificateErrors" and group name strings > ("ShowAndPossiblySend", "DontShowAndDontSend") should be constants. They should > probably be constants in ssl_blocking_page.cc too, come to think of it... so > maybe declare the constants in ssl_blocking_page.h and #include them to use > here? Done.
felt@ do you think you can look at this tomorrow? We can start getting data on M43 if we land this by tomorrow.
https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:115: kHTTPSErrorReporterFinchExperimentName) nit: this indentation is very odd, is this what git cl format did? https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:116: .compare(kHTTPSErrorReporterFinchGroupShowPossiblySend) == 0 && nit: you normally see this as FindFullName(name) == kGroup, instead of using .compare; it's a little cleaner IMO https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:123: // Check Finch parameters either leave off this comment (as it's obvious), or add more detail to it to include something non-obvious; the latter might be worthwhile, as this is an unusual use of finch params https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:127: if (param.compare("") != 0) { this is an awfully weird check too. if (!param.empty()) perhaps? https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:128: double possiblySend; nit: would "sendingThreshold" be a more descriptive variable name? https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:129: if (base::StringToDouble(param, &possiblySend)) you'll need { } for the outermost one because its inner contents are more than 1 line long https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:492: // command-line option is set. does this comment need to be updated? https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:685: const bool enabled = should this pref check be moved into ShouldShowCertificateReporterCheckbox?
https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.h:25: // Constants for the HTTPSErrorReporter Finch experiment Is this the right place in the declaration order for these?
https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:115: kHTTPSErrorReporterFinchExperimentName) On 2015/04/03 14:32:28, felt wrote: > nit: this indentation is very odd, is this what git cl format did? Yep, I usually run git cl format before git cl upload https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:116: .compare(kHTTPSErrorReporterFinchGroupShowPossiblySend) == 0 && On 2015/04/03 14:32:29, felt wrote: > nit: you normally see this as FindFullName(name) == kGroup, instead of using > .compare; it's a little cleaner IMO Done. https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:123: // Check Finch parameters On 2015/04/03 14:32:29, felt wrote: > either leave off this comment (as it's obvious), or add more detail to it to > include something non-obvious; the latter might be worthwhile, as this is an > unusual use of finch params Done. https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:127: if (param.compare("") != 0) { On 2015/04/03 14:32:29, felt wrote: > this is an awfully weird check too. if (!param.empty()) perhaps? Done. https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:128: double possiblySend; On 2015/04/03 14:32:28, felt wrote: > nit: would "sendingThreshold" be a more descriptive variable name? Done. https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:129: if (base::StringToDouble(param, &possiblySend)) On 2015/04/03 14:32:29, felt wrote: > you'll need { } for the outermost one because its inner contents are more than 1 > line long Done. https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:492: // command-line option is set. On 2015/04/03 14:32:29, felt wrote: > does this comment need to be updated? Done. https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:685: const bool enabled = On 2015/04/03 14:32:29, felt wrote: > should this pref check be moved into ShouldShowCertificateReporterCheckbox? Currently, IsPrefEnabled is a protected member of SecurityInterstitialPage. We either have to make ShouldShowCertificateReporterCheckbox a member of SSLBlockingPage or leave the check as it is. What do you think? https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.h:25: // Constants for the HTTPSErrorReporter Finch experiment On 2015/04/03 14:34:46, felt wrote: > Is this the right place in the declaration order for these? Done.
https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:685: const bool enabled = On 2015/04/03 17:39:31, fahl wrote: > On 2015/04/03 14:32:29, felt wrote: > > should this pref check be moved into ShouldShowCertificateReporterCheckbox? > > Currently, IsPrefEnabled is a protected member of SecurityInterstitialPage. We > either have to make ShouldShowCertificateReporterCheckbox a member of > SSLBlockingPage or leave the check as it is. What do you think? I don't think there's any problem with making ShouldShowCertificateReporterCheckbox a member of SSLBlockingPage, but I don't think moving this check into that function is correct. The call on line 493 should return true even if the preference is false.
On 2015/04/03 17:44:36, emily wrote: > https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_blocking_page.cc:685: const bool enabled = > On 2015/04/03 17:39:31, fahl wrote: > > On 2015/04/03 14:32:29, felt wrote: > > > should this pref check be moved into ShouldShowCertificateReporterCheckbox? > > > > Currently, IsPrefEnabled is a protected member of SecurityInterstitialPage. We > > either have to make ShouldShowCertificateReporterCheckbox a member of > > SSLBlockingPage or leave the check as it is. What do you think? > > I don't think there's any problem with making > ShouldShowCertificateReporterCheckbox a member of SSLBlockingPage, but I don't > think moving this check into that function is correct. The call on line 493 > should return true even if the preference is false. I agree. Given our time constraints, Adrienne, do you think the code is ready to be committed?
https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/250001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:685: const bool enabled = On 2015/04/03 17:44:36, emily wrote: > On 2015/04/03 17:39:31, fahl wrote: > > On 2015/04/03 14:32:29, felt wrote: > > > should this pref check be moved into ShouldShowCertificateReporterCheckbox? > > > > Currently, IsPrefEnabled is a protected member of SecurityInterstitialPage. We > > either have to make ShouldShowCertificateReporterCheckbox a member of > > SSLBlockingPage or leave the check as it is. What do you think? > > I don't think there's any problem with making > ShouldShowCertificateReporterCheckbox a member of SSLBlockingPage, but I don't > think moving this check into that function is correct. The call on line 493 > should return true even if the preference is false. you're right emily, nevermind sascha https://codereview.chromium.org/1035023002/diff/270001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/270001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:130: if (!sendingThreshold.empty()) { err, I must have commented on the wrong line before or something. what I meant was: replace |possiblySend| with |sendingThreshold| https://codereview.chromium.org/1035023002/diff/270001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1035023002/diff/270001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:486: if (param_value.compare("") != 0) { !param_value.empty()
https://codereview.chromium.org/1035023002/diff/270001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1035023002/diff/270001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:130: if (!sendingThreshold.empty()) { On 2015/04/03 22:38:09, felt wrote: > err, I must have commented on the wrong line before or something. what I meant > was: replace |possiblySend| with |sendingThreshold| Done. https://codereview.chromium.org/1035023002/diff/270001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1035023002/diff/270001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:486: if (param_value.compare("") != 0) { On 2015/04/03 22:38:09, felt wrote: > !param_value.empty() Done.
The CQ bit was checked by felt@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org Link to the patchset: https://codereview.chromium.org/1035023002/#ps290001 (title: "felt's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1035023002/290001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by fahl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/1035023002/#ps310001 (title: "fix conflicts")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1035023002/310001
https://codereview.chromium.org/1035023002/diff/310001/chrome/common/chrome_s... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1035023002/diff/310001/chrome/common/chrome_s... chrome/common/chrome_switches.cc:468: const char kEnableIconNtp[] = "enable-icon-ntp"; I don't think this should be here... mix-up during the conflict resolution? It looks like these kEnableIconNtp things were supposed to be deleted.
https://codereview.chromium.org/1035023002/diff/310001/chrome/common/chrome_s... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/1035023002/diff/310001/chrome/common/chrome_s... chrome/common/chrome_switches.h:137: extern const char kEnableIconNtp[]; same here
The CQ bit was checked by fahl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/1035023002/#ps330001 (title: "fix resolution mix-up")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1035023002/330001
Message was sent while issue was closed.
Committed patchset #12 (id:330001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/a9e9187b29a86e140021533b4a0d76da3bfbe386 Cr-Commit-Position: refs/heads/master@{#323870}
Message was sent while issue was closed.
https://codereview.chromium.org/1035023002/diff/310001/chrome/common/chrome_s... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1035023002/diff/310001/chrome/common/chrome_s... chrome/common/chrome_switches.cc:468: const char kEnableIconNtp[] = "enable-icon-ntp"; On 2015/04/04 01:17:06, emily wrote: > I don't think this should be here... mix-up during the conflict resolution? It > looks like these kEnableIconNtp things were supposed to be deleted. Done. https://codereview.chromium.org/1035023002/diff/310001/chrome/common/chrome_s... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/1035023002/diff/310001/chrome/common/chrome_s... chrome/common/chrome_switches.h:137: extern const char kEnableIconNtp[]; On 2015/04/04 01:17:34, emily wrote: > same here Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
