Add checkbox for reporting invalid TLS/SSL cert chains

Add checkbox for reporting invalid TLS/SSL cert chains

This is a stub for now; eventually it will report invalid certificate
chains on SSL interstitials to a server endpoint. Right now it logs the
report but doesn't actually send it anywhere. The checkbox is
tied in with Safe Browsing Extended Reporting.

BUG=461588
Committed: https://crrev.com/93272ab9ec84b4dd90a93fb2aab8ddd94f250d63
Cr-Commit-Position: refs/heads/master@{#322265}

[estark, 2015-02-17 20:14:47]

estark@chromium.org changed reviewers:
+ felt@chromium.org

[estark, 2015-02-17 20:14:47]

This is mostly just a stub, but I wanted to start getting some feedback. It adds
a checkbox to the SSL interstitial and a stub for reporting the invalid cert
chains to the server.

Still to-do:
* actually upload the information somewhere
* encryption
* collect both chains, not just the one
* Finch experiment
* enterprise policy to stop people from opting in (maybe it'll Just Work with
the SBER enterprise policy)

[felt, 2015-02-18 04:16:29]

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/interstit...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:88: namespace {
hmm, I'm not sure that this belongs here. normally constants are defined in the
.cc file, not in the header file. you should take a look at other files to see
how constants are normally done.

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:108: const
net::SSLInfo& ssl_info) {
Should this double check here that the user is opted in to the reporting
program? (Even though it doesn't actually do anything now.) (Or if you want to
delay that to another CL, add a TODO here for that?)

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:108: const
net::SSLInfo& ssl_info) {
Have you thought about having a single method for both, that differs based on a
bool? That might avoid repeating code down the line, and it would be nice for
the cert pinning reporter to get some/all of the same improvements that you'll
be adding.

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/security_warnings/interstitial_v2.html (right):

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/resources...
chrome/browser/resources/security_warnings/interstitial_v2.html:27: <input
type="checkbox" id="malware-opt-in-checkbox">
Is there a reason why you're adding a separate checkbox instead of reusing the
same one?

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:561:
ssl_error_classification_->GetRequestURL().host(), ssl_info_);
why are you pulling the hostname from ssl_error_classification_? I think you may
be looking for request_url_ instead

[estark, 2015-02-18 05:22:44]

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/interstit...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:88: namespace {
On 2015/02/18 04:16:29, felt wrote:
> hmm, I'm not sure that this belongs here. normally constants are defined in
the
> .cc file, not in the header file. you should take a look at other files to see
> how constants are normally done.

Oh, ok, I see. Based on //url/url_constants.h, I left the declarations here and
moved the definitions to the .cc file.

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:108: const
net::SSLInfo& ssl_info) {
On 2015/02/18 04:16:29, felt wrote:
> Should this double check here that the user is opted in to the reporting
> program? (Even though it doesn't actually do anything now.) (Or if you want to
> delay that to another CL, add a TODO here for that?)

Good idea, I added a TODO. Not immediately sure how to actually do this -- I
think we'll need access to the Profile, which I don't think we have here. Maybe
I can pass a Profile in to the ChromeFraudulentCertificateReporter constructor
and use it here to check the SBER preference... unless there is a much simpler
way that I'm missing?

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:108: const
net::SSLInfo& ssl_info) {
On 2015/02/18 04:16:29, felt wrote:
> Have you thought about having a single method for both, that differs based on
a
> bool? That might avoid repeating code down the line, and it would be nice for
> the cert pinning reporter to get some/all of the same improvements that you'll
> be adding.

Yeah, I did think about that; my reasoning was that I could factor out common
code into a helper method, and that
`reporter->SendPinningViolationReport(hostname, ssl_info)` is a little more
readable than `reporter->SendReport(hostname, ssl_info, true)` (i.e. so that the
reader doesn't have to always look up that boolean parameter and figure out what
true/false means), and that having different method names makes it easier to
expand to other types of reports later (e.g. we might later want to add
SendHPKPReport). Whaddya think?

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/security_warnings/interstitial_v2.html (right):

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/resources...
chrome/browser/resources/security_warnings/interstitial_v2.html:27: <input
type="checkbox" id="malware-opt-in-checkbox">
On 2015/02/18 04:16:29, felt wrote:
> Is there a reason why you're adding a separate checkbox instead of reusing the
> same one?

Hmm, no! I think when I started I thought they would need to have different
labels and onclick behavior and stuff... Changed it back to one.

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:561:
ssl_error_classification_->GetRequestURL().host(), ssl_info_);
On 2015/02/18 04:16:29, felt wrote:
> why are you pulling the hostname from ssl_error_classification_? I think you
may
> be looking for request_url_ instead

Done.

[felt, 2015-02-19 00:32:32]

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:108: const
net::SSLInfo& ssl_info) {
On 2015/02/18 05:22:44, emily wrote:
> On 2015/02/18 04:16:29, felt wrote:
> > Should this double check here that the user is opted in to the reporting
> > program? (Even though it doesn't actually do anything now.) (Or if you want
to
> > delay that to another CL, add a TODO here for that?)
> 
> Good idea, I added a TODO. Not immediately sure how to actually do this -- I
> think we'll need access to the Profile, which I don't think we have here.
Maybe
> I can pass a Profile in to the ChromeFraudulentCertificateReporter constructor
> and use it here to check the SBER preference... unless there is a much simpler
> way that I'm missing?

Yes, I think that passing in a Profile to the constructor would be the best way
to do this. TODO works for now.

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:108: const
net::SSLInfo& ssl_info) {
On 2015/02/18 05:22:44, emily wrote:
> On 2015/02/18 04:16:29, felt wrote:
> > Have you thought about having a single method for both, that differs based
on
> a
> > bool? That might avoid repeating code down the line, and it would be nice
for
> > the cert pinning reporter to get some/all of the same improvements that
you'll
> > be adding.
> 
> Yeah, I did think about that; my reasoning was that I could factor out common
> code into a helper method, and that
> `reporter->SendPinningViolationReport(hostname, ssl_info)` is a little more
> readable than `reporter->SendReport(hostname, ssl_info, true)` (i.e. so that
the
> reader doesn't have to always look up that boolean parameter and figure out
what
> true/false means), and that having different method names makes it easier to
> expand to other types of reports later (e.g. we might later want to add
> SendHPKPReport). Whaddya think?

Well, you could create an enum so that the param is clear (e.g. HPKP,
GOOGLE_PIN, EXTENDED_REPORTING) to avoid the boolean parameter.

Another thing to think about: with this current structure, we will send two
reports for each google.com pinning violation. One via SendInvalidChainReport,
and one via SendPinningViolationReport from its separate call site. You should
decide whether that's desirable or not in the future. If you end up condensing
them to a single call site that decides which one to send it to, then having two
separate methods might not make sense.

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/interstit...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:66: class
SecurityInterstitialPageWithExtendedReporting
I'm debating the pros and cons of having this as a separate subclass vs just
putting these methods directly on the SecurityInterstitialPage, with the captive
portal simply not implementing anything for PopulateExtendedReportingOption. Why
did you choose to do it this way (with a separate subclass)?

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/security_warnings/interstitial_v2.html (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/resources...
chrome/browser/resources/security_warnings/interstitial_v2.html:49: </html>
I can't tell what changed here (removed newline?), can you just revert all
changes to the file so it doesn't show up as changed at all?

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/security_warnings/safe_browsing.js (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/resources...
chrome/browser/resources/security_warnings/safe_browsing.js:15: // Must match
SSLBlockingPageCommands in ssl_blocking_page.h.
Hmm, seems unfortunate to have SSLBlockingPageCommands in safe_browsing.js.
Maybe since this code is now shared, setupCheckbox should be moved into the
shared .js file?

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/resources...
chrome/browser/resources/security_warnings/safe_browsing.js:28: if (type !=
'SAFEBROWSING' && type != 'SSL') {
what other type might this be? what are you checking for here?

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/resources...
chrome/browser/resources/security_warnings/safe_browsing.js:32: if
(loadTimeData.getString('type') != type ||
if you're going to check loadTimeData.getString('type') again here, why are you
also passing the value in as a parameter? seems like you should do one or the
other?

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:613: base::DictionaryValue*
load_time_data) {
Should we exclude Lucas's clock interstitials, both from showing a checkbox and
from uploading?

[estark, 2015-02-19 02:32:09]

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:108: const
net::SSLInfo& ssl_info) {
On 2015/02/19 00:32:32, felt wrote:
> On 2015/02/18 05:22:44, emily wrote:
> > On 2015/02/18 04:16:29, felt wrote:
> > > Have you thought about having a single method for both, that differs based
> on
> > a
> > > bool? That might avoid repeating code down the line, and it would be nice
> for
> > > the cert pinning reporter to get some/all of the same improvements that
> you'll
> > > be adding.
> > 
> > Yeah, I did think about that; my reasoning was that I could factor out
common
> > code into a helper method, and that
> > `reporter->SendPinningViolationReport(hostname, ssl_info)` is a little more
> > readable than `reporter->SendReport(hostname, ssl_info, true)` (i.e. so that
> the
> > reader doesn't have to always look up that boolean parameter and figure out
> what
> > true/false means), and that having different method names makes it easier to
> > expand to other types of reports later (e.g. we might later want to add
> > SendHPKPReport). Whaddya think?
> 
> Well, you could create an enum so that the param is clear (e.g. HPKP,
> GOOGLE_PIN, EXTENDED_REPORTING) to avoid the boolean parameter.
> 
> Another thing to think about: with this current structure, we will send two
> reports for each http://google.com pinning violation. One via
SendInvalidChainReport,
> and one via SendPinningViolationReport from its separate call site. You should
> decide whether that's desirable or not in the future. If you end up condensing
> them to a single call site that decides which one to send it to, then having
two
> separate methods might not make sense.

Oh I like the enum idea! Did that. Wasn't sure whether to make it the first
argument or the last... eventually made it the first arg because it looked a
little cleaner to me but no super strong opinion, and definitely willing to be
convinced otherwise.

My initial reaction to the google.com pinning thing is that it makes sense to
get two separate reports so that we can easily answer questions such as "what %
of SSL errors are pinning violations?", but I'll have to think about that a bit.
(Maybe we already have data to answer that question?)

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/interstit...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:66: class
SecurityInterstitialPageWithExtendedReporting
On 2015/02/19 00:32:32, felt wrote:
> I'm debating the pros and cons of having this as a separate subclass vs just
> putting these methods directly on the SecurityInterstitialPage, with the
captive
> portal simply not implementing anything for PopulateExtendedReportingOption.
Why
> did you choose to do it this way (with a separate subclass)?

Hmm, TBH the latter option didn't really occur to me. What are the pros and cons
you're thinking of? The main (somewhat superficial) argument I can think of is
that SecurityInterstitialPageWithExtendedReporting is just a really annoyingly
long name.

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/security_warnings/interstitial_v2.html (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/resources...
chrome/browser/resources/security_warnings/interstitial_v2.html:49: </html>
On 2015/02/19 00:32:32, felt wrote:
> I can't tell what changed here (removed newline?), can you just revert all
> changes to the file so it doesn't show up as changed at all?

Done.

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/security_warnings/safe_browsing.js (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/resources...
chrome/browser/resources/security_warnings/safe_browsing.js:15: // Must match
SSLBlockingPageCommands in ssl_blocking_page.h.
On 2015/02/19 00:32:32, felt wrote:
> Hmm, seems unfortunate to have SSLBlockingPageCommands in safe_browsing.js.
> Maybe since this code is now shared, setupCheckbox should be moved into the
> shared .js file?

Hm, when you say "setupCheckbox should be moved" you mean "all the code in this
file", right? (I don't see another way to do it.) It does seem sort of nice to
have the separate file just for cleanliness, so what do you think of renaming
this file to extended_reporting.js, and maybe also renaming setupCheckbox to
setupExtendedReportingCheckbox?

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/resources...
chrome/browser/resources/security_warnings/safe_browsing.js:28: if (type !=
'SAFEBROWSING' && type != 'SSL') {
On 2015/02/19 00:32:32, felt wrote:
> what other type might this be? what are you checking for here?

Oops, this check was blatantly wrong: replaced it with an early return for
irrelevant types.

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/resources...
chrome/browser/resources/security_warnings/safe_browsing.js:32: if
(loadTimeData.getString('type') != type ||
On 2015/02/19 00:32:32, felt wrote:
> if you're going to check loadTimeData.getString('type') again here, why are
you
> also passing the value in as a parameter? seems like you should do one or the
> other?

Done.

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:613: base::DictionaryValue*
load_time_data) {
On 2015/02/19 00:32:32, felt wrote:
> Should we exclude Lucas's clock interstitials, both from showing a checkbox
and
> from uploading?

Done.

[estark, 2015-02-19 02:37:30]

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:613: base::DictionaryValue*
load_time_data) {
On 2015/02/19 00:32:32, felt wrote:
> Should we exclude Lucas's clock interstitials, both from showing a checkbox
and
> from uploading?

On second thought, I do think at some point it could be interesting to collect
data from the clock interstitials... for example, I wonder if users are ever
getting "red herring" clock interstitials, where they received a bad cert but
saw the clock interstitial because their clock happened to be off. Still, I
think it's fine to not collect data from clock interstitials for now to avoid
overwhelming our data set with them, and we can always turn it on later.

[felt, 2015-02-20 19:20:34]

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/935663004/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:108: const
net::SSLInfo& ssl_info) {
On 2015/02/19 02:32:08, emily wrote:
> On 2015/02/19 00:32:32, felt wrote:
> > On 2015/02/18 05:22:44, emily wrote:
> > > On 2015/02/18 04:16:29, felt wrote:
> > > > Have you thought about having a single method for both, that differs
based
> > on
> > > a
> > > > bool? That might avoid repeating code down the line, and it would be
nice
> > for
> > > > the cert pinning reporter to get some/all of the same improvements that
> > you'll
> > > > be adding.
> > > 
> > > Yeah, I did think about that; my reasoning was that I could factor out
> common
> > > code into a helper method, and that
> > > `reporter->SendPinningViolationReport(hostname, ssl_info)` is a little
more
> > > readable than `reporter->SendReport(hostname, ssl_info, true)` (i.e. so
that
> > the
> > > reader doesn't have to always look up that boolean parameter and figure
out
> > what
> > > true/false means), and that having different method names makes it easier
to
> > > expand to other types of reports later (e.g. we might later want to add
> > > SendHPKPReport). Whaddya think?
> > 
> > Well, you could create an enum so that the param is clear (e.g. HPKP,
> > GOOGLE_PIN, EXTENDED_REPORTING) to avoid the boolean parameter.
> > 
> > Another thing to think about: with this current structure, we will send two
> > reports for each http://google.com pinning violation. One via
> SendInvalidChainReport,
> > and one via SendPinningViolationReport from its separate call site. You
should
> > decide whether that's desirable or not in the future. If you end up
condensing
> > them to a single call site that decides which one to send it to, then having
> two
> > separate methods might not make sense.
> 
> Oh I like the enum idea! Did that. Wasn't sure whether to make it the first
> argument or the last... eventually made it the first arg because it looked a
> little cleaner to me but no super strong opinion, and definitely willing to be
> convinced otherwise.
> 
> My initial reaction to the http://google.com pinning thing is that it makes
sense to
> get two separate reports so that we can easily answer questions such as "what
%
> of SSL errors are pinning violations?", but I'll have to think about that a
bit.
> (Maybe we already have data to answer that question?)

Might make sense. I can see it go either way. We can perhaps use UMA to answer
that specific question.

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/interstit...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:66: class
SecurityInterstitialPageWithExtendedReporting
On 2015/02/19 02:32:08, emily wrote:
> On 2015/02/19 00:32:32, felt wrote:
> > I'm debating the pros and cons of having this as a separate subclass vs just
> > putting these methods directly on the SecurityInterstitialPage, with the
> captive
> > portal simply not implementing anything for PopulateExtendedReportingOption.
> Why
> > did you choose to do it this way (with a separate subclass)?
> 
> Hmm, TBH the latter option didn't really occur to me. What are the pros and
cons
> you're thinking of? The main (somewhat superficial) argument I can think of is
> that SecurityInterstitialPageWithExtendedReporting is just a really annoyingly
> long name.

Pro - it cleanly separates out the reporting logic

Con - it seems like a possibly unnecessary extra subclass; furthermore there may
be interstitials that don't show the reporting stuff anyway (e.g. enterprise
config) and so it isn't really a clean separateion between pages with and
without extended reporting

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:613: base::DictionaryValue*
load_time_data) {
On 2015/02/19 02:37:30, emily wrote:
> On 2015/02/19 00:32:32, felt wrote:
> > Should we exclude Lucas's clock interstitials, both from showing a checkbox
> and
> > from uploading?
> 
> On second thought, I do think at some point it could be interesting to collect
> data from the clock interstitials... for example, I wonder if users are ever
> getting "red herring" clock interstitials, where they received a bad cert but
> saw the clock interstitial because their clock happened to be off. Still, I
> think it's fine to not collect data from clock interstitials for now to avoid
> overwhelming our data set with them, and we can always turn it on later.

That's an interesting point, I hadn't thought of that. The weird thing though is
that the checkbox text might not make sense on the clock interstitial. Would we
want to upload the clock interstitial if the user is already opted in, but not
show the checkbox? The downside of that is that it might skew the %ages.

[estark, 2015-02-21 06:53:04]

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:613: base::DictionaryValue*
load_time_data) {
On 2015/02/20 19:20:34, felt wrote:
> On 2015/02/19 02:37:30, emily wrote:
> > On 2015/02/19 00:32:32, felt wrote:
> > > Should we exclude Lucas's clock interstitials, both from showing a
checkbox
> > and
> > > from uploading?
> > 
> > On second thought, I do think at some point it could be interesting to
collect
> > data from the clock interstitials... for example, I wonder if users are ever
> > getting "red herring" clock interstitials, where they received a bad cert
but
> > saw the clock interstitial because their clock happened to be off. Still, I
> > think it's fine to not collect data from clock interstitials for now to
avoid
> > overwhelming our data set with them, and we can always turn it on later.
> 
> That's an interesting point, I hadn't thought of that. The weird thing though
is
> that the checkbox text might not make sense on the clock interstitial. Would
we
> want to upload the clock interstitial if the user is already opted in, but not
> show the checkbox? The downside of that is that it might skew the %ages.

Hmm, so I can think of two or three ways that we could possibly address this:
1. Ideally, maybe it doesn't even make sense to show the clock interstitial at
all if the certificate would fail to validate for some other reason. Or maybe we
should be showing some hybrid interstitial that says both "you should fix your
clock" and "this website is broken." But this sounds kind of hard to implement.
2. Maybe we could use special wording on the checkbox label for the clock
interstitial so that it makes more sense, though I'm not coming up with anything
great at the moment.
3. We briefly discussed today the possibility of recording the causes of the
validation failures in addition to the chains themselves (wrong name, expired
cert, pinning violation, etc.). If we do that, we could go with your idea
(report the clock interstitial if the user is already opted in) and report
inaccurate client clock as the reason for the validation failure. That could
give us some potentially useful data (e.g. reports of red herring clock
interstitials) but would allow us to easily exclude the clock interstitials when
calculating percentages.

[estark, 2015-02-23 20:42:18]

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/interstit...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:66: class
SecurityInterstitialPageWithExtendedReporting
On 2015/02/20 19:20:34, felt wrote:
> On 2015/02/19 02:32:08, emily wrote:
> > On 2015/02/19 00:32:32, felt wrote:
> > > I'm debating the pros and cons of having this as a separate subclass vs
just
> > > putting these methods directly on the SecurityInterstitialPage, with the
> > captive
> > > portal simply not implementing anything for
PopulateExtendedReportingOption.
> > Why
> > > did you choose to do it this way (with a separate subclass)?
> > 
> > Hmm, TBH the latter option didn't really occur to me. What are the pros and
> cons
> > you're thinking of? The main (somewhat superficial) argument I can think of
is
> > that SecurityInterstitialPageWithExtendedReporting is just a really
annoyingly
> > long name.
> 
> Pro - it cleanly separates out the reporting logic
> 
> Con - it seems like a possibly unnecessary extra subclass; furthermore there
may
> be interstitials that don't show the reporting stuff anyway (e.g. enterprise
> config) and so it isn't really a clean separateion between pages with and
> without extended reporting

Okay, I find that Con fairly convincing -- done.

[felt, 2015-02-24 01:57:53]

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/security_warnings/safe_browsing.js (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/resources...
chrome/browser/resources/security_warnings/safe_browsing.js:15: // Must match
SSLBlockingPageCommands in ssl_blocking_page.h.
On 2015/02/19 02:32:08, emily wrote:
> On 2015/02/19 00:32:32, felt wrote:
> > Hmm, seems unfortunate to have SSLBlockingPageCommands in safe_browsing.js.
> > Maybe since this code is now shared, setupCheckbox should be moved into the
> > shared .js file?
> 
> Hm, when you say "setupCheckbox should be moved" you mean "all the code in
this
> file", right? (I don't see another way to do it.) It does seem sort of nice to
> have the separate file just for cleanliness, so what do you think of renaming
> this file to extended_reporting.js, and maybe also renaming setupCheckbox to
> setupExtendedReportingCheckbox?

Yes, renaming it to extended_reporting.js could work.

The only remaining issue is w/r/t the constants. As you notice all the other
constants for ssl are defined in ssl.js. I believe that is OK, you can put them
there and they will still be usable in this scope. But double check in case I'm
mis-remembering.

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:613: base::DictionaryValue*
load_time_data) {
On 2015/02/21 06:53:04, emily wrote:
> On 2015/02/20 19:20:34, felt wrote:
> > On 2015/02/19 02:37:30, emily wrote:
> > > On 2015/02/19 00:32:32, felt wrote:
> > > > Should we exclude Lucas's clock interstitials, both from showing a
> checkbox
> > > and
> > > > from uploading?
> > > 
> > > On second thought, I do think at some point it could be interesting to
> collect
> > > data from the clock interstitials... for example, I wonder if users are
ever
> > > getting "red herring" clock interstitials, where they received a bad cert
> but
> > > saw the clock interstitial because their clock happened to be off. Still,
I
> > > think it's fine to not collect data from clock interstitials for now to
> avoid
> > > overwhelming our data set with them, and we can always turn it on later.
> > 
> > That's an interesting point, I hadn't thought of that. The weird thing
though
> is
> > that the checkbox text might not make sense on the clock interstitial. Would
> we
> > want to upload the clock interstitial if the user is already opted in, but
not
> > show the checkbox? The downside of that is that it might skew the %ages.
> 
> Hmm, so I can think of two or three ways that we could possibly address this:
> 1. Ideally, maybe it doesn't even make sense to show the clock interstitial at
> all if the certificate would fail to validate for some other reason. Or maybe
we
> should be showing some hybrid interstitial that says both "you should fix your
> clock" and "this website is broken." But this sounds kind of hard to
implement.
> 2. Maybe we could use special wording on the checkbox label for the clock
> interstitial so that it makes more sense, though I'm not coming up with
anything
> great at the moment.
> 3. We briefly discussed today the possibility of recording the causes of the
> validation failures in addition to the chains themselves (wrong name, expired
> cert, pinning violation, etc.). If we do that, we could go with your idea
> (report the clock interstitial if the user is already opted in) and report
> inaccurate client clock as the reason for the validation failure. That could
> give us some potentially useful data (e.g. reports of red herring clock
> interstitials) but would allow us to easily exclude the clock interstitials
when
> calculating percentages.

My suggestion to proceed here: include the clock interstitial for now, but take
a screenshot of the checkbox on the clock interstitial and check with Alex
Ainslie (ainslie@) about whether it looks OK.

https://codereview.chromium.org/935663004/diff/120001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/120001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:79: namespace
interstitials {
nit: I feel like this should be above the class declaration, although I can't
find a style guideline to support that.

https://codereview.chromium.org/935663004/diff/120001/chrome/browser/resource...
File chrome/browser/resources/security_warnings/safe_browsing.js (right):

https://codereview.chromium.org/935663004/diff/120001/chrome/browser/resource...
chrome/browser/resources/security_warnings/safe_browsing.js:45: SB_CMD_DO_REPORT
: SSL_CMD_DO_REPORT;
oof, these commands need to be unified in a future clean-up CL. driving me nuts
that SB and SSL have separate commands. (note: not expecting you to do that,
just noting to myself that this needs a refactor.)

https://codereview.chromium.org/935663004/diff/120001/chrome/browser/ssl/capt...
File chrome/browser/ssl/captive_portal_blocking_page.h (right):

https://codereview.chromium.org/935663004/diff/120001/chrome/browser/ssl/capt...
chrome/browser/ssl/captive_portal_blocking_page.h:60: base::DictionaryValue*
load_time_data) override{};
nit: missing a space?

https://codereview.chromium.org/935663004/diff/120001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/120001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:648:
l10n_util::GetStringFUTF16(IDS_SAFE_BROWSING_MALWARE_REPORTING_AGREE,
note: in a future CL you should probably rename these strings now that they're
being used for a more general interstitial opt-in

https://codereview.chromium.org/935663004/diff/120001/net/url_request/fraudul...
File net/url_request/fraudulent_certificate_reporter.h (right):

https://codereview.chromium.org/935663004/diff/120001/net/url_request/fraudul...
net/url_request/fraudulent_certificate_reporter.h:19: enum ReportType {
REPORT_TYPE_PIN_VIOLATION, REPORT_TYPE_EXTENDED_REPORTING };
nit: I've never seen an enum formatted like this. more normal would be to put
each enum name on its own line like

REPORT_TYPE_PIN_VIOLATION,
REPORT_TYPE_EXTENDED_REPORTING

[estark, 2015-02-24 18:47:09]

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/security_warnings/safe_browsing.js (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/resources...
chrome/browser/resources/security_warnings/safe_browsing.js:15: // Must match
SSLBlockingPageCommands in ssl_blocking_page.h.
On 2015/02/24 01:57:52, felt wrote:
> On 2015/02/19 02:32:08, emily wrote:
> > On 2015/02/19 00:32:32, felt wrote:
> > > Hmm, seems unfortunate to have SSLBlockingPageCommands in
safe_browsing.js.
> > > Maybe since this code is now shared, setupCheckbox should be moved into
the
> > > shared .js file?
> > 
> > Hm, when you say "setupCheckbox should be moved" you mean "all the code in
> this
> > file", right? (I don't see another way to do it.) It does seem sort of nice
to
> > have the separate file just for cleanliness, so what do you think of
renaming
> > this file to extended_reporting.js, and maybe also renaming setupCheckbox to
> > setupExtendedReportingCheckbox?
> 
> Yes, renaming it to extended_reporting.js could work.
> 
> The only remaining issue is w/r/t the constants. As you notice all the other
> constants for ssl are defined in ssl.js. I believe that is OK, you can put
them
> there and they will still be usable in this scope. But double check in case
I'm
> mis-remembering.

Done.

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/40001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:613: base::DictionaryValue*
load_time_data) {
On 2015/02/24 01:57:52, felt wrote:
> On 2015/02/21 06:53:04, emily wrote:
> > On 2015/02/20 19:20:34, felt wrote:
> > > On 2015/02/19 02:37:30, emily wrote:
> > > > On 2015/02/19 00:32:32, felt wrote:
> > > > > Should we exclude Lucas's clock interstitials, both from showing a
> > checkbox
> > > > and
> > > > > from uploading?
> > > > 
> > > > On second thought, I do think at some point it could be interesting to
> > collect
> > > > data from the clock interstitials... for example, I wonder if users are
> ever
> > > > getting "red herring" clock interstitials, where they received a bad
cert
> > but
> > > > saw the clock interstitial because their clock happened to be off.
Still,
> I
> > > > think it's fine to not collect data from clock interstitials for now to
> > avoid
> > > > overwhelming our data set with them, and we can always turn it on later.
> > > 
> > > That's an interesting point, I hadn't thought of that. The weird thing
> though
> > is
> > > that the checkbox text might not make sense on the clock interstitial.
Would
> > we
> > > want to upload the clock interstitial if the user is already opted in, but
> not
> > > show the checkbox? The downside of that is that it might skew the %ages.
> > 
> > Hmm, so I can think of two or three ways that we could possibly address
this:
> > 1. Ideally, maybe it doesn't even make sense to show the clock interstitial
at
> > all if the certificate would fail to validate for some other reason. Or
maybe
> we
> > should be showing some hybrid interstitial that says both "you should fix
your
> > clock" and "this website is broken." But this sounds kind of hard to
> implement.
> > 2. Maybe we could use special wording on the checkbox label for the clock
> > interstitial so that it makes more sense, though I'm not coming up with
> anything
> > great at the moment.
> > 3. We briefly discussed today the possibility of recording the causes of the
> > validation failures in addition to the chains themselves (wrong name,
expired
> > cert, pinning violation, etc.). If we do that, we could go with your idea
> > (report the clock interstitial if the user is already opted in) and report
> > inaccurate client clock as the reason for the validation failure. That could
> > give us some potentially useful data (e.g. reports of red herring clock
> > interstitials) but would allow us to easily exclude the clock interstitials
> when
> > calculating percentages.
> 
> My suggestion to proceed here: include the clock interstitial for now, but
take
> a screenshot of the checkbox on the clock interstitial and check with Alex
> Ainslie (ainslie@) about whether it looks OK.

Done -- cc'ed you on the email to ainslie.

https://codereview.chromium.org/935663004/diff/120001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/120001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:79: namespace
interstitials {
On 2015/02/24 01:57:53, felt wrote:
> nit: I feel like this should be above the class declaration, although I can't
> find a style guideline to support that.

Done -- there are other |namespace {...}|s up there so it makes sense even
without the style guideline. :)

https://codereview.chromium.org/935663004/diff/120001/chrome/browser/resource...
File chrome/browser/resources/security_warnings/safe_browsing.js (right):

https://codereview.chromium.org/935663004/diff/120001/chrome/browser/resource...
chrome/browser/resources/security_warnings/safe_browsing.js:45: SB_CMD_DO_REPORT
: SSL_CMD_DO_REPORT;
On 2015/02/24 01:57:53, felt wrote:
> oof, these commands need to be unified in a future clean-up CL. driving me
nuts
> that SB and SSL have separate commands. (note: not expecting you to do that,
> just noting to myself that this needs a refactor.)

noted!

https://codereview.chromium.org/935663004/diff/120001/chrome/browser/ssl/capt...
File chrome/browser/ssl/captive_portal_blocking_page.h (right):

https://codereview.chromium.org/935663004/diff/120001/chrome/browser/ssl/capt...
chrome/browser/ssl/captive_portal_blocking_page.h:60: base::DictionaryValue*
load_time_data) override{};
On 2015/02/24 01:57:53, felt wrote:
> nit: missing a space?

Hmm, 'git cl format' removes it if I add a space there :(

https://codereview.chromium.org/935663004/diff/120001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/120001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:648:
l10n_util::GetStringFUTF16(IDS_SAFE_BROWSING_MALWARE_REPORTING_AGREE,
On 2015/02/24 01:57:53, felt wrote:
> note: in a future CL you should probably rename these strings now that they're
> being used for a more general interstitial opt-in

added to my list

https://codereview.chromium.org/935663004/diff/120001/net/url_request/fraudul...
File net/url_request/fraudulent_certificate_reporter.h (right):

https://codereview.chromium.org/935663004/diff/120001/net/url_request/fraudul...
net/url_request/fraudulent_certificate_reporter.h:19: enum ReportType {
REPORT_TYPE_PIN_VIOLATION, REPORT_TYPE_EXTENDED_REPORTING };
On 2015/02/24 01:57:53, felt wrote:
> nit: I've never seen an enum formatted like this. more normal would be to put
> each enum name on its own line like
> 
> REPORT_TYPE_PIN_VIOLATION,
> REPORT_TYPE_EXTENDED_REPORTING

This is another thing that 'git cl format' undoes.

[estark, 2015-02-24 18:48:35]

estark@chromium.org changed reviewers:
+ rsleevi@chromium.org

[estark, 2015-02-24 18:48:36]

rsleevi, could you please take a look? Thanks! This is the first part of the
implementation for reporting invalid cert chains
(https://docs.google.com/a/google.com/document/d/1T-CbdkVw92RHq3_NnR9_LgHoJEgK...).

And thanks Adrienne for all the helpful comments so far.

[felt, 2015-02-24 18:55:40]

On 2015/02/24 18:48:36, emily wrote:
> rsleevi, could you please take a look? Thanks! This is the first part of the
> implementation for reporting invalid cert chains
>
(https://docs.google.com/a/google.com/document/d/1T-CbdkVw92RHq3_NnR9_LgHoJEgK...).
> 
> And thanks Adrienne for all the helpful comments so far.

btw, I just noticed in the screenshot that the white checkbox does not work on
the light gray background. you will need to add a CSS rule to style the checkbox
differently for the light gray background of ssl warnings.

[estark, 2015-02-24 19:58:12]

On 2015/02/24 18:55:40, felt wrote:
> On 2015/02/24 18:48:36, emily wrote:
> > rsleevi, could you please take a look? Thanks! This is the first part of the
> > implementation for reporting invalid cert chains
> >
>
(https://docs.google.com/a/google.com/document/d/1T-CbdkVw92RHq3_NnR9_LgHoJEgK...).
> > 
> > And thanks Adrienne for all the helpful comments so far.
> 
> btw, I just noticed in the screenshot that the white checkbox does not work on
> the light gray background. you will need to add a CSS rule to style the
checkbox
> differently for the light gray background of ssl warnings.

fixed in patch set 12

[Ryan Sleevi, 2015-02-25 23:07:49]

Subtle threading danger below

https://codereview.chromium.org/935663004/diff/210001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/210001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:558: net::URLRequestContext*
request_context = web_contents()
THREADING BUG: It's only legal to access the fraudulent cert reporter on the IO
thread (It's full of URLRequest-y goodness)

You're calling here from the UI thread (judging by line 535 and friends). You
need to marshall this over to the IO thread before accessing.

The Getter you an pass from the UI thread to the IO thread, but the Context you
can only use on the IO thread.

https://codereview.chromium.org/935663004/diff/210001/net/url_request/fraudul...
File net/url_request/fraudulent_certificate_reporter.h (right):

https://codereview.chromium.org/935663004/diff/210001/net/url_request/fraudul...
net/url_request/fraudulent_certificate_reporter.h:19: enum ReportType {
REPORT_TYPE_PIN_VIOLATION, REPORT_TYPE_EXTENDED_REPORTING };
Document

https://codereview.chromium.org/935663004/diff/210001/net/url_request/fraudul...
net/url_request/fraudulent_certificate_reporter.h:25: virtual void
SendReport(ReportType type,
Update documentation

[estark, 2015-02-26 07:01:27]

Thanks Ryan. I thiiiink I fixed the threading bug (?), and added some comments.

https://codereview.chromium.org/935663004/diff/210001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/210001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:558: net::URLRequestContext*
request_context = web_contents()
On 2015/02/25 23:07:49, Ryan Sleevi wrote:
> THREADING BUG: It's only legal to access the fraudulent cert reporter on the
IO
> thread (It's full of URLRequest-y goodness)
> 
> You're calling here from the UI thread (judging by line 535 and friends). You
> need to marshall this over to the IO thread before accessing.
> 
> The Getter you an pass from the UI thread to the IO thread, but the Context
you
> can only use on the IO thread.

Done -- I think. Doing this on the IO thread causes the browser tests for this
feature to get a little flaky, presumably because the report is not necessarily
sent at the point where the test checks whether the report has been sent? I
typed a random sequence of characters in the test --
base::RunLoop().RunUntilIdle() -- that seemed to make the flakiness go away, but
I couldn't quite figure out if that makes any sense or not. :)

https://codereview.chromium.org/935663004/diff/210001/net/url_request/fraudul...
File net/url_request/fraudulent_certificate_reporter.h (right):

https://codereview.chromium.org/935663004/diff/210001/net/url_request/fraudul...
net/url_request/fraudulent_certificate_reporter.h:19: enum ReportType {
REPORT_TYPE_PIN_VIOLATION, REPORT_TYPE_EXTENDED_REPORTING };
On 2015/02/25 23:07:49, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/935663004/diff/210001/net/url_request/fraudul...
net/url_request/fraudulent_certificate_reporter.h:25: virtual void
SendReport(ReportType type,
On 2015/02/25 23:07:49, Ryan Sleevi wrote:
> Update documentation

Done.

[Ryan Sleevi, 2015-02-27 00:46:34]

Almost got it :)

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:550: void
SSLBlockingPage::FinishCertCollectionInternal(
There's no need for this to be a static class member, is there?

This is an internal implementation detail, not accessing any access-controlled
data members, so you should be able to move this to line 218 as just

void FinishCertCollectionInternal(...) { }

This also avoids having to change the .h file if/when your internal
implementation changes.

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:557: reporter->SendReport(
API CONTRACT VIOLATION:

The URLRequestContext allows |fradulent_certificate_reporter()| to return NULL.
You should handle that here.

(e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...
)

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.h:18: #include
"net/url_request/url_request_context_getter.h"
Forward declare

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.h:117:
scoped_refptr<net::URLRequestContextGetter> request_context_getter,
This should be "const scoped_refptr<net::URLRequestContextGetter>&"

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:394: base::RunLoop().RunUntilIdle();
May want a comment explaining why this is.

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:421: class MockReporter : public
ChromeFraudulentCertificateReporter {
Since you're not relying on this class being able to access members of this
test, I'd instead move this from being a nested class to being a member of an
unnamed namespace, moving it wholesale to line 177

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:425: latest_hostname_reported_ = "";
This is not necessary. Strings are default initialized to empty

(Note, in terms of preference, prefer .clear(), then = std::string(), then = "";
although all are unnecessary here)

[estark, 2015-02-27 02:49:37]

Addressed rsleevi's most recent round of comments. Also found that there was a
similar threading bug in the browser tests I had written, so I fixed that. (TIL
that I should always run Debug/browser_tests instead of Release/, those
assertions are handy...)

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:550: void
SSLBlockingPage::FinishCertCollectionInternal(
On 2015/02/27 00:46:33, Ryan Sleevi wrote:
> There's no need for this to be a static class member, is there?
> 
> This is an internal implementation detail, not accessing any access-controlled
> data members, so you should be able to move this to line 218 as just
> 
> void FinishCertCollectionInternal(...) { }
> 
> This also avoids having to change the .h file if/when your internal
> implementation changes.

Done.

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:557: reporter->SendReport(
On 2015/02/27 00:46:33, Ryan Sleevi wrote:
> API CONTRACT VIOLATION:
> 
> The URLRequestContext allows |fradulent_certificate_reporter()| to return
NULL.
> You should handle that here.
> 
> (e.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...
> )

Done.

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.h:18: #include
"net/url_request/url_request_context_getter.h"
On 2015/02/27 00:46:33, Ryan Sleevi wrote:
> Forward declare

Just removed the #include all together because I moved
|FinishCertCollectionInternal| out of the class, but this prompted me to go look
up when to forward declare in the style guide, so hooray I learned something
useful :)

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.h:117:
scoped_refptr<net::URLRequestContextGetter> request_context_getter,
On 2015/02/27 00:46:33, Ryan Sleevi wrote:
> This should be "const scoped_refptr<net::URLRequestContextGetter>&"

Done.

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:394: base::RunLoop().RunUntilIdle();
On 2015/02/27 00:46:33, Ryan Sleevi wrote:
> May want a comment explaining why this is.

Done.

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:421: class MockReporter : public
ChromeFraudulentCertificateReporter {
On 2015/02/27 00:46:33, Ryan Sleevi wrote:
> Since you're not relying on this class being able to access members of this
> test, I'd instead move this from being a nested class to being a member of an
> unnamed namespace, moving it wholesale to line 177

Done.

https://codereview.chromium.org/935663004/diff/230001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:425: latest_hostname_reported_ = "";
On 2015/02/27 00:46:33, Ryan Sleevi wrote:
> This is not necessary. Strings are default initialized to empty
> 
> (Note, in terms of preference, prefer .clear(), then = std::string(), then =
"";
> although all are unnecessary here)

Done.

[Ryan Sleevi, 2015-02-27 03:49:59]

https://codereview.chromium.org/935663004/diff/270001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/270001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:228: if (reporter != NULL) {
if (reporter) {

}

[Why: It's the consistent form that works across raw pointers, scoped_ptrs
(which use a boolean conversion trick) and scoped_refptrs (which use a similar
trick, ish). In theory, you could use != nullptr, but that would require a
template specialization for pointer comparisons to recognize std::nullptr_t from
T* / other integral types. Though we use C++11, we don't have full library
support, so we can't quite use std::nullptr_t, so using simply the boolean for
comparison is the universal form for all pointer types]

https://codereview.chromium.org/935663004/diff/270001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/935663004/diff/270001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:392: bool switch_enabled = true) {
Alas, defaults for arguments are not allowed -
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum...

[estark, 2015-02-27 04:47:50]

https://codereview.chromium.org/935663004/diff/270001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/270001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:228: if (reporter != NULL) {
On 2015/02/27 03:49:59, Ryan Sleevi wrote:
> if (reporter) {
> 
> }
> 
> [Why: It's the consistent form that works across raw pointers, scoped_ptrs
> (which use a boolean conversion trick) and scoped_refptrs (which use a similar
> trick, ish). In theory, you could use != nullptr, but that would require a
> template specialization for pointer comparisons to recognize std::nullptr_t
from
> T* / other integral types. Though we use C++11, we don't have full library
> support, so we can't quite use std::nullptr_t, so using simply the boolean for
> comparison is the universal form for all pointer types]

Done, and thanks for the explanation.

https://codereview.chromium.org/935663004/diff/270001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/935663004/diff/270001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:392: bool switch_enabled = true) {
On 2015/02/27 03:49:59, Ryan Sleevi wrote:
> Alas, defaults for arguments are not allowed -
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum...

Curses! Done.

[felt, 2015-02-28 16:44:47]

https://codereview.chromium.org/935663004/diff/290002/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/290002/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:224: const net::SSLInfo& ssl_info) {
Would it make more sense to have a DCHECK for the IO thread constraint, rather
than a comment?

https://codereview.chromium.org/935663004/diff/290002/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:640:
IsPrefEnabled(prefs::kSafeBrowsingExtendedReportingEnabled);
This is missing the Incognito check that's in PopulateExtendedReportingOption.

This also makes me think the malware page is missing an incognito check too. :(

[estark, 2015-02-28 20:59:00]

Thanks Adrienne!

https://codereview.chromium.org/935663004/diff/290002/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/290002/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:224: const net::SSLInfo& ssl_info) {
On 2015/02/28 16:44:47, felt wrote:
> Would it make more sense to have a DCHECK for the IO thread constraint, rather
> than a comment?

Done.

https://codereview.chromium.org/935663004/diff/290002/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:640:
IsPrefEnabled(prefs::kSafeBrowsingExtendedReportingEnabled);
On 2015/02/28 16:44:47, felt wrote:
> This is missing the Incognito check that's in PopulateExtendedReportingOption.
> 
> This also makes me think the malware page is missing an incognito check too.
:(

Ah, good catch -- fixed + added a test. You're right that the malware page has
the same problem; I can fix that in a separate CL.

[estark, 2015-02-28 21:03:12]

https://codereview.chromium.org/935663004/diff/320001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/320001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:643:
UMA_HISTOGRAM_BOOLEAN("SB2.ExtendedReportingIsEnabled", enabled);
I'm not quite sure what this is measuring -- should we be not running this line
when in incognito?

[felt, 2015-03-02 16:06:59]

https://codereview.chromium.org/935663004/diff/290002/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/290002/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:224: const net::SSLInfo& ssl_info) {
On 2015/02/28 16:44:47, felt wrote:
> Would it make more sense to have a DCHECK for the IO thread constraint, rather
> than a comment?

BTW, for reference:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));

https://codereview.chromium.org/935663004/diff/320001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/320001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:643:
UMA_HISTOGRAM_BOOLEAN("SB2.ExtendedReportingIsEnabled", enabled);
On 2015/02/28 21:03:11, emily wrote:
> I'm not quite sure what this is measuring -- should we be not running this
line
> when in incognito?

That's meant to measure how many interstitials had a report done. So you're
right with  your guess, this should be moved inside the if-statement below.

[estark, 2015-03-02 16:26:11]

https://codereview.chromium.org/935663004/diff/320001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/320001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:643:
UMA_HISTOGRAM_BOOLEAN("SB2.ExtendedReportingIsEnabled", enabled);
On 2015/03/02 16:06:58, felt wrote:
> On 2015/02/28 21:03:11, emily wrote:
> > I'm not quite sure what this is measuring -- should we be not running this
> line
> > when in incognito?
> 
> That's meant to measure how many interstitials had a report done. So you're
> right with  your guess, this should be moved inside the if-statement below.

Ok! In that case I just moved the off-the-record check to the top of the
function which I think is a clearer place for it anyway.

[estark, 2015-03-03 02:44:51]

Can this patch be landed with its current functionality, or do we need to wait
until I make it actually do something (i.e. until there's a live server
endpoint)? Landing it would unblock Sascha working on the Finch experiment.

[felt, 2015-03-03 02:49:00]

Yes, this can be landed as-is once the reviews are done. I'll do a full review
pass tonight, but I think it's looking close.

[felt, 2015-03-03 02:53:31]

c/b/interstitials/ and c/b/ssl/* lgtm

the .js and .html changes also look OK to me functionally, but I'll leave it to
an appropriate OWNERS reviewer to comment on js/html style. I'd recommend
bauerb@ for that review, as he's familiar with this code.

[estark, 2015-03-03 02:57:20]

estark@chromium.org changed reviewers:
+ bauerb@chromium.org, mpearson@chromium.org, noelutz@google.com

[estark, 2015-03-03 02:57:22]

Thanks Adrienne!

mpearson@chromium.org: Could you please review histograms.xml?

noelutz@google.com: Could you please review chrome/browser/safe_browsing?

bauerb@chromium.org: Could you please review chrome/browser/resources?

[felt, 2015-03-03 02:58:57]

On 2015/03/03 02:57:22, emily wrote:
> Thanks Adrienne!
> 
> mailto:mpearson@chromium.org: Could you please review histograms.xml?
> 
> mailto:noelutz@google.com: Could you please review
chrome/browser/safe_browsing?
> 
> mailto:bauerb@chromium.org: Could you please review chrome/browser/resources?

You're going to want mattm@ to review c/b/safe_browsing, AFAIK noe doesn't
review Chrome code anymore.

[estark, 2015-03-03 03:00:19]

estark@chromium.org changed reviewers:
+ mattm@chromium.org
- noelutz@google.com

[estark, 2015-03-03 03:00:21]

mattm@: could you please review changes in chrome/browser/safe_browsing?

[Mark P, 2015-03-03 03:22:07]

histograms.xml

[felt, 2015-03-03 03:56:12]

On 2015/03/03 03:22:07, Mark P wrote:
> histograms.xml

Mark did you mean to add a l-g-t-m there?

[Bernhard Bauer, 2015-03-03 09:45:59]

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/net/chro...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/net/chro...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:30: static const
char kInvalidCertificateChainUploadEndpoint[] = "";
Can you add a TODO to add the real endpoint?

Also, use an anonymous namespace instead of static (or if you have only
variables, you don't even need that, as constants automatically get internal
linkage).

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/resource...
File chrome/browser/resources/security_warnings/extended_reporting.js (right):

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/resource...
chrome/browser/resources/security_warnings/extended_reporting.js:1: // Copyright
2014 The Chromium Authors. All rights reserved.
It's 2015! :)

(Alternatively, you can try to convince git to lower the similarity bar when
looking for copies, which will stop me from nitpicking existing code ;-) )

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/resource...
chrome/browser/resources/security_warnings/extended_reporting.js:5: // Must
match the commands handled by SafeBrowsingBlockingPage::CommandReceived.
'use strict'; ?

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/resource...
chrome/browser/resources/security_warnings/extended_reporting.js:36:
interstitialType == 'SAFEBROWSING' ? 'safe-browsing-opt-in' : 'ssl-opt-in');
This should be indented four spaces (which sadly means you'll have to break the
line).

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/resource...
chrome/browser/resources/security_warnings/extended_reporting.js:43:
SB_CMD_DO_REPORT : SSL_CMD_DO_REPORT;
Indent four spaces.

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:611: "optInLink",
If you have constants for the other keys, you might as well add one for this
one.

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/capt...
File chrome/browser/ssl/captive_portal_blocking_page.h (right):

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/capt...
chrome/browser/ssl/captive_portal_blocking_page.h:60: base::DictionaryValue*
load_time_data) override{};
Space before {}, and no semicolon at the end.

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:639:
web_contents()->GetBrowserContext()->IsOffTheRecord())
Add braces around the body if the if-statement is complicated like that.

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.h:46: CMD_DONT_REPORT = 7
Add a comma at the end here -- modern compilers won't complain, and it reduces
the number of changed lines when you add more values.

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:402: MockReporter reporter(NULL);
nullptr

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:411: base::RunLoop().RunUntilIdle();
Urr... this only spins a message loop on the UI thread. How do you know the task
has actually run on the IO thread?

(What you want to do is probably use PostTaskAndReply, where the reply task is
the run loop's QuitClosure).

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:417:
EXPECT_EQ(reporter.GetLatestHostnameReported(), "");
Using an empty std::string() constructor saves a handful of instructions.

[estark, 2015-03-03 15:23:15]

Patchset #21 (id:380001) has been deleted

[estark, 2015-03-03 15:36:24]

Thanks bauerb!

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/net/chro...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/net/chro...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:30: static const
char kInvalidCertificateChainUploadEndpoint[] = "";
On 2015/03/03 09:45:58, Bernhard Bauer wrote:
> Can you add a TODO to add the real endpoint?
> 
> Also, use an anonymous namespace instead of static (or if you have only
> variables, you don't even need that, as constants automatically get internal
> linkage).

Done.

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/resource...
File chrome/browser/resources/security_warnings/extended_reporting.js (right):

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/resource...
chrome/browser/resources/security_warnings/extended_reporting.js:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2015/03/03 09:45:58, Bernhard Bauer wrote:
> It's 2015! :)
> 
> (Alternatively, you can try to convince git to lower the similarity bar when
> looking for copies, which will stop me from nitpicking existing code ;-) )

Done.

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/resource...
chrome/browser/resources/security_warnings/extended_reporting.js:5: // Must
match the commands handled by SafeBrowsingBlockingPage::CommandReceived.
On 2015/03/03 09:45:59, Bernhard Bauer wrote:
> 'use strict'; ?

Done.

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/resource...
chrome/browser/resources/security_warnings/extended_reporting.js:36:
interstitialType == 'SAFEBROWSING' ? 'safe-browsing-opt-in' : 'ssl-opt-in');
On 2015/03/03 09:45:58, Bernhard Bauer wrote:
> This should be indented four spaces (which sadly means you'll have to break
the
> line).

Done.

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/resource...
chrome/browser/resources/security_warnings/extended_reporting.js:43:
SB_CMD_DO_REPORT : SSL_CMD_DO_REPORT;
On 2015/03/03 09:45:59, Bernhard Bauer wrote:
> Indent four spaces.

Done.

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:611: "optInLink",
On 2015/03/03 09:45:59, Bernhard Bauer wrote:
> If you have constants for the other keys, you might as well add one for this
> one.

Done.

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/capt...
File chrome/browser/ssl/captive_portal_blocking_page.h (right):

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/capt...
chrome/browser/ssl/captive_portal_blocking_page.h:60: base::DictionaryValue*
load_time_data) override{};
On 2015/03/03 09:45:59, Bernhard Bauer wrote:
> Space before {}, and no semicolon at the end.

Done.

(Adrienne: you were previously asking me to remove the space and 'git cl format'
kept undoing it -- turns out removing the semicolon was the key to making it
stick.)

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:639:
web_contents()->GetBrowserContext()->IsOffTheRecord())
On 2015/03/03 09:45:59, Bernhard Bauer wrote:
> Add braces around the body if the if-statement is complicated like that.

Done.

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.h:46: CMD_DONT_REPORT = 7
On 2015/03/03 09:45:59, Bernhard Bauer wrote:
> Add a comma at the end here -- modern compilers won't complain, and it reduces
> the number of changed lines when you add more values.

Done.

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:402: MockReporter reporter(NULL);
On 2015/03/03 09:45:59, Bernhard Bauer wrote:
> nullptr

Done.

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:411: base::RunLoop().RunUntilIdle();
On 2015/03/03 09:45:59, Bernhard Bauer wrote:
> Urr... this only spins a message loop on the UI thread. How do you know the
task
> has actually run on the IO thread?
> 
> (What you want to do is probably use PostTaskAndReply, where the reply task is
> the run loop's QuitClosure).

Done, I think -- can you please check that I did the right thing?

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:417:
EXPECT_EQ(reporter.GetLatestHostnameReported(), "");
On 2015/03/03 09:45:59, Bernhard Bauer wrote:
> Using an empty std::string() constructor saves a handful of instructions.

Done.

[estark, 2015-03-03 15:37:54]

By the way, is there something like 'git cl format' that will do
formatting/indentation for JS/HTML files?

[Bernhard Bauer, 2015-03-03 16:17:45]

On 2015/03/03 15:37:54, emily wrote:
> By the way, is there something like 'git cl format' that will do
> formatting/indentation for JS/HTML files?

I think clang-format can also do JS now?

For HTML you'll probably have to use a different pretty-printer, but I think
there should be something online.

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:411: base::RunLoop().RunUntilIdle();
On 2015/03/03 15:36:24, emily wrote:
> On 2015/03/03 09:45:59, Bernhard Bauer wrote:
> > Urr... this only spins a message loop on the UI thread. How do you know the
> task
> > has actually run on the IO thread?
> > 
> > (What you want to do is probably use PostTaskAndReply, where the reply task
is
> > the run loop's QuitClosure).
> 
> Done, I think -- can you please check that I did the right thing?

Almost -- but you don't have to post an additional DoNothing task to the IO
thread; you can directly call PostTaskAndReply with SetUpMockReporter and the
QuitClosure. The RunUntilIdle now also isn't necessary anymore.

https://codereview.chromium.org/935663004/diff/400001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/935663004/diff/400001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:444: WaitForIOThread();
This one is a bit more tricky... The problem with posting an empty task to the
IO thread is that you effectively rely on the fact that the interstitial posts
the send task to the IO thread and sending finishes once that tasks runs, which
is an implementation detail of the interstitial that is buried somewhere deep
down (it took me a couple of minutes to find it).

Given that the interstitial is otherwise completely on the UI thread, it might
make sense to add a callback to the interstitial that will be called on the UI
thread once the report has been sent (by doing the PostTaskAndReply thing in the
interstitial). For the sake of simplicity, I would suggest a method
SetReportSentCallbackForTesting or something.

[Mark P, 2015-03-03 16:45:50]

histograms.xml lgtm

On 2015/03/03 03:56:12, felt wrote:
> On 2015/03/03 03:22:07, Mark P wrote:
> > histograms.xml
> 
> Mark did you mean to add a l-g-t-m there?

Oops, late night error.

--mark

[estark, 2015-03-03 18:06:06]

On 2015/03/03 16:17:45, Bernhard Bauer wrote:
> On 2015/03/03 15:37:54, emily wrote:
> > By the way, is there something like 'git cl format' that will do
> > formatting/indentation for JS/HTML files?
> 
> I think clang-format can also do JS now?
> 
> For HTML you'll probably have to use a different pretty-printer, but I think
> there should be something online.

Ok, cool, thanks.

[estark, 2015-03-03 18:06:20]

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/935663004/diff/360001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:411: base::RunLoop().RunUntilIdle();
On 2015/03/03 16:17:45, Bernhard Bauer wrote:
> On 2015/03/03 15:36:24, emily wrote:
> > On 2015/03/03 09:45:59, Bernhard Bauer wrote:
> > > Urr... this only spins a message loop on the UI thread. How do you know
the
> > task
> > > has actually run on the IO thread?
> > > 
> > > (What you want to do is probably use PostTaskAndReply, where the reply
task
> is
> > > the run loop's QuitClosure).
> > 
> > Done, I think -- can you please check that I did the right thing?
> 
> Almost -- but you don't have to post an additional DoNothing task to the IO
> thread; you can directly call PostTaskAndReply with SetUpMockReporter and the
> QuitClosure. The RunUntilIdle now also isn't necessary anymore.

Done.

https://codereview.chromium.org/935663004/diff/400001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/935663004/diff/400001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:444: WaitForIOThread();
On 2015/03/03 16:17:45, Bernhard Bauer wrote:
> This one is a bit more tricky... The problem with posting an empty task to the
> IO thread is that you effectively rely on the fact that the interstitial posts
> the send task to the IO thread and sending finishes once that tasks runs,
which
> is an implementation detail of the interstitial that is buried somewhere deep
> down (it took me a couple of minutes to find it).
> 
> Given that the interstitial is otherwise completely on the UI thread, it might
> make sense to add a callback to the interstitial that will be called on the UI
> thread once the report has been sent (by doing the PostTaskAndReply thing in
the
> interstitial). For the sake of simplicity, I would suggest a method
> SetReportSentCallbackForTesting or something.

Makes sense! Done. I named it SetCertificateReportCallback since in some cases
it gets called to signal that the report has been not sent rather than sent
(e.g. incognito method). And I put it on SecurityInterstitialPage instead of
SSLBlockingPage, because we anticipate doing certificate reporting on
CaptivePortalBlockingPage in a follow-up CL.

[Ryan Sleevi, 2015-03-03 19:16:39]

This is where coffee + a good nights sleep means I'm an awfully cruel reviewer
who suggests big CL changes late in the game. :(

Sorry for springing this on you after having written so much code.

https://codereview.chromium.org/935663004/diff/420001/net/url_request/fraudul...
File net/url_request/fraudulent_certificate_reporter.h (right):

https://codereview.chromium.org/935663004/diff/420001/net/url_request/fraudul...
net/url_request/fraudulent_certificate_reporter.h:19: // The type of a report:
either a pinning violation on a google.com
This is a bit of a layering violation (or more aptly, documenting the layering
violation)

enum ReportType {
  // The report is an HPKP pinning violation
  REPORT_TYPE_PIN_VIOLATION,

  // The report is being manually sent
  REPORT_TYPE_EXTENDED_REPORTING
};

From a layering perspective, you shouldn't be talking about who/what is being
sent - it seems like all HPKP reports are equal from the API

Having REPORT_TYPE_EXTENDED_REPORTING does feel somewhat weird as well, from an
API layer, since it's really not something //net is using. In this respect, in
looking at it again, it almost feels like SendReport() should NOT have
*ReportType* at all. After all, the comments on line 14-16 indicate this is just
for pinning errors.

A way to accomplish what you want - reuse of code - without doing what gets me
grumpy - layering violations - would be to first (in a separate CL) split the
FraudulentCertificateReporter up.

That is, one class is responsible for building reports, and another class is
responsible for *sending* reports. I suspect the build-report code lives in
//chrome (since the report format is specific to the fraudulent reporting), and
the send report code lives here (as a base interface). Refactor the //chrome
implementation of this interface (without your report type changes) to call the
build report to build the report when SendReport is called, then sending to the
appropriate endpoints.

You could then reuse the BuildReport code (and possibly the SendReport) code
specifically in //chrome, without reaching into this //net interface.

Does that logic make sense?

[Bernhard Bauer, 2015-03-03 21:38:06]

Very nice! Just some opportunities for simplification:

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:83:
base::Callback<void()> certificate_report_callback_for_testing_;
This is typedef'd as base::Closure.

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:640:
certificate_report_callback_for_testing_.Run();
BTW, there is ScopedClosureRunner to make sure the callback runs at the end of
the method. That would also automatically no-op a null callback, so you wouldn't
need to initialize it with base::DoNothing.

[mattm, 2015-03-04 02:58:42]

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.cc (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.cc:100:
pref->SetBoolean(prefs::kSafeBrowsingExtendedReportingEnabled, report);
Should kSafeBrowsingExtendedReportingEnabled be renamed? I guess it could be
confusing to have the variable name say one thing and the actual string value be
different, but it seems better than not.
At the least, the comment in chrome/common/pref_names.cc could be updated.

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.cc:101:
UMA_HISTOGRAM_BOOLEAN("SB2.SetExtendedReportingEnabled", report);
Similar comment as above. Should we make a new histogram and deprecate the old
one?

Or at the least, the description of the histogram in histograms.xml should be
updated.

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:47: void
SetCertificateReportCallbackForTesting(base::Callback<void()>& callback);
Shouldn't this (and the member) be on SSLBlockingPage?

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/net/chro...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/net/chro...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:86: LOG(ERROR) <<
"SSL report for " << hostname << ":\n"
This dumps a serialized protobuf to the log? I think it should be removed, or at
least converted to DVLOG(20).

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:188: //
SecurityInterstitialPageWithExtendedReporting implementation:
I don't see a "SecurityInterstitialPageWithExtendedReporting" anywhere

[felt, 2015-03-04 03:05:08]

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.cc (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.cc:100:
pref->SetBoolean(prefs::kSafeBrowsingExtendedReportingEnabled, report);
On 2015/03/04 02:58:41, mattm wrote:
> Should kSafeBrowsingExtendedReportingEnabled be renamed? I guess it could be
> confusing to have the variable name say one thing and the actual string value
be
> different, but it seems better than not.
> At the least, the comment in chrome/common/pref_names.cc could be updated.

The feature is still referred to as Safe Browsing Extended Reporting, because SB
is providing the backend and will own the analysis pipeline.

[estark, 2015-03-11 22:40:48]

Patchset #23 (id:440001) has been deleted

[estark, 2015-03-11 22:43:06]

Patchset #23 (id:460001) has been deleted

[estark, 2015-03-12 21:18:59]

Patchset #26 (id:540001) has been deleted

[estark, 2015-03-12 21:26:11]

Patchset #25 (id:520001) has been deleted

[estark, 2015-03-12 22:22:21]

Sorry for the delay, everyone. I've been working on the refactoring suggested by
rsleevi earlier in this thread, and I've now landed that patch and rebased this
CL on top of it. (The refactoring is https://codereview.chromium.org/979893003/,
for reference.)

Most of the logic for setting up the checkbox, plumbing through the extended
reporting preference, etc. is unchanged. The main changes are to how
SSLBlockingPage actually sends reports:

* SSLBlockingPage now uses a CertificateErrorReporter (a reporting class that
knows how to send both pinning violation and extended reporting reports) instead
of net::FraudulentCertificateReporter from the URLRequestContext.

* The CertificateErrorReporter has to outlive the SSLBlockingPage, I think,
because we don't want in-flight requests to get cancelled when the
SSLBlockingPage goes away. (I'm operating on the assumption that the
SSLBlockingPage will get destroyed when the user navigates away or proceeds
through it -- is that correct?) For this purpose, I created a
CertificateErrorReporter member variable on ChromeContentBrowserClient that gets
passed down to SSLBlockingPage, and added an argument to
CertificateErrorReporter::SendReport so that SSLBlockingPage can specify the
URLRequestContext at the time that it sends the report.

* This also means that a CertificateErrorReport has to be passed from the UI
thread to the IO thread in SSLBlockingPage, which IIUC means that
CertificateErrorReporter has to be RefCountedThreadSafe.

Please take a look when you can.

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:47: void
SetCertificateReportCallbackForTesting(base::Callback<void()>& callback);
On 2015/03/04 02:58:41, mattm wrote:
> Shouldn't this (and the member) be on SSLBlockingPage?

I put them here because in a follow-up CL I'm going to add a similar feature to
captive portal interstitials, which are a subclass of SecurityInterstitialPage.
(In other words, they're going to end up here eventually.)

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:83:
base::Callback<void()> certificate_report_callback_for_testing_;
On 2015/03/03 21:38:06, Bernhard Bauer wrote:
> This is typedef'd as base::Closure.

Done.

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/net/chro...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/net/chro...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:86: LOG(ERROR) <<
"SSL report for " << hostname << ":\n"
On 2015/03/04 02:58:41, mattm wrote:
> This dumps a serialized protobuf to the log? I think it should be removed, or
at
> least converted to DVLOG(20).

Changed to DVLOG(3).

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:188: //
SecurityInterstitialPageWithExtendedReporting implementation:
On 2015/03/04 02:58:41, mattm wrote:
> I don't see a "SecurityInterstitialPageWithExtendedReporting" anywhere

Fixed to SecurityInterstitialPage.

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:640:
certificate_report_callback_for_testing_.Run();
On 2015/03/03 21:38:06, Bernhard Bauer wrote:
> BTW, there is ScopedClosureRunner to make sure the callback runs at the end of
> the method. That would also automatically no-op a null callback, so you
wouldn't
> need to initialize it with base::DoNothing.

Done. (Though sadly I found that I got a segfault if I didn't still initialize
the callback with base::DoNothing.)

[estark, 2015-03-12 22:25:09]

On 2015/03/04 03:05:08, felt wrote:
>
https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
> File chrome/browser/interstitials/security_interstitial_page.cc (right):
> 
>
https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
> chrome/browser/interstitials/security_interstitial_page.cc:100:
> pref->SetBoolean(prefs::kSafeBrowsingExtendedReportingEnabled, report);
> On 2015/03/04 02:58:41, mattm wrote:
> > Should kSafeBrowsingExtendedReportingEnabled be renamed? I guess it could be
> > confusing to have the variable name say one thing and the actual string
value
> be
> > different, but it seems better than not.
> > At the least, the comment in chrome/common/pref_names.cc could be updated.
> 
> The feature is still referred to as Safe Browsing Extended Reporting, because
SB
> is providing the backend and will own the analysis pipeline.

Do you think we should edit the histograms.xml description? This is what it
currently says:
    Whether the user has Safe Browsing extended reporting enabled at the time a
    Safe Browsing warning was dismissed.  This tracks the fraction of all SB
    interstitials that had reporting enabled.
I'm thinking maybe just changing "Safe Browsing warning" => "Safe Browsing or
SSL warning" and "SB interstitials" => "interstitials" would be clearer. What do
you think?

[Bernhard Bauer, 2015-03-13 13:48:48]

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:640:
certificate_report_callback_for_testing_.Run();
On 2015/03/12 22:22:21, emily wrote:
> On 2015/03/03 21:38:06, Bernhard Bauer wrote:
> > BTW, there is ScopedClosureRunner to make sure the callback runs at the end
of
> > the method. That would also automatically no-op a null callback, so you
> wouldn't
> > need to initialize it with base::DoNothing.
> 
> Done. (Though sadly I found that I got a segfault if I didn't still initialize
> the callback with base::DoNothing.)

Really? ScopedClosureRunner does have a check for is_null() before running its
callback o_O

https://codereview.chromium.org/935663004/diff/600001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/935663004/diff/600001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.h:84: const
scoped_refptr<chrome_browser_net::CertificateErrorReporter>
Should be const-ref.

https://codereview.chromium.org/935663004/diff/600001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.h:94: void
SetCertificateErrorReporterForTesting(scoped_refptr<
Const-ref

https://codereview.chromium.org/935663004/diff/600001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/935663004/diff/600001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:401: base::Callback<void()>
report_callback = report_run_loop.QuitClosure();
This is typedef'd as base::Closure.

https://codereview.chromium.org/935663004/diff/600001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler.h (right):

https://codereview.chromium.org/935663004/diff/600001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.h:58:
scoped_refptr<chrome_browser_net::CertificateErrorReporter>
Pass as const-ref?

[estark, 2015-03-13 15:53:38]

Patchset #28 (id:620001) has been deleted

[estark, 2015-03-13 16:21:17]

Thanks bauerb.

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:640:
certificate_report_callback_for_testing_.Run();
On 2015/03/13 13:48:47, Bernhard Bauer wrote:
> On 2015/03/12 22:22:21, emily wrote:
> > On 2015/03/03 21:38:06, Bernhard Bauer wrote:
> > > BTW, there is ScopedClosureRunner to make sure the callback runs at the
end
> of
> > > the method. That would also automatically no-op a null callback, so you
> > wouldn't
> > > need to initialize it with base::DoNothing.
> > 
> > Done. (Though sadly I found that I got a segfault if I didn't still
initialize
> > the callback with base::DoNothing.)
> 
> Really? ScopedClosureRunner does have a check for is_null() before running its
> callback o_O

Ah, I think the segfault is from when |PostTaskAndReply| calls the released
ScopedClosureRunner (line 673 of
https://codereview.chromium.org/935663004/diff/640001/chrome/browser/ssl/ssl_...).

https://codereview.chromium.org/935663004/diff/600001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/935663004/diff/600001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.h:84: const
scoped_refptr<chrome_browser_net::CertificateErrorReporter>
On 2015/03/13 13:48:47, Bernhard Bauer wrote:
> Should be const-ref.

Done.

https://codereview.chromium.org/935663004/diff/600001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.h:94: void
SetCertificateErrorReporterForTesting(scoped_refptr<
On 2015/03/13 13:48:48, Bernhard Bauer wrote:
> Const-ref

Done.

https://codereview.chromium.org/935663004/diff/600001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/935663004/diff/600001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:401: base::Callback<void()>
report_callback = report_run_loop.QuitClosure();
On 2015/03/13 13:48:48, Bernhard Bauer wrote:
> This is typedef'd as base::Closure.

Done.

https://codereview.chromium.org/935663004/diff/600001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler.h (right):

https://codereview.chromium.org/935663004/diff/600001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.h:58:
scoped_refptr<chrome_browser_net::CertificateErrorReporter>
On 2015/03/13 13:48:48, Bernhard Bauer wrote:
> Pass as const-ref?

Done.

[Bernhard Bauer, 2015-03-13 16:32:41]

LGTM with a suggestion:

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:640:
certificate_report_callback_for_testing_.Run();
On 2015/03/13 16:21:17, emily wrote:
> On 2015/03/13 13:48:47, Bernhard Bauer wrote:
> > On 2015/03/12 22:22:21, emily wrote:
> > > On 2015/03/03 21:38:06, Bernhard Bauer wrote:
> > > > BTW, there is ScopedClosureRunner to make sure the callback runs at the
> end
> > of
> > > > the method. That would also automatically no-op a null callback, so you
> > > wouldn't
> > > > need to initialize it with base::DoNothing.
> > > 
> > > Done. (Though sadly I found that I got a segfault if I didn't still
> initialize
> > > the callback with base::DoNothing.)
> > 
> > Really? ScopedClosureRunner does have a check for is_null() before running
its
> > callback o_O
> 
> Ah, I think the segfault is from when |PostTaskAndReply| calls the released
> ScopedClosureRunner (line 673 of
>
https://codereview.chromium.org/935663004/diff/640001/chrome/browser/ssl/ssl_...).
> 

Aww, that's a pity! What you could do though is replace the null callback with
base::DoNothing immediately before the PostTaskAndReply, so it's at least clear
why we're doing this.

[estark, 2015-03-13 17:00:14]

On 2015/03/13 16:32:41, Bernhard Bauer wrote:
> LGTM with a suggestion:
> 
>
https://codereview.chromium.org/935663004/diff/420001/chrome/browser/ssl/ssl_...
> File chrome/browser/ssl/ssl_blocking_page.cc (right):
> 
>
https://codereview.chromium.org/935663004/diff/420001/chrome/browser/ssl/ssl_...
> chrome/browser/ssl/ssl_blocking_page.cc:640:
> certificate_report_callback_for_testing_.Run();
> On 2015/03/13 16:21:17, emily wrote:
> > On 2015/03/13 13:48:47, Bernhard Bauer wrote:
> > > On 2015/03/12 22:22:21, emily wrote:
> > > > On 2015/03/03 21:38:06, Bernhard Bauer wrote:
> > > > > BTW, there is ScopedClosureRunner to make sure the callback runs at
the
> > end
> > > of
> > > > > the method. That would also automatically no-op a null callback, so
you
> > > > wouldn't
> > > > > need to initialize it with base::DoNothing.
> > > > 
> > > > Done. (Though sadly I found that I got a segfault if I didn't still
> > initialize
> > > > the callback with base::DoNothing.)
> > > 
> > > Really? ScopedClosureRunner does have a check for is_null() before running
> its
> > > callback o_O
> > 
> > Ah, I think the segfault is from when |PostTaskAndReply| calls the released
> > ScopedClosureRunner (line 673 of
> >
>
https://codereview.chromium.org/935663004/diff/640001/chrome/browser/ssl/ssl_...).
> > 
> 
> Aww, that's a pity! What you could do though is replace the null callback with
> base::DoNothing immediately before the PostTaskAndReply, so it's at least
clear
> why we're doing this.

Done. Thanks!

[felt, 2015-03-13 18:52:38]

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.cc (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.cc:101:
UMA_HISTOGRAM_BOOLEAN("SB2.SetExtendedReportingEnabled", report);
On 2015/03/04 02:58:41, mattm wrote:
> Similar comment as above. Should we make a new histogram and deprecate the old
> one?
> 
> Or at the least, the description of the histogram in histograms.xml should be
> updated.

Yes. Now that this is shared code, perhaps we can deprecate this and instead
make it part of SecurityInterstitialInteraction. And record it using
RecordUserInteraction. That way we can see the different % opt-in rates for
different interstitials separately.

[felt, 2015-03-13 19:18:11]

I'm a little uneasy about adding a new member variable to
chrome/browser/chrome_content_browser_client.h. Is there an OWNER for that file
on here who can comment about it? The alternate option being to create a browser
service that owns a reporter and handles reports.

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.cc (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.cc:26: const char
kPrivacyLinkHtml[] =
This needs to be updated, given that https://codereview.chromium.org/981243003/
simplified the commands. Should take a %d now for the command. See
https://codereview.chromium.org/981243003/diff/160001/chrome/browser/safe_bro...

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/net/cert...
File chrome/browser/net/certificate_error_reporter.h (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/net/cert...
chrome/browser/net/certificate_error_reporter.h:62: // Construct, serialize, and
send a certificate reporter to the report
Is this sending a cert "report", or a cert "reporter"?

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/resource...
File chrome/browser/resources/security_warnings/ssl.js (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/resource...
chrome/browser/resources/security_warnings/ssl.js:5: // Should match
SSLBlockingPageCommands in ssl_blocking_page.h.
This file needs to be rebased. As of https://codereview.chromium.org/981243003/,
the commands are now all shared between SSL and SB.

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.h:51: CMD_DONT_REPORT = 7,
You also need to rebase/update this, now that commands are shared

[Ryan Sleevi, 2015-03-14 02:47:16]

On 2015/03/12 22:22:21, emily wrote:
> * This also means that a CertificateErrorReport has to be passed from the UI
> thread to the IO thread in SSLBlockingPage, which IIUC means that
> CertificateErrorReporter has to be RefCountedThreadSafe.

No no no, this is an anti-pattern.

Making things RefCounted is super-bad, in general. At least, it's exceptionally
rare that you ever with to expose this as part of the public API contract.

The other thing that's bad is using an object on multiple threads; we should
prefer to create our objects as single-threaded, live, die, and are accessed on
single threads.

Just from the description (and I haven't dug in to the CL), it sounds like what
you want is a WeakPtr. That is

CertificateErrorReporter is created on the IO thread (at some point in time)
On the IO thread, a WeakPtr is created and then passed to the UI thread
When the UI thread wants to send a report, it PostTasks to the IO thread to do
so. It could either bind to the WeakPtr directly, or pass it as a param; depend
son what you're doing
On the IO thread, if you're using a helper, you'd validate that the WeakPtr was
still valid, grab the raw pointer, and then push that down into the lower layers
if needed.

[Ryan Sleevi, 2015-03-14 03:09:42]

OK, the approach still feels non-idiomatic here / is different than the
surrounding code. I've identified two alternative routes to pursue, both of
which are more consistent with the //content patterns. Matt may be able to guide
you further, or you can ask a //content owner like davidben@ for more guidance
on some of the layering bits.

I think you're 90% there, and most of the "no no no" is just dealing with some
of the Chromium threading nuances and best patterns for dep injection. Matt and
David are for sure more authoritative than I am on the dep management side.

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1855:
SSLErrorHandler::HandleSSLError(tab, cert_error, ssl_info, request_url,
I suspect that in HandleSSLError, it would grab the Profile (ala line 1887) and
then grab the IOdata / appropriate objects.

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:342: certificate_error_reporter_;
+1 to felt that we shouldn't be sticking this here. I suspect it belongs on the
ProfileIOData, but Matt may have Opinions.

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:49: // Useful for
tests to inject a mock reporter.
add newline between 48/49

The comment style between 46/47 and 49 is somewhat inconsistent. It'd be nice to
harmonize.

Do you really need both methods? It seems like either one or the other is
appropriate. That is, setting the reporter implicitly acts as notification.

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:51:
scoped_refptr<chrome_browser_net::CertificateErrorReporter>
always pass scoped_refptr's const and by-ref.

Put differently, the only time you get to use by-value semantics are
scoped_ptr<>. All other class and struct types are passed by const-ref,
including container objects like WeakPtr.

(Note: This is moot w/ my remarks about *not* making this object ref counted)

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:52:
certificate_reporter_for_testing);
Is this method vestigal? Didn't see it in the .cc

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/net/cert...
File chrome/browser/net/certificate_error_reporter.h (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/net/cert...
chrome/browser/net/certificate_error_reporter.h:32: public
base::RefCountedThreadSafe<CertificateErrorReporter> {
Definitely don't want to do this

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:666: if (!enabled ||
!certificate_error_reporter_)
If you go the callback, it'd be something like

if (!enabled || send_report_callback.is_null())

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.h:85: certificate_error_reporter,
Should you pass a reporter?

Should you pass a callback to report? If so, what would the contents be? I
suspect |hostname| and |ssl_info|, but you could in fact pass this as a no-op
closure and expect the invoker of the SSLBlockingPage to bind the hostname and
ssl_info themselves

That is

const base::Closure& send_report_callback,
const base::Callback<void(bool)>& result_callback);

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler.cc (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:94: certificate_error_reporter,
Rather than require this to be passed, you could do
CertificateErrorReporter::FromWebContents(web_contents); in the ctor of the
SSLErrorHandler

Or grab the Profile from the WebContents and grab the CertificateErrorReporter
off the profile's structures.

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:212: certificate_error_reporter_,
callback_))->Show();
namespace {

void SendReport(const base::WeakPtr<CertificateErrorReporter>& reporter, const
net::SSLHostInfo& info, const std::string& hostname) {
   PostTask(BrowserThread::IO,
                  base::Bind(&CertificateErrorReporter::SendReport, reporter,
info, hostname));
}

}

....
base::WeakPtr<CertificateErrorReporter> reporter = ...;  // From Profile? From
WebContents
new SSLBlockingPage(...,
   base::Bind(&SendReport, reporter, ssl_info_, request_url_.host()),
callback_))->Show();

[estark, 2015-03-16 23:20:17]

On 2015/03/13 18:52:38, felt wrote:
>
https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
> File chrome/browser/interstitials/security_interstitial_page.cc (right):
> 
>
https://codereview.chromium.org/935663004/diff/420001/chrome/browser/intersti...
> chrome/browser/interstitials/security_interstitial_page.cc:101:
> UMA_HISTOGRAM_BOOLEAN("SB2.SetExtendedReportingEnabled", report);
> On 2015/03/04 02:58:41, mattm wrote:
> > Similar comment as above. Should we make a new histogram and deprecate the
old
> > one?
> > 
> > Or at the least, the description of the histogram in histograms.xml should
be
> > updated.
> 
> Yes. Now that this is shared code, perhaps we can deprecate this and instead
> make it part of SecurityInterstitialInteraction. And record it using
> RecordUserInteraction. That way we can see the different % opt-in rates for
> different interstitials separately.

felt@, to deprecate the histogram, do I just add an <obsolete> tag in
histograms.xml? Is there anything else I need to do?

[estark, 2015-03-16 23:39:33]

On 2015/03/14 03:09:42, Ryan Sleevi wrote:
> OK, the approach still feels non-idiomatic here / is different than the
> surrounding code. I've identified two alternative routes to pursue, both of
> which are more consistent with the //content patterns. Matt may be able to
guide
> you further, or you can ask a //content owner like davidben@ for more guidance
> on some of the layering bits.

Which Matt? mattm@?

In the latest patch set I've put the CertificateErrorReporter on ProfileIOData
as suggested. Questions for Ryan and/or Matt (whichever Matt that may be):
* For injecting a mock reporter in browser tests, I've put a
|set_certificate_error_reporter_for_testing| method on ProfileIOData. I do see
other set_blah_for_testing methods on ProfileIOData, but I'm not sure if it's
the right way to do it.
* In this patchset I have SSLBlockingPage grabbing the reporter from the
ProfileIOData, but it sounds like you (Ryan) are suggesting that the reporter
should get grabbed a level up in |HandleSSLError| and then passed into the
SSLBlockingPage. Is there a reason to do it in HandleSSLError when
SSLBlockingPage can do it itself?


> 
> I think you're 90% there, and most of the "no no no" is just dealing with some
> of the Chromium threading nuances and best patterns for dep injection. Matt
and
> David are for sure more authoritative than I am on the dep management side.
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/chrome_c...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/chrome_c...
> chrome/browser/chrome_content_browser_client.cc:1855:
> SSLErrorHandler::HandleSSLError(tab, cert_error, ssl_info, request_url,
> I suspect that in HandleSSLError, it would grab the Profile (ala line 1887)
and
> then grab the IOdata / appropriate objects.
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/chrome_c...
> File chrome/browser/chrome_content_browser_client.h (right):
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/chrome_c...
> chrome/browser/chrome_content_browser_client.h:342:
certificate_error_reporter_;
> +1 to felt that we shouldn't be sticking this here. I suspect it belongs on
the
> ProfileIOData, but Matt may have Opinions.
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/intersti...
> File chrome/browser/interstitials/security_interstitial_page.h (right):
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/intersti...
> chrome/browser/interstitials/security_interstitial_page.h:49: // Useful for
> tests to inject a mock reporter.
> add newline between 48/49
> 
> The comment style between 46/47 and 49 is somewhat inconsistent. It'd be nice
to
> harmonize.
> 
> Do you really need both methods? It seems like either one or the other is
> appropriate. That is, setting the reporter implicitly acts as notification.
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/intersti...
> chrome/browser/interstitials/security_interstitial_page.h:51:
> scoped_refptr<chrome_browser_net::CertificateErrorReporter>
> always pass scoped_refptr's const and by-ref.
> 
> Put differently, the only time you get to use by-value semantics are
> scoped_ptr<>. All other class and struct types are passed by const-ref,
> including container objects like WeakPtr.
> 
> (Note: This is moot w/ my remarks about *not* making this object ref counted)
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/intersti...
> chrome/browser/interstitials/security_interstitial_page.h:52:
> certificate_reporter_for_testing);
> Is this method vestigal? Didn't see it in the .cc
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/net/cert...
> File chrome/browser/net/certificate_error_reporter.h (right):
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/net/cert...
> chrome/browser/net/certificate_error_reporter.h:32: public
> base::RefCountedThreadSafe<CertificateErrorReporter> {
> Definitely don't want to do this
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
> File chrome/browser/ssl/ssl_blocking_page.cc (right):
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
> chrome/browser/ssl/ssl_blocking_page.cc:666: if (!enabled ||
> !certificate_error_reporter_)
> If you go the callback, it'd be something like
> 
> if (!enabled || send_report_callback.is_null())
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
> File chrome/browser/ssl/ssl_blocking_page.h (right):
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
> chrome/browser/ssl/ssl_blocking_page.h:85: certificate_error_reporter,
> Should you pass a reporter?
> 
> Should you pass a callback to report? If so, what would the contents be? I
> suspect |hostname| and |ssl_info|, but you could in fact pass this as a no-op
> closure and expect the invoker of the SSLBlockingPage to bind the hostname and
> ssl_info themselves
> 
> That is
> 
> const base::Closure& send_report_callback,
> const base::Callback<void(bool)>& result_callback);
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
> File chrome/browser/ssl/ssl_error_handler.cc (right):
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
> chrome/browser/ssl/ssl_error_handler.cc:94: certificate_error_reporter,
> Rather than require this to be passed, you could do
> CertificateErrorReporter::FromWebContents(web_contents); in the ctor of the
> SSLErrorHandler
> 
> Or grab the Profile from the WebContents and grab the CertificateErrorReporter
> off the profile's structures.
> 
>
https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
> chrome/browser/ssl/ssl_error_handler.cc:212: certificate_error_reporter_,
> callback_))->Show();
> namespace {
> 
> void SendReport(const base::WeakPtr<CertificateErrorReporter>& reporter, const
> net::SSLHostInfo& info, const std::string& hostname) {
>    PostTask(BrowserThread::IO,
>                   base::Bind(&CertificateErrorReporter::SendReport, reporter,
> info, hostname));
> }
> 
> }
> 
> ....
> base::WeakPtr<CertificateErrorReporter> reporter = ...;  // From Profile? From
> WebContents
> new SSLBlockingPage(...,
>    base::Bind(&SendReport, reporter, ssl_info_, request_url_.host()),
> callback_))->Show();

[estark, 2015-03-16 23:40:53]

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.cc (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.cc:26: const char
kPrivacyLinkHtml[] =
On 2015/03/13 19:18:11, felt wrote:
> This needs to be updated, given that
https://codereview.chromium.org/981243003/
> simplified the commands. Should take a %d now for the command. See
>
https://codereview.chromium.org/981243003/diff/160001/chrome/browser/safe_bro...

Done.

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:49: // Useful for
tests to inject a mock reporter.
On 2015/03/14 03:09:42, Ryan Sleevi wrote:
> add newline between 48/49
> 
> The comment style between 46/47 and 49 is somewhat inconsistent. It'd be nice
to
> harmonize.

No longer applicable since the reporter now lives on ProfileIOData.

 
> Do you really need both methods? It seems like either one or the other is
> appropriate. That is, setting the reporter implicitly acts as notification.

I'm not 100% sure what you mean by "setting the reporter implicitly acts as
notification" so this might be a non-sequitur: part of the purpose of the
callback is so that the SSLBlockingPage can notify the test when a report has
*not* been sent (e.g. in incognito mode). Setting the reporter isn't sufficient
to get that type of notification.

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:51:
scoped_refptr<chrome_browser_net::CertificateErrorReporter>
On 2015/03/14 03:09:42, Ryan Sleevi wrote:
> always pass scoped_refptr's const and by-ref.
> 
> Put differently, the only time you get to use by-value semantics are
> scoped_ptr<>. All other class and struct types are passed by const-ref,
> including container objects like WeakPtr.
> 
> (Note: This is moot w/ my remarks about *not* making this object ref counted)

no longer applicable but noted for future

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:52:
certificate_reporter_for_testing);
On 2015/03/14 03:09:42, Ryan Sleevi wrote:
> Is this method vestigal? Didn't see it in the .cc

Done.

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/net/cert...
File chrome/browser/net/certificate_error_reporter.h (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/net/cert...
chrome/browser/net/certificate_error_reporter.h:32: public
base::RefCountedThreadSafe<CertificateErrorReporter> {
On 2015/03/14 03:09:42, Ryan Sleevi wrote:
> Definitely don't want to do this

Done.

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/net/cert...
chrome/browser/net/certificate_error_reporter.h:62: // Construct, serialize, and
send a certificate reporter to the report
On 2015/03/13 19:18:11, felt wrote:
> Is this sending a cert "report", or a cert "reporter"?

Done.

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/resource...
File chrome/browser/resources/security_warnings/ssl.js (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/resource...
chrome/browser/resources/security_warnings/ssl.js:5: // Should match
SSLBlockingPageCommands in ssl_blocking_page.h.
On 2015/03/13 19:18:11, felt wrote:
> This file needs to be rebased. As of
https://codereview.chromium.org/981243003/,
> the commands are now all shared between SSL and SB.

Done.

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:666: if (!enabled ||
!certificate_error_reporter_)
On 2015/03/14 03:09:42, Ryan Sleevi wrote:
> If you go the callback, it'd be something like
> 
> if (!enabled || send_report_callback.is_null())

What I ended up doing was just grabbing the reporter from ProfileIOData here,
but see my previous question about whether it would be better to grab the
reporter in HandleSSLError and why.

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/935663004/diff/660001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.h:51: CMD_DONT_REPORT = 7,
On 2015/03/13 19:18:11, felt wrote:
> You also need to rebase/update this, now that commands are shared

Done.

[mattm, 2015-03-17 20:58:13]

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/net/chro...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/net/chro...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:86: LOG(ERROR) <<
"SSL report for " << hostname << ":\n"
On 2015/03/12 22:22:21, emily wrote:
> On 2015/03/04 02:58:41, mattm wrote:
> > This dumps a serialized protobuf to the log? I think it should be removed,
or
> at
> > least converted to DVLOG(20).
> 
> Changed to DVLOG(3).

I still question the wisdom of logging binary data.  Eg. on linux you can
randomly screw up your terminal state.

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:96:
base::DictionaryValue* load_time_data) = 0;
This doesn't need to be in SecurityInterstitialPage. Each subclasses just calls
its own implementation directly, having a virtual function on the parent isn't
necessary.

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1077: main_request_context_.get(),
If these are going to be handled by safebrowsing servers as felt said, this
should probably be using the safebrowsing_url_request_context...
Actually if that's the case, sending the report could just be a method on
SafeBrowsingPingManager, rather than needing a separate CertificateErrorReporter
class and finding something to own it, etc.

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/resource...
File chrome/browser/resources/security_warnings/extended_reporting.js (right):

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/resource...
chrome/browser/resources/security_warnings/extended_reporting.js:7: // Must
match the commands handled by SafeBrowsingBlockingPage::CommandReceived.
This file also needs to be updated for the command changes.

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:1094: TestBrokenHTTPSReporting(true,
true, true, true, browser());
Use enums with self-descriptive value names instead of a bunch of bool values
which are hard to read and error prone.

[estark, 2015-03-17 21:26:15]

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1077: main_request_context_.get(),
On 2015/03/17 20:58:12, mattm wrote:
> If these are going to be handled by safebrowsing servers as felt said, this
> should probably be using the safebrowsing_url_request_context...
> Actually if that's the case, sending the report could just be a method on
> SafeBrowsingPingManager, rather than needing a separate
CertificateErrorReporter
> class and finding something to own it, etc.

Could SafeBrowsingPingManager be the thing that owns the reporter object? I
think we still need a separate CertificateErrorReporter class, because it is
used to implement ChromeFraudulentCertificateReporter.

[mattm, 2015-03-17 23:57:51]

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1077: main_request_context_.get(),
On 2015/03/17 21:26:15, emily wrote:
> On 2015/03/17 20:58:12, mattm wrote:
> > If these are going to be handled by safebrowsing servers as felt said, this
> > should probably be using the safebrowsing_url_request_context...
> > Actually if that's the case, sending the report could just be a method on
> > SafeBrowsingPingManager, rather than needing a separate
> CertificateErrorReporter
> > class and finding something to own it, etc.
> 
> Could SafeBrowsingPingManager be the thing that owns the reporter object? I
> think we still need a separate CertificateErrorReporter class, because it is
> used to implement ChromeFraudulentCertificateReporter.

I suppose, yeah.

Oh, I forgot to mention, with using the SafeBrowsingRequestContext and talking
to safebrowsing servers, you'd want to have cookies enabled.

[estark, 2015-03-18 15:57:19]

I've changed the CertificateErrorReporter to be owned by SafeBrowsingPingManager
and exposed to the SSLBlockingPage via the SafeBrowsingUIManager, instead of
being owned by ProfileIOData.

mattm@: I added a hook to inject a mock reporter for the browser tests, but I'm
not sure if that's the right way to do these tests. Can you advise?

felt@: I deprecated the existing extended reporting histogram values and
replaced with SecurityInterstitialMetricsHelper::RecordUserInteraction's. Can
you please take a look and see if it looks right?

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/net/chro...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/935663004/diff/420001/chrome/browser/net/chro...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:86: LOG(ERROR) <<
"SSL report for " << hostname << ":\n"
On 2015/03/17 20:58:12, mattm wrote:
> On 2015/03/12 22:22:21, emily wrote:
> > On 2015/03/04 02:58:41, mattm wrote:
> > > This dumps a serialized protobuf to the log? I think it should be removed,
> or
> > at
> > > least converted to DVLOG(20).
> > 
> > Changed to DVLOG(3).
> 
> I still question the wisdom of logging binary data.  Eg. on linux you can
> randomly screw up your terminal state.

Ok, I changed it to just log the hostname. We don't really need to log the full
report anyway; it's just a stub until I implement the actual upload.

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:96:
base::DictionaryValue* load_time_data) = 0;
On 2015/03/17 20:58:12, mattm wrote:
> This doesn't need to be in SecurityInterstitialPage. Each subclasses just
calls
> its own implementation directly, having a virtual function on the parent isn't
> necessary.

Done.

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1077: main_request_context_.get(),
On 2015/03/17 23:57:51, mattm wrote:
> On 2015/03/17 21:26:15, emily wrote:
> > On 2015/03/17 20:58:12, mattm wrote:
> > > If these are going to be handled by safebrowsing servers as felt said,
this
> > > should probably be using the safebrowsing_url_request_context...
> > > Actually if that's the case, sending the report could just be a method on
> > > SafeBrowsingPingManager, rather than needing a separate
> > CertificateErrorReporter
> > > class and finding something to own it, etc.
> > 
> > Could SafeBrowsingPingManager be the thing that owns the reporter object? I
> > think we still need a separate CertificateErrorReporter class, because it is
> > used to implement ChromeFraudulentCertificateReporter.
> 
> I suppose, yeah.

Done.

> 
> Oh, I forgot to mention, with using the SafeBrowsingRequestContext and talking
> to safebrowsing servers, you'd want to have cookies enabled.

Could you point me to an example of how to do that (enable cookies)?

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/resource...
File chrome/browser/resources/security_warnings/extended_reporting.js (right):

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/resource...
chrome/browser/resources/security_warnings/extended_reporting.js:7: // Must
match the commands handled by SafeBrowsingBlockingPage::CommandReceived.
On 2015/03/17 20:58:12, mattm wrote:
> This file also needs to be updated for the command changes.

Done.

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:1094: TestBrokenHTTPSReporting(true,
true, true, true, browser());
On 2015/03/17 20:58:13, mattm wrote:
> Use enums with self-descriptive value names instead of a bunch of bool values
> which are hard to read and error prone.

Done.

https://codereview.chromium.org/935663004/diff/880001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/935663004/diff/880001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:8898: +    The amount of time that
elapsed during extensions::FeatureProvider::Static.
This change and the one below (both unrelated to the rest of this CL) came in
when I ran pretty_print.py. Should I revert them?

[estark, 2015-03-20 00:17:41]

Friendly ping, mattm! How do you feel about `SetBlahForTesting` methods on the
SafeBrowsingPingManager?

On 2015/03/18 15:57:19, emily wrote:
> I've changed the CertificateErrorReporter to be owned by
SafeBrowsingPingManager
> and exposed to the SSLBlockingPage via the SafeBrowsingUIManager, instead of
> being owned by ProfileIOData.
> 
> mattm@: I added a hook to inject a mock reporter for the browser tests, but
I'm
> not sure if that's the right way to do these tests. Can you advise?
> 
> felt@: I deprecated the existing extended reporting histogram values and
> replaced with SecurityInterstitialMetricsHelper::RecordUserInteraction's. Can
> you please take a look and see if it looks right?
> 
>
https://codereview.chromium.org/935663004/diff/420001/chrome/browser/net/chro...
> File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right):
> 
>
https://codereview.chromium.org/935663004/diff/420001/chrome/browser/net/chro...
> chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:86: LOG(ERROR) <<
> "SSL report for " << hostname << ":\n"
> On 2015/03/17 20:58:12, mattm wrote:
> > On 2015/03/12 22:22:21, emily wrote:
> > > On 2015/03/04 02:58:41, mattm wrote:
> > > > This dumps a serialized protobuf to the log? I think it should be
removed,
> > or
> > > at
> > > > least converted to DVLOG(20).
> > > 
> > > Changed to DVLOG(3).
> > 
> > I still question the wisdom of logging binary data.  Eg. on linux you can
> > randomly screw up your terminal state.
> 
> Ok, I changed it to just log the hostname. We don't really need to log the
full
> report anyway; it's just a stub until I implement the actual upload.
> 
>
https://codereview.chromium.org/935663004/diff/760001/chrome/browser/intersti...
> File chrome/browser/interstitials/security_interstitial_page.h (right):
> 
>
https://codereview.chromium.org/935663004/diff/760001/chrome/browser/intersti...
> chrome/browser/interstitials/security_interstitial_page.h:96:
> base::DictionaryValue* load_time_data) = 0;
> On 2015/03/17 20:58:12, mattm wrote:
> > This doesn't need to be in SecurityInterstitialPage. Each subclasses just
> calls
> > its own implementation directly, having a virtual function on the parent
isn't
> > necessary.
> 
> Done.
> 
>
https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
> File chrome/browser/profiles/profile_io_data.cc (right):
> 
>
https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
> chrome/browser/profiles/profile_io_data.cc:1077: main_request_context_.get(),
> On 2015/03/17 23:57:51, mattm wrote:
> > On 2015/03/17 21:26:15, emily wrote:
> > > On 2015/03/17 20:58:12, mattm wrote:
> > > > If these are going to be handled by safebrowsing servers as felt said,
> this
> > > > should probably be using the safebrowsing_url_request_context...
> > > > Actually if that's the case, sending the report could just be a method
on
> > > > SafeBrowsingPingManager, rather than needing a separate
> > > CertificateErrorReporter
> > > > class and finding something to own it, etc.
> > > 
> > > Could SafeBrowsingPingManager be the thing that owns the reporter object?
I
> > > think we still need a separate CertificateErrorReporter class, because it
is
> > > used to implement ChromeFraudulentCertificateReporter.
> > 
> > I suppose, yeah.
> 
> Done.
> 
> > 
> > Oh, I forgot to mention, with using the SafeBrowsingRequestContext and
talking
> > to safebrowsing servers, you'd want to have cookies enabled.
> 
> Could you point me to an example of how to do that (enable cookies)?
> 
>
https://codereview.chromium.org/935663004/diff/760001/chrome/browser/resource...
> File chrome/browser/resources/security_warnings/extended_reporting.js (right):
> 
>
https://codereview.chromium.org/935663004/diff/760001/chrome/browser/resource...
> chrome/browser/resources/security_warnings/extended_reporting.js:7: // Must
> match the commands handled by SafeBrowsingBlockingPage::CommandReceived.
> On 2015/03/17 20:58:12, mattm wrote:
> > This file also needs to be updated for the command changes.
> 
> Done.
> 
>
https://codereview.chromium.org/935663004/diff/760001/chrome/browser/ssl/ssl_...
> File chrome/browser/ssl/ssl_browser_tests.cc (right):
> 
>
https://codereview.chromium.org/935663004/diff/760001/chrome/browser/ssl/ssl_...
> chrome/browser/ssl/ssl_browser_tests.cc:1094: TestBrokenHTTPSReporting(true,
> true, true, true, browser());
> On 2015/03/17 20:58:13, mattm wrote:
> > Use enums with self-descriptive value names instead of a bunch of bool
values
> > which are hard to read and error prone.
> 
> Done.
> 
>
https://codereview.chromium.org/935663004/diff/880001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/935663004/diff/880001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:8898: +    The amount of time that
> elapsed during extensions::FeatureProvider::Static.
> This change and the one below (both unrelated to the rest of this CL) came in
> when I ran pretty_print.py. Should I revert them?

[felt, 2015-03-20 23:35:32]

mattm, any chance you could take a look at this before heading out today?

[mattm, 2015-03-23 05:31:18]

On 2015/03/20 00:17:41, emily wrote:
> Friendly ping, mattm! How do you feel about `SetBlahForTesting` methods on the
> SafeBrowsingPingManager?

Seems okay.

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1077: main_request_context_.get(),
On 2015/03/18 15:57:18, emily wrote:
> On 2015/03/17 23:57:51, mattm wrote:
> > On 2015/03/17 21:26:15, emily wrote:
> > > On 2015/03/17 20:58:12, mattm wrote:
> > > > If these are going to be handled by safebrowsing servers as felt said,
> this
> > > > should probably be using the safebrowsing_url_request_context...
> > > > Actually if that's the case, sending the report could just be a method
on
> > > > SafeBrowsingPingManager, rather than needing a separate
> > > CertificateErrorReporter
> > > > class and finding something to own it, etc.
> > > 
> > > Could SafeBrowsingPingManager be the thing that owns the reporter object?
I
> > > think we still need a separate CertificateErrorReporter class, because it
is
> > > used to implement ChromeFraudulentCertificateReporter.
> > 
> > I suppose, yeah.
> 
> Done.
> 
> > 
> > Oh, I forgot to mention, with using the SafeBrowsingRequestContext and
talking
> > to safebrowsing servers, you'd want to have cookies enabled.
> 
> Could you point me to an example of how to do that (enable cookies)?

Just need to remove the LOAD_DO_NOT_SEND_COOKIES and LOAD_DO_NOT_SAVE_COOKIES
from the load flags.

https://codereview.chromium.org/935663004/diff/880001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/880001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:67: void
SetCertificateReportCallbackForTesting(const base::Closure& callback);
Should be on SSLBlockingPage instead of SecurityInterstitialPage?

https://codereview.chromium.org/935663004/diff/880001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ping_manager.h (right):

https://codereview.chromium.org/935663004/diff/880001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ping_manager.h:63:
chrome_browser_net::CertificateErrorReporter* certificate_error_reporter);
should be passed as a scoped_ptr since it takes ownership

https://codereview.chromium.org/935663004/diff/880001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/935663004/diff/880001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.cc:267:
sb_service_->ping_manager()->ReportInvalidCertificateChain(hostname,
This should have null checks like in SendSerializedMalwareDetails

https://codereview.chromium.org/935663004/diff/880001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ui_manager.h (right):

https://codereview.chromium.org/935663004/diff/880001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.h:131: void
ReportInvalidCertificateChain(const std::string& hostname,
Add comment

[felt, 2015-03-23 14:04:32]

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1077: main_request_context_.get(),
On 2015/03/23 05:31:17, mattm wrote:
> On 2015/03/18 15:57:18, emily wrote:
> > On 2015/03/17 23:57:51, mattm wrote:
> > > On 2015/03/17 21:26:15, emily wrote:
> > > > On 2015/03/17 20:58:12, mattm wrote:
> > > > > If these are going to be handled by safebrowsing servers as felt said,
> > this
> > > > > should probably be using the safebrowsing_url_request_context...
> > > > > Actually if that's the case, sending the report could just be a method
> on
> > > > > SafeBrowsingPingManager, rather than needing a separate
> > > > CertificateErrorReporter
> > > > > class and finding something to own it, etc.
> > > > 
> > > > Could SafeBrowsingPingManager be the thing that owns the reporter
object?
> I
> > > > think we still need a separate CertificateErrorReporter class, because
it
> is
> > > > used to implement ChromeFraudulentCertificateReporter.
> > > 
> > > I suppose, yeah.
> > 
> > Done.
> > 
> > > 
> > > Oh, I forgot to mention, with using the SafeBrowsingRequestContext and
> talking
> > > to safebrowsing servers, you'd want to have cookies enabled.
> > 
> > Could you point me to an example of how to do that (enable cookies)?
> 
> Just need to remove the LOAD_DO_NOT_SEND_COOKIES and LOAD_DO_NOT_SAVE_COOKIES
> from the load flags.

Hold up real quick -- what are the implications of enabling cookies? Are there
privacy implications of this? Why are cookies needed?

[estark, 2015-03-23 16:42:13]

https://codereview.chromium.org/935663004/diff/880001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/880001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:67: void
SetCertificateReportCallbackForTesting(const base::Closure& callback);
On 2015/03/23 05:31:17, mattm wrote:
> Should be on SSLBlockingPage instead of SecurityInterstitialPage?

Done.

https://codereview.chromium.org/935663004/diff/880001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ping_manager.h (right):

https://codereview.chromium.org/935663004/diff/880001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ping_manager.h:63:
chrome_browser_net::CertificateErrorReporter* certificate_error_reporter);
On 2015/03/23 05:31:17, mattm wrote:
> should be passed as a scoped_ptr since it takes ownership

Done.

https://codereview.chromium.org/935663004/diff/880001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/935663004/diff/880001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.cc:267:
sb_service_->ping_manager()->ReportInvalidCertificateChain(hostname,
On 2015/03/23 05:31:17, mattm wrote:
> This should have null checks like in SendSerializedMalwareDetails

Done.

https://codereview.chromium.org/935663004/diff/880001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ui_manager.h (right):

https://codereview.chromium.org/935663004/diff/880001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.h:131: void
ReportInvalidCertificateChain(const std::string& hostname,
On 2015/03/23 05:31:17, mattm wrote:
> Add comment

Done.

[mattm, 2015-03-23 19:17:26]

On 2015/03/23 14:04:32, felt wrote:
>
https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
> File chrome/browser/profiles/profile_io_data.cc (right):
> 
>
https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
> chrome/browser/profiles/profile_io_data.cc:1077: main_request_context_.get(),
> On 2015/03/23 05:31:17, mattm wrote:
> > On 2015/03/18 15:57:18, emily wrote:
> > > On 2015/03/17 23:57:51, mattm wrote:
> > > > On 2015/03/17 21:26:15, emily wrote:
> > > > > On 2015/03/17 20:58:12, mattm wrote:
> > > > > > If these are going to be handled by safebrowsing servers as felt
said,
> > > this
> > > > > > should probably be using the safebrowsing_url_request_context...
> > > > > > Actually if that's the case, sending the report could just be a
method
> > on
> > > > > > SafeBrowsingPingManager, rather than needing a separate
> > > > > CertificateErrorReporter
> > > > > > class and finding something to own it, etc.
> > > > > 
> > > > > Could SafeBrowsingPingManager be the thing that owns the reporter
> object?
> > I
> > > > > think we still need a separate CertificateErrorReporter class, because
> it
> > is
> > > > > used to implement ChromeFraudulentCertificateReporter.
> > > > 
> > > > I suppose, yeah.
> > > 
> > > Done.
> > > 
> > > > 
> > > > Oh, I forgot to mention, with using the SafeBrowsingRequestContext and
> > talking
> > > > to safebrowsing servers, you'd want to have cookies enabled.
> > > 
> > > Could you point me to an example of how to do that (enable cookies)?
> > 
> > Just need to remove the LOAD_DO_NOT_SEND_COOKIES and
LOAD_DO_NOT_SAVE_COOKIES
> > from the load flags.
> 
> Hold up real quick -- what are the implications of enabling cookies? Are there
> privacy implications of this? Why are cookies needed?

See crbug.com/103243 for a bit of background.  It's gone through privacy review,
and is described in the chrome privacy whitepaper too.

[mattm, 2015-03-23 19:22:06]

https://codereview.chromium.org/935663004/diff/940001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ping_manager.cc (right):

https://codereview.chromium.org/935663004/diff/940001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ping_manager.cc:115:
certificate_error_reporter_.reset(certificate_error_reporter.release());
.Pass()

[estark, 2015-03-23 20:55:28]

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/935663004/diff/760001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1077: main_request_context_.get(),
On 2015/03/17 23:57:51, mattm wrote:
> Oh, I forgot to mention, with using the SafeBrowsingRequestContext and talking
> to safebrowsing servers, you'd want to have cookies enabled.

Done.

I think I should add unit tests that CertificateErrorReporter sends cookies on
REPORT_TYPE_EXTENDED_REPORTING reports and not on other reports, but I have to
figure how to do that and I'd like to do it in a follow-up CL, if that's okay.

https://codereview.chromium.org/935663004/diff/940001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ping_manager.cc (right):

https://codereview.chromium.org/935663004/diff/940001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ping_manager.cc:115:
certificate_error_reporter_.reset(certificate_error_reporter.release());
On 2015/03/23 19:22:06, mattm wrote:
> .Pass()

Done, sort of. I get a compiler error if I just replace |.release()| with
|.Pass()| here, but `certificate_error_reporter_ =
certificate_error_reporter.Pass()` seems to work. Is that correct?

[mattm, 2015-03-24 21:56:39]

https://codereview.chromium.org/935663004/diff/940001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ping_manager.cc (right):

https://codereview.chromium.org/935663004/diff/940001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ping_manager.cc:115:
certificate_error_reporter_.reset(certificate_error_reporter.release());
On 2015/03/23 20:55:28, emily wrote:
> On 2015/03/23 19:22:06, mattm wrote:
> > .Pass()
> 
> Done, sort of. I get a compiler error if I just replace |.release()| with
> |.Pass()| here, but `certificate_error_reporter_ =
> certificate_error_reporter.Pass()` seems to work. Is that correct?

Yeah, that's right.

https://codereview.chromium.org/935663004/diff/960001/chrome/browser/net/cert...
File chrome/browser/net/certificate_error_reporter.cc (right):

https://codereview.chromium.org/935663004/diff/960001/chrome/browser/net/cert...
chrome/browser/net/certificate_error_reporter.cc:25: const char
kExtendedReportingUploadUrl[] = "http://example.test";
Should this just go in ping_manager.cc?

https://codereview.chromium.org/935663004/diff/960001/chrome/browser/net/cert...
chrome/browser/net/certificate_error_reporter.cc:99: : DO_NOT_SEND_COOKIES);
Feels like this would make sense to be controlled by the constructor, along with
passing in the URLRequestContext and GURL?

[estark, 2015-03-24 23:06:06]

https://codereview.chromium.org/935663004/diff/960001/chrome/browser/net/cert...
File chrome/browser/net/certificate_error_reporter.cc (right):

https://codereview.chromium.org/935663004/diff/960001/chrome/browser/net/cert...
chrome/browser/net/certificate_error_reporter.cc:25: const char
kExtendedReportingUploadUrl[] = "http://example.test";
On 2015/03/24 21:56:39, mattm wrote:
> Should this just go in ping_manager.cc?

Done.

https://codereview.chromium.org/935663004/diff/960001/chrome/browser/net/cert...
chrome/browser/net/certificate_error_reporter.cc:99: : DO_NOT_SEND_COOKIES);
On 2015/03/24 21:56:39, mattm wrote:
> Feels like this would make sense to be controlled by the constructor, along
with
> passing in the URLRequestContext and GURL?

Done.

[mattm, 2015-03-25 00:19:14]

lgtm with nit

https://codereview.chromium.org/935663004/diff/1020001/chrome/browser/net/cer...
File chrome/browser/net/certificate_error_reporter.h (right):

https://codereview.chromium.org/935663004/diff/1020001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.h:25: extern const char
kExtendedReportingUploadUrl[];
can be removed too.

[estark, 2015-03-25 00:41:23]

Thanks mattm.

rsleevi@, felt@, do either of you want to take another look before I commit?

https://codereview.chromium.org/935663004/diff/1020001/chrome/browser/net/cer...
File chrome/browser/net/certificate_error_reporter.h (right):

https://codereview.chromium.org/935663004/diff/1020001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.h:25: extern const char
kExtendedReportingUploadUrl[];
On 2015/03/25 00:19:14, mattm wrote:
> can be removed too.

Done.

[felt, 2015-03-25 01:07:17]

https://codereview.chromium.org/935663004/diff/1040001/chrome/browser/interst...
File chrome/browser/interstitials/security_interstitial_metrics_helper.h
(right):

https://codereview.chromium.org/935663004/diff/1040001/chrome/browser/interst...
chrome/browser/interstitials/security_interstitial_metrics_helper.h:46:
SET_EXTENDED_REPORTING_ENABLED_SAFE_BROWSING,
You don't need to distinguish between SSL and SAFE_BROWSING here. The metrics
helper records the same enum under different names for each type of warning.

[estark, 2015-03-25 04:08:02]

https://codereview.chromium.org/935663004/diff/1040001/chrome/browser/interst...
File chrome/browser/interstitials/security_interstitial_metrics_helper.h
(right):

https://codereview.chromium.org/935663004/diff/1040001/chrome/browser/interst...
chrome/browser/interstitials/security_interstitial_metrics_helper.h:46:
SET_EXTENDED_REPORTING_ENABLED_SAFE_BROWSING,
On 2015/03/25 01:07:17, felt wrote:
> You don't need to distinguish between SSL and SAFE_BROWSING here. The metrics
> helper records the same enum under different names for each type of warning.

Done. Also, oops, I wasn't actually using SET_EXTENDED_REPORTING_ENABLED
anywhere; fixed that too. I also added SET_EXTENDED_REPORTING_DISABLED so that
we can see when users turn off extended reporting after opting in previously.
Does that sound about right to you?

[felt, 2015-03-25 04:21:56]

yup, lgtm now.

[Ryan Sleevi, 2015-03-25 04:49:46]

For the sake of expediency, I'll go ahead and LGTM

From a design level, I am slightly uncofortable making the SSL bits
(SSLBlockingPage, SSLErrorHandler) depend on the SafeBrowsing code. To me, that
strikes me as a .... (wait for it) layering violation.

That is, it seems weird to have /ssl/ code depend on /safe_browsing/. It seems
like the /ssl/ code perhaps should define some pure interface that is
implemented by higher glue code, which then redirects into the safe-browsing
code. This avoids coupling the two, which makes it easier in the future to do
refactors.

In SOLID terms, this is Dependency Inversion Principle (
http://en.wikipedia.org/wiki/Dependency_inversion_principle ), and the "Figure
1/Figure 2" show how it inverts the dependency.

That said, I'm not 100% down for the rules of that //chrome/browser/ code, other
than if it it was //contents or //components, we'd use that interface
separation. Certainly, something NOT to be addressed in this CL, but worth
mentioning (and hopefully this is concrete enough to show what the alternative
might look like; otherwise, happy to dig in)

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/interst...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/interst...
chrome/browser/interstitials/security_interstitial_page.h:8: #include
"base/callback.h"
Unused?

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/net/cer...
File chrome/browser/net/certificate_error_reporter.cc (right):

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:84: }
You haven't added code to test this

(Edit: I see you plan to do it in a follow-up CL; fair enough, this is a pretty
big CL. Careful though, the boogysleevi comes and haunts all those who promise
him tests and don't deliver either fresh tests or fresh cookies)

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/net/cer...
File chrome/browser/net/certificate_error_reporter.h (right):

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.h:41: enum CookiesPreference {
SEND_COOKIES, DO_NOT_SEND_COOKIES };
Wait, is there ever a situation we want to send cookies?

Normal policy is "No, never for background reports"

(Edit: I See Matt's discussion. That makes me quite sad. Quite quite sad ;( )

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/resourc...
File chrome/browser/resources/security_warnings/extended_reporting.js (right):

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/resourc...
chrome/browser/resources/security_warnings/extended_reporting.js:29:
'ssl-opt-in';
Don't think this seems consistent with
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...
re: ternary?

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/resourc...
chrome/browser/resources/security_warnings/extended_reporting.js:36:
$('opt-in-checkbox').checked ? CMD_DO_REPORT : CMD_DONT_REPORT);
ditto this

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ping_manager.cc (right):

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ping_manager.cc:29: const char
kExtendedReportingUploadUrl[] = "http://example.test";
What will ensure this gets fixed? This seems like a pretty far distance from the
actual code to report. I want to make sure we don't accidentally send reports
here.

An empty/invalid URL + a DCHECK() somewhere seems... safer?

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ping_manager.cc:51:
CertificateErrorReporter::SEND_COOKIES)) {
o_O

(Edit: ;( )

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.h (right):

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.h:31: }
both this and line 27 should have the boilerplate

// namespace base

// namespace net

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.h:35: : public
base::RefCountedThreadSafe<SafeBrowsingUIManager> {
Not your fault, but this raises red flags for me on threading anti-patterns;
it'd be nice to shuffle this up sometime.

[estark, 2015-03-25 05:34:33]

Thanks rsleevi. Filed crbug.com/470415 for the future removal of ssl ->
safe_browsing dependency, and crbug.com/470412 for the cookies tests.

Going to rebase this tomorrow with fresh eyes (it might conflict with
1000333003, not sure) before the CQ.

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/interst...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/interst...
chrome/browser/interstitials/security_interstitial_page.h:8: #include
"base/callback.h"
On 2015/03/25 04:49:45, Ryan Sleevi wrote:
> Unused?

Done.

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/net/cer...
File chrome/browser/net/certificate_error_reporter.cc (right):

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:84: }
On 2015/03/25 04:49:45, Ryan Sleevi wrote:
> You haven't added code to test this
> 
> (Edit: I see you plan to do it in a follow-up CL; fair enough, this is a
pretty
> big CL. Careful though, the boogysleevi comes and haunts all those who promise
> him tests and don't deliver either fresh tests or fresh cookies)

Do I get bonus points for delivering both?

(crbug.com/470412)

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/resourc...
File chrome/browser/resources/security_warnings/extended_reporting.js (right):

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/resourc...
chrome/browser/resources/security_warnings/extended_reporting.js:29:
'ssl-opt-in';
On 2015/03/25 04:49:45, Ryan Sleevi wrote:
> Don't think this seems consistent with
>
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...
> re: ternary?

Done.

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/resourc...
chrome/browser/resources/security_warnings/extended_reporting.js:36:
$('opt-in-checkbox').checked ? CMD_DO_REPORT : CMD_DONT_REPORT);
On 2015/03/25 04:49:45, Ryan Sleevi wrote:
> ditto this

Done.

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ping_manager.cc (right):

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ping_manager.cc:29: const char
kExtendedReportingUploadUrl[] = "http://example.test";
On 2015/03/25 04:49:45, Ryan Sleevi wrote:
> What will ensure this gets fixed? This seems like a pretty far distance from
the
> actual code to report. I want to make sure we don't accidentally send reports
> here.
> 
> An empty/invalid URL + a DCHECK() somewhere seems... safer?

Done. Changed to an empty URL, and moved the DCHECK for an empty url from the
CertificateErrorReporter constructor to |SendReport|. (If I were to leave it in
the constructor, then it would trigger every time regardless of whether the
extended reporting switch or preference is set.)

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.h (right):

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.h:31: }
On 2015/03/25 04:49:46, Ryan Sleevi wrote:
> both this and line 27 should have the boilerplate
> 
> // namespace base
> 
> // namespace net

Done.

https://codereview.chromium.org/935663004/diff/1080001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.h:35: : public
base::RefCountedThreadSafe<SafeBrowsingUIManager> {
On 2015/03/25 04:49:46, Ryan Sleevi wrote:
> Not your fault, but this raises red flags for me on threading anti-patterns;
> it'd be nice to shuffle this up sometime.

Acknowledged.

[estark, 2015-03-25 17:15:32]

A couple changes I've made this morning:

* Move protected |metrics_helper_| data member on SecurityInterstitialPage to
private, with protected |metrics_helper()| and |set_metrics_helper()| methods.

* Fix some SafeBrowsingPingManager unit tests that were failing because they
were passing NULL as the URLRequestContextGetter. To fix this, I allowed the
ping manager's |certificate_error_reporter_| to be null when the
URLRequestContextGetter is null, with a DCHECK that the reporter is non-null if
you actually try to send a report.

When I rebased I pulled in a couple new CaptivePortal browser tests that are
failing; looking into those now.

[estark, 2015-03-25 19:12:34]

The CQ bit was checked by estark@chromium.org

[estark, 2015-03-25 19:12:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org, bauerb@chromium.org, mattm@chromium.org,
felt@chromium.org, rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/935663004/#ps1180001
(title: "revert accidental deletion (fixes failing CaptivePortal tests)")

[commit-bot: I haz the power, 2015-03-25 19:14:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/935663004/1180001

[commit-bot: I haz the power, 2015-03-25 20:39:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-25 20:39:25]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[estark, 2015-03-25 21:54:48]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2015-03-25 21:55:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/935663004/1180001

[commit-bot: I haz the power, 2015-03-25 23:54:13]

Committed patchset #55 (id:1180001)

[commit-bot: I haz the power, 2015-03-25 23:55:23]

Patchset 55 (id:??) landed as
https://crrev.com/93272ab9ec84b4dd90a93fb2aab8ddd94f250d63
Cr-Commit-Position: refs/heads/master@{#322265}