Remove certificate reporting code from SafeBrowsing ping and ui managers

Remove certificate reporting code from SafeBrowsing ping and ui managers

This CL removes remaining certificate reporting code from SafeBrowsingPingManager
and SafeBrowsingUIManager.

It also removes MockErrorReporter class which was passed to SafeBrowsingPingManager
in tests, and uses MockSSLCertReporter directly to keep track of sent reports. This
removes some coverage of reporting code which is now inside CertificateReportingService.
However, CertificateReportingService is extensively tested in its own tests, so there
shouldn't be any downside to the removal of MockErrorReporter in terms of code coverage.

A better implementation of Captive portal and SSL UI browser tests should probably
go through the whole reporting code without using any mock reporter, but that requires
rewriting large portions of those browser tests and is best done in a separate CL.

BUG=554323
Committed: https://crrev.com/f73eb0ecdbbfe0ff6388cdc4bff5fa8672a2583c
Cr-Commit-Position: refs/heads/master@{#441009}

[meacer, 2016-12-16 00:33:18]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 00:34:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 06:03:18]

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

[commit-bot: I haz the power, 2016-12-16 06:03:19]

Dry run: This issue passed the CQ dry run.

[meacer, 2016-12-16 20:34:50]

Description was changed from

==========
Remove certificate reporting code from SafeBrowsing ping and ui managers

BUG=554323
==========

to

==========
Remove certificate reporting code from SafeBrowsing ping and ui managers

This CL removes remaining certificate reporting code from
SafeBrowsingPingManager
and SafeBrowsingUIManager.

It also removes MockErrorReporter class which was passed to
SafeBrowsingPingManager
in tests, and uses MockSSLCertReporter directly to keep track of sent reports.
This
removes some coverage of reporting code which is now inside
CertificateReportingService.
However, CertificateReportingService is extensively tested in its other places,
so there
shouldn't be any downside to this.

A better implementation of Captive portal and SSL UI browser tests should
probably
go through the whole reporting code without using any mock reporter, but that
requires
rewriting large portions of those browser tests and is best done in a separate
CL.

BUG=554323
==========

[meacer, 2016-12-16 20:37:01]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 20:37:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[meacer, 2016-12-16 22:18:52]

meacer@chromium.org changed reviewers:
+ estark@chromium.org, jialiul@chromium.org

[meacer, 2016-12-16 22:18:52]

jialiul, estark: Hopefully the final CL. Thanks!

[meacer, 2016-12-16 22:19:49]

Description was changed from

==========
Remove certificate reporting code from SafeBrowsing ping and ui managers

This CL removes remaining certificate reporting code from
SafeBrowsingPingManager
and SafeBrowsingUIManager.

It also removes MockErrorReporter class which was passed to
SafeBrowsingPingManager
in tests, and uses MockSSLCertReporter directly to keep track of sent reports.
This
removes some coverage of reporting code which is now inside
CertificateReportingService.
However, CertificateReportingService is extensively tested in its other places,
so there
shouldn't be any downside to this.

A better implementation of Captive portal and SSL UI browser tests should
probably
go through the whole reporting code without using any mock reporter, but that
requires
rewriting large portions of those browser tests and is best done in a separate
CL.

BUG=554323
==========

to

==========
Remove certificate reporting code from SafeBrowsing ping and ui managers

This CL removes remaining certificate reporting code from
SafeBrowsingPingManager
and SafeBrowsingUIManager.

It also removes MockErrorReporter class which was passed to
SafeBrowsingPingManager
in tests, and uses MockSSLCertReporter directly to keep track of sent reports.
This
removes some coverage of reporting code which is now inside
CertificateReportingService.
However, CertificateReportingService is extensively tested in its own tests, so
there
shouldn't be any downside to the removal of MockErrorReporter in terms of code
coverage.

A better implementation of Captive portal and SSL UI browser tests should
probably
go through the whole reporting code without using any mock reporter, but that
requires
rewriting large portions of those browser tests and is best done in a separate
CL.

BUG=554323
==========

[Jialiu Lin, 2016-12-16 22:25:20]

lgtm for c/b/safe_browsing :-)

[meacer, 2016-12-22 00:51:15]

Rebased.

[estark, 2016-12-29 02:32:26]

lgtm

https://codereview.chromium.org/2578873003/diff/60001/chrome/browser/ssl/cert...
File chrome/browser/ssl/certificate_reporting_test_utils.h (right):

https://codereview.chromium.org/2578873003/diff/60001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_reporting_test_utils.h:28: // It outlives the
SSLCertReporter when an interstitial owning that
I feel like this sentence is missing a phrase or word or something? Is this
class destroyed when the interstitial is dismissed?

[meacer, 2016-12-29 03:08:15]

https://codereview.chromium.org/2578873003/diff/60001/chrome/browser/ssl/cert...
File chrome/browser/ssl/certificate_reporting_test_utils.h (right):

https://codereview.chromium.org/2578873003/diff/60001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_reporting_test_utils.h:28: // It outlives the
SSLCertReporter when an interstitial owning that
On 2016/12/29 02:32:26, estark wrote:
> I feel like this sentence is missing a phrase or word or something? Is this
> class destroyed when the interstitial is dismissed?

Umm, nope. SSLCertReporter is destroyed when the interstitial is dismissed.
Since it's destroyed once the interstitial is dismissed, we can't check whether
it sent the report after the interstitial is dismissed.

So instead, SSLCertReporter calls ReportSent on this callback when it sends a
report. The callback outlives both SSLCertReporter and the interstitial. 

I don't know if I made it any clearer :/

[estark, 2016-12-29 16:00:49]

https://codereview.chromium.org/2578873003/diff/60001/chrome/browser/ssl/cert...
File chrome/browser/ssl/certificate_reporting_test_utils.h (right):

https://codereview.chromium.org/2578873003/diff/60001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_reporting_test_utils.h:28: // It outlives the
SSLCertReporter when an interstitial owning that
On 2016/12/29 03:08:15, Mustafa Emre Acer wrote:
> On 2016/12/29 02:32:26, estark wrote:
> > I feel like this sentence is missing a phrase or word or something? Is this
> > class destroyed when the interstitial is dismissed?
> 
> Umm, nope. SSLCertReporter is destroyed when the interstitial is dismissed.
> Since it's destroyed once the interstitial is dismissed, we can't check
whether
> it sent the report after the interstitial is dismissed.
> 
> So instead, SSLCertReporter calls ReportSent on this callback when it sends a
> report. The callback outlives both SSLCertReporter and the interstitial. 
> 
> I don't know if I made it any clearer :/

Ah, I see, that makes sense. Thanks! Still lgtm

[meacer, 2016-12-29 23:07:56]

https://codereview.chromium.org/2578873003/diff/60001/chrome/browser/ssl/cert...
File chrome/browser/ssl/certificate_reporting_test_utils.h (right):

https://codereview.chromium.org/2578873003/diff/60001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_reporting_test_utils.h:28: // It outlives the
SSLCertReporter when an interstitial owning that
On 2016/12/29 16:00:49, estark wrote:
> On 2016/12/29 03:08:15, Mustafa Emre Acer wrote:
> > On 2016/12/29 02:32:26, estark wrote:
> > > I feel like this sentence is missing a phrase or word or something? Is
this
> > > class destroyed when the interstitial is dismissed?
> > 
> > Umm, nope. SSLCertReporter is destroyed when the interstitial is dismissed.
> > Since it's destroyed once the interstitial is dismissed, we can't check
> whether
> > it sent the report after the interstitial is dismissed.
> > 
> > So instead, SSLCertReporter calls ReportSent on this callback when it sends
a
> > report. The callback outlives both SSLCertReporter and the interstitial. 
> > 
> > I don't know if I made it any clearer :/
> 
> Ah, I see, that makes sense. Thanks! Still lgtm

Thanks, I extended the comment a bit.

[meacer, 2016-12-29 23:08:00]

The CQ bit was checked by meacer@chromium.org

[meacer, 2016-12-29 23:08:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
jialiul@chromium.org, estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2578873003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2016-12-29 23:08:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-30 00:04:37]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483052880121080,
"parent_rev": "fedfe06569c2204a6f5523c1a9777fcfbc799bca", "commit_rev":
"65cef667ae83d03da7e6f280939d94a5a80cecb9"}

[commit-bot: I haz the power, 2016-12-30 00:05:04]

Description was changed from

==========
Remove certificate reporting code from SafeBrowsing ping and ui managers

This CL removes remaining certificate reporting code from
SafeBrowsingPingManager
and SafeBrowsingUIManager.

It also removes MockErrorReporter class which was passed to
SafeBrowsingPingManager
in tests, and uses MockSSLCertReporter directly to keep track of sent reports.
This
removes some coverage of reporting code which is now inside
CertificateReportingService.
However, CertificateReportingService is extensively tested in its own tests, so
there
shouldn't be any downside to the removal of MockErrorReporter in terms of code
coverage.

A better implementation of Captive portal and SSL UI browser tests should
probably
go through the whole reporting code without using any mock reporter, but that
requires
rewriting large portions of those browser tests and is best done in a separate
CL.

BUG=554323
==========

to

==========
Remove certificate reporting code from SafeBrowsing ping and ui managers

This CL removes remaining certificate reporting code from
SafeBrowsingPingManager
and SafeBrowsingUIManager.

It also removes MockErrorReporter class which was passed to
SafeBrowsingPingManager
in tests, and uses MockSSLCertReporter directly to keep track of sent reports.
This
removes some coverage of reporting code which is now inside
CertificateReportingService.
However, CertificateReportingService is extensively tested in its own tests, so
there
shouldn't be any downside to the removal of MockErrorReporter in terms of code
coverage.

A better implementation of Captive portal and SSL UI browser tests should
probably
go through the whole reporting code without using any mock reporter, but that
requires
rewriting large portions of those browser tests and is best done in a separate
CL.

BUG=554323

Review-Url: https://codereview.chromium.org/2578873003
==========

[commit-bot: I haz the power, 2016-12-30 00:05:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2017-01-02 15:53:55]

Description was changed from

==========
Remove certificate reporting code from SafeBrowsing ping and ui managers

This CL removes remaining certificate reporting code from
SafeBrowsingPingManager
and SafeBrowsingUIManager.

It also removes MockErrorReporter class which was passed to
SafeBrowsingPingManager
in tests, and uses MockSSLCertReporter directly to keep track of sent reports.
This
removes some coverage of reporting code which is now inside
CertificateReportingService.
However, CertificateReportingService is extensively tested in its own tests, so
there
shouldn't be any downside to the removal of MockErrorReporter in terms of code
coverage.

A better implementation of Captive portal and SSL UI browser tests should
probably
go through the whole reporting code without using any mock reporter, but that
requires
rewriting large portions of those browser tests and is best done in a separate
CL.

BUG=554323

Review-Url: https://codereview.chromium.org/2578873003
==========

to

==========
Remove certificate reporting code from SafeBrowsing ping and ui managers

This CL removes remaining certificate reporting code from
SafeBrowsingPingManager
and SafeBrowsingUIManager.

It also removes MockErrorReporter class which was passed to
SafeBrowsingPingManager
in tests, and uses MockSSLCertReporter directly to keep track of sent reports.
This
removes some coverage of reporting code which is now inside
CertificateReportingService.
However, CertificateReportingService is extensively tested in its own tests, so
there
shouldn't be any downside to the removal of MockErrorReporter in terms of code
coverage.

A better implementation of Captive portal and SSL UI browser tests should
probably
go through the whole reporting code without using any mock reporter, but that
requires
rewriting large portions of those browser tests and is best done in a separate
CL.

BUG=554323

Committed: https://crrev.com/f73eb0ecdbbfe0ff6388cdc4bff5fa8672a2583c
Cr-Commit-Position: refs/heads/master@{#441009}
==========

[commit-bot: I haz the power, 2017-01-02 15:53:56]

Patchset 5 (id:??) landed as
https://crrev.com/f73eb0ecdbbfe0ff6388cdc4bff5fa8672a2583c
Cr-Commit-Position: refs/heads/master@{#441009}