Refactor CertificateReportSender

Refactor CertificateReportSender

CertificateReportSender renamed to ReportSender
TransportSecurityState::ReportSender renamed to TransportSecurityState::ReportSenderInterface

BUG=613883
Committed: https://crrev.com/bd5be520553a3886c5a44a4a02d53ce0fd322e64
Cr-Commit-Position: refs/heads/master@{#399081}

[stefanocs, 2016-06-01 03:42:31]

stefanocs@google.com changed reviewers:
+ kcarattini@chromium.org

[kcarattini, 2016-06-01 03:48:26]

kcarattini@chromium.org changed reviewers:
+ estark@chromium.org, nparker@chromium.org

[kcarattini, 2016-06-01 03:48:27]

+Nathan and Emily for comment on the name change.

We are planning to reuse the CertificateReportSender for sending Permission
Action reports to Safe Browsing, and so looking for a more generic name.

[estark, 2016-06-01 04:26:04]

Hi, sorry I didn't get a chance to look at your doc yet!

I don't think the net stack should have to know about Safe Browsing, so it's a
bit weird to have SB in the name. In addition,
the-class-formerly-known-as-CertificateReportSender is involved with completely
non-Safe-Browsing activities such as Public Key Pinning reports. Maybe it should
just be called ReportSender...? Feels kind of vague, but, after all, it is a
rather vague class that just throws reports at a server.

[kcarattini, 2016-06-01 04:29:18]

I think the problem was there is a conflict with that name and the class that
this class inherits from.

[estark, 2016-06-01 04:38:39]

On 2016/06/01 04:29:18, kcarattini wrote:
> I think the problem was there is a conflict with that name and the class that
> this class inherits from.

Oh, right. Hmm. Perhaps TransportSecurityState::ReportSender could be renamed as
well, then. TransportSecurityState::ReportSender ->
TransportSecurityState::HPKPReporter, and CertificateReportSender ->
ReportSender?

(The TransportSecurityState::ReportSender is the interface used to send HPKP
reports, and it basically exists so that TransportSecurityState tests can stub
it out for testing.)

[kcarattini, 2016-06-01 05:04:09]

On 2016/06/01 04:38:39, estark wrote:
> On 2016/06/01 04:29:18, kcarattini wrote:
> > I think the problem was there is a conflict with that name and the class
that
> > this class inherits from.
> 
> Oh, right. Hmm. Perhaps TransportSecurityState::ReportSender could be renamed
as
> well, then. TransportSecurityState::ReportSender ->
> TransportSecurityState::HPKPReporter, and CertificateReportSender ->
> ReportSender?
> 
> (The TransportSecurityState::ReportSender is the interface used to send HPKP
> reports, and it basically exists so that TransportSecurityState tests can stub
> it out for testing.)

There doesn't seem to be anything HPKP specific in the ReportSender interface so
it might be good to keep a generic name. That's actually why we want to reuse
the CertificateReportSender for permission reports, because as far as I can tell
it's a generic way to send a report to a URI (Safe Browsing CSD in our case). If
there's anything specific to certificates in there then we probably shouldn't
reuse the class (please flag if I've missed something).

What about  TransportSecurityState::ReportSender stays the same and
CertificateReportSender -> ReportSenderImpl? I don't see any other non-test
implementations of it.

[estark, 2016-06-01 05:39:09]

On 2016/06/01 05:04:09, kcarattini wrote:
> On 2016/06/01 04:38:39, estark wrote:
> > On 2016/06/01 04:29:18, kcarattini wrote:
> > > I think the problem was there is a conflict with that name and the class
> that
> > > this class inherits from.
> > 
> > Oh, right. Hmm. Perhaps TransportSecurityState::ReportSender could be
renamed
> as
> > well, then. TransportSecurityState::ReportSender ->
> > TransportSecurityState::HPKPReporter, and CertificateReportSender ->
> > ReportSender?
> > 
> > (The TransportSecurityState::ReportSender is the interface used to send HPKP
> > reports, and it basically exists so that TransportSecurityState tests can
stub
> > it out for testing.)
> 
> There doesn't seem to be anything HPKP specific in the ReportSender interface
so
> it might be good to keep a generic name. That's actually why we want to reuse
> the CertificateReportSender for permission reports, because as far as I can
tell
> it's a generic way to send a report to a URI (Safe Browsing CSD in our case).
If
> there's anything specific to certificates in there then we probably shouldn't
> reuse the class (please flag if I've missed something).
> 
> What about  TransportSecurityState::ReportSender stays the same and
> CertificateReportSender -> ReportSenderImpl? I don't see any other non-test
> implementations of it.

I feel like I don't quite follow this reasoning and might be missing something.
Are you planning to depend on TransportSecurityState::ReportSender in your
permission reporting code? I was thinking that you were planning to depend on
the renamed CertificateReportSender (in which case it would seem a little weird
to me to depend on an Impl class, though I suppose it's not unheard-of), and
your permission reporting code wouldn't know anything about TSS:ReportSender.

[kcarattini, 2016-06-01 05:44:19]

On 2016/06/01 05:39:09, estark wrote:
> On 2016/06/01 05:04:09, kcarattini wrote:
> > On 2016/06/01 04:38:39, estark wrote:
> > > On 2016/06/01 04:29:18, kcarattini wrote:
> > > > I think the problem was there is a conflict with that name and the class
> > that
> > > > this class inherits from.
> > > 
> > > Oh, right. Hmm. Perhaps TransportSecurityState::ReportSender could be
> renamed
> > as
> > > well, then. TransportSecurityState::ReportSender ->
> > > TransportSecurityState::HPKPReporter, and CertificateReportSender ->
> > > ReportSender?
> > > 
> > > (The TransportSecurityState::ReportSender is the interface used to send
HPKP
> > > reports, and it basically exists so that TransportSecurityState tests can
> stub
> > > it out for testing.)
> > 
> > There doesn't seem to be anything HPKP specific in the ReportSender
interface
> so
> > it might be good to keep a generic name. That's actually why we want to
reuse
> > the CertificateReportSender for permission reports, because as far as I can
> tell
> > it's a generic way to send a report to a URI (Safe Browsing CSD in our
case).
> If
> > there's anything specific to certificates in there then we probably
shouldn't
> > reuse the class (please flag if I've missed something).
> > 
> > What about  TransportSecurityState::ReportSender stays the same and
> > CertificateReportSender -> ReportSenderImpl? I don't see any other non-test
> > implementations of it.
> 
> I feel like I don't quite follow this reasoning and might be missing
something.
> Are you planning to depend on TransportSecurityState::ReportSender in your
> permission reporting code? I was thinking that you were planning to depend on
> the renamed CertificateReportSender (in which case it would seem a little
weird
> to me to depend on an Impl class, though I suppose it's not unheard-of), and
> your permission reporting code wouldn't know anything about TSS:ReportSender.

Yes, that's correct (see the doc I added you to earlier). The permission code
would own an instance of the CertificateReportSender (or whatever we rename it
to).

[stefanocs, 2016-06-02 01:05:33]

Description was changed from

==========
Refactor CertificateReportSender

Class name renamed to SafeBrowsingReportSender
File name renamed to safe_browsing_report_sender

BUG=613883
==========

to

==========
Refactor CertificateReportSender

CertificateReportSender renamed to ReportSender
TransportSecurityState::ReportSender renamed to
TransportSecurityState::ReportSenderInterface

BUG=613883
==========

[stefanocs, 2016-06-02 01:23:12]

CertificateReportSender renamed to ReportSender

TransportSecurityState::ReportSender renamed to
TransportSecurityState::ReportSenderInterface

[kcarattini, 2016-06-02 03:26:41]

lgtm

[stefanocs, 2016-06-02 03:37:26]

stefanocs@google.com changed reviewers:
+ mattm@chromium.org, sdefresne@chromium.org, sky@chromium.org

[stefanocs, 2016-06-02 03:37:27]

sdefresne@chromium.org: Please review changes in ios directory

sky@chromium.org: Please review changes in chrome/browser/profiles

mattm@chromium.org: Please review changes in net

[sky, 2016-06-02 17:49:16]

sky@chromium.org changed reviewers:
+ erg@chromium.org

[sky, 2016-06-02 17:49:44]

sky@chromium.org changed reviewers:
- sky@chromium.org

[sky, 2016-06-02 17:49:45]

sky->erg (for chrome/browser/profiles)

[Elliot Glaysher, 2016-06-02 17:51:12]

profile class rename lgtm

[mattm, 2016-06-02 21:50:08]

lgtm

Regarding the discussion about ReportSender and
TransportSecurityState::ReportSenderInterface:
It feels like ReportSender isn't really tied to TransportSecurityState, it just
implements that interface for expedience (eg, I wouldn't say ReportSender "Is A"
TransportSecurityState::ReportSender).

Maybe the organization would be clearer if ReportSender didn't implement that
interface, but there was a separate class (say,
TransportSecurityStateReportSenderImpl) that did, and held a ReportSender as a
member that it used to actually implement the
TransportSecurityState::ReportSenderInterface methods?

Anyawy, just some food for thought, this CL is okay.

[stefanocs, 2016-06-09 00:46:57]

ping

sdefresne: Please review changes in ios/
estark: Please review changes in chrome/browser/ssl/ and
components/certificate_reporting/

Thanks!

[estark, 2016-06-09 03:02:12]

chrome/browser/ssl and components/certificate_reporting lgtm

[sdefresne, 2016-06-09 07:52:34]

ios/ lgtm

[stefanocs, 2016-06-09 07:55:18]

The CQ bit was checked by stefanocs@google.com

[commit-bot: I haz the power, 2016-06-09 07:55:32]

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

[commit-bot: I haz the power, 2016-06-09 09:03:33]

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

[commit-bot: I haz the power, 2016-06-09 09:03:34]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[stefanocs, 2016-06-09 23:33:09]

The CQ bit was checked by stefanocs@google.com

[stefanocs, 2016-06-09 23:33:09]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org,
erg@chromium.org, sdefresne@chromium.org, estark@chromium.org,
kcarattini@chromium.org
Link to the patchset: https://codereview.chromium.org/2026213002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-06-09 23:33:31]

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

[commit-bot: I haz the power, 2016-06-10 01:35:35]

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

[commit-bot: I haz the power, 2016-06-10 01:35:36]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[stefanocs, 2016-06-10 01:45:03]

The CQ bit was checked by stefanocs@google.com

[commit-bot: I haz the power, 2016-06-10 01:45:34]

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

[commit-bot: I haz the power, 2016-06-10 03:38:08]

Description was changed from

==========
Refactor CertificateReportSender

CertificateReportSender renamed to ReportSender
TransportSecurityState::ReportSender renamed to
TransportSecurityState::ReportSenderInterface

BUG=613883
==========

to

==========
Refactor CertificateReportSender

CertificateReportSender renamed to ReportSender
TransportSecurityState::ReportSender renamed to
TransportSecurityState::ReportSenderInterface

BUG=613883
==========

[commit-bot: I haz the power, 2016-06-10 03:38:10]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-10 03:38:25]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-10 03:41:37]

Description was changed from

==========
Refactor CertificateReportSender

CertificateReportSender renamed to ReportSender
TransportSecurityState::ReportSender renamed to
TransportSecurityState::ReportSenderInterface

BUG=613883
==========

to

==========
Refactor CertificateReportSender

CertificateReportSender renamed to ReportSender
TransportSecurityState::ReportSender renamed to
TransportSecurityState::ReportSenderInterface

BUG=613883

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

[commit-bot: I haz the power, 2016-06-10 03:41:39]

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