Move error classification into the ssl_errors component

Move error classification into the ssl_errors component

This moves SSLErrorClassification and its unit test into the ssl_errors
component as a set of namespaced util functions.

BUG=488673
TBR=blundell@chromium.org

Committed: https://crrev.com/70127b44581879eee5a417b2c47d294df782ad38
Cr-Commit-Position: refs/heads/master@{#358918}

[felt, 2015-09-23 01:08:41]

Patchset #1 (id:1) has been deleted

[felt, 2015-09-23 01:08:58]

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

[felt, 2015-09-23 01:15:03]

estark, PTAL?

By the way, here are some rambly thoughts to pre-empt a question I'll expect you
to have.

A notable decision in this CL was the relationship between ErrorClassification
and the captive portal-specific code. I had a few options:

The captive portal-specific code could have been moved up into SSLErrorHandler,
but that would have meant changing the lifecycle of SSLErrorHandler. (Right now,
it disappears right after an interstitial is displayed, but we want to get
notifications during the lifetime of the interstitial.) So that was ruled out.

Given the above, we were going to need two separate classes:
ChromeErrorClassification and ErrorClassification. Should it be an is-a
relationship or a has-a relationship? In the near future, the blocking page guts
will become platform-agnostic, and we'll want that to have an
ErrorClassification. So it made sense to me to have a has-a relationship, so
that all security blocking pages will take an ErrorClassification as a
parameter. (The caller can decide whether it should be a
ChromeErrorClassification, a plain ErrorClassification, or some other type of
classification.)

[estark, 2015-09-23 22:42:19]

I did a quick pass over this but while I'm doing a more thorough look I wanted
to respond to your rambly thoughts with my own rambly thoughts.

Another way (I think) this could be done is like so: SSLBlockingPage takes an
ssl_errors::ErrorClassification as a parameter, as you said. ErrorClassification
has an ErrorClassificationDelegate interface, implemented by a class call
CaptivePortalStatsRecorder in chrome/.

The code that constructs the SSLBlockingPage would look like this:

scoped_ptr<ErrorClassification> error_classification(...);
error_classification->set_delegate(new CaptivePortalStatsRecorder());
SSLBlockingPage blocking_page(..., error_classification.Pass(), ...);

and ~SSLBlockingPage() would do something like this:

error_classification_->RecordCleanupStatistics(overridable_); // or something
with a less terrible name. Which would call delegate_->RecordCleanupStatistics()
which is implemented by CaptivePortalStatsRecorder in chrome/ to record captive
portal stats

I don't think this approach is clearly better or worse than what you've done, so
I'm mostly wondering if you considered it and what you think. Here are some very
slight advantages that I see:

- ChromeECDelegate would depend on a pure virtual interface
(ssl_errors::ECDelegate) rather than ChromeEC depending on an implementation
(ssl_errors::EC).
- Seems to maybe follow the sort-of-recommended c14n pattern (see "Dependency
Inversion" on
https://dev.chromium.org/developers/design-documents/browser-components/cookbook)
- It would make the captive portal stats code easier to reuse elsewhere within
chrome/ if we ever would want to do that (seems unlikely though).

https://codereview.chromium.org/1355413003/diff/20001/chrome/browser/ssl/bad_...
File chrome/browser/ssl/bad_clock_blocking_page.cc (right):

https://codereview.chromium.org/1355413003/diff/20001/chrome/browser/ssl/bad_...
chrome/browser/ssl/bad_clock_blocking_page.cc:194: new
ChromeErrorClassification(web_contents, time_triggered_, request_url,
I think this can just be a ErrorClassification (doesn't need to be ChromeEC).

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

https://codereview.chromium.org/1355413003/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:35: #include
"components/ssl_errors/error_classification.h"
Do you need to include both, or would chrome_error_classification.h be enough?
(same question on bad_clock_blocking_page.cc)

https://codereview.chromium.org/1355413003/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:160: new
ChromeErrorClassification(web_contents, time_triggered_, request_url,
If you decide to stick with this design (ChromeEC inheriting from
ssl_errors::EC), maybe keep |ssl_error_classification_| as a
ErrorClassification, and define a virtual
ErrorClassification::RecordCleanupStatistics() method that ChromeEC overrides to
record the captive portal statistics. And call
ssl_error_classification_->RecordCleanupStatistics() in the SSLBlockingPage
destructor instead of calling RecordCaptivePortalStatistics directly. Mostly
suggesting this because I think you'll eventually have to do this anyway when
you componentize SSLBlockingPage.

https://codereview.chromium.org/1355413003/diff/20001/components/ssl_errors/e...
File components/ssl_errors/error_classification.h (right):

https://codereview.chromium.org/1355413003/diff/20001/components/ssl_errors/e...
components/ssl_errors/error_classification.h:64: void
InvalidDateSeverityScore();
While you're here: it doesn't look like this method (or the two below it) are
ever actually used anywhere. Is that true? Can we remove them?

https://codereview.chromium.org/1355413003/diff/20001/components/ssl_errors/e...
components/ssl_errors/error_classification.h:143: // This stores the current
time.
nit on pre-existing code, which must surely be the most annoying kind of nit: I
feel like this comment doesn't add much.

https://codereview.chromium.org/1355413003/diff/20001/components/ssl_errors/e...
File components/ssl_errors/error_classification_unittest.cc (right):

https://codereview.chromium.org/1355413003/diff/20001/components/ssl_errors/e...
components/ssl_errors/error_classification_unittest.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
nit: update copyright year

[estark, 2015-09-23 22:52:35]

Ok, sorry, I'm kinda going down a rabbit hole here, but I had another thought.
If you take out those three ErrorClassification methods that don't look like
they're used anywhere (InvalidDateSeverityScore and friends), and ignore the
static methods, and account for the fact that TimePassedSinceExpiry() could
actually be private (it's only called from within the class itself), the public
interface of ErrorClassification is exactly one method -- RecordUMAStatistics().
I wonder if ErrorClassification doesn't actually need to be a class but instead
could just be a collection of static methods in the ssl_errors namespace.
(RecordUMAStatistics() could just take what it currently gets from data members
as arguments.)

Then we could have a separate CaptivePortalStatsRecorder class in chrome/ that
does its thing. For now SSLBlockingPage could have its own
CaptivePortalStatsRecorder. When you componentize SSLBlockingPage, presumably
you'll have a ChromeSSLBlockingPageDelegate or something like that, which can
own the CaptivePortalStatsRecorder.

</rant>

[felt, 2015-10-02 06:04:17]

PTAL: Completely redid the CL.

On 2015/09/23 22:52:35, estark wrote:
> Ok, sorry, I'm kinda going down a rabbit hole here, but I had another thought.
> If you take out those three ErrorClassification methods that don't look like
> they're used anywhere (InvalidDateSeverityScore and friends), and ignore the
> static methods, and account for the fact that TimePassedSinceExpiry() could
> actually be private (it's only called from within the class itself), the
public
> interface of ErrorClassification is exactly one method --
RecordUMAStatistics().
> I wonder if ErrorClassification doesn't actually need to be a class but
instead
> could just be a collection of static methods in the ssl_errors namespace.
> (RecordUMAStatistics() could just take what it currently gets from data
members
> as arguments.)
> 
> Then we could have a separate CaptivePortalStatsRecorder class in chrome/ that
> does its thing. For now SSLBlockingPage could have its own
> CaptivePortalStatsRecorder. When you componentize SSLBlockingPage, presumably
> you'll have a ChromeSSLBlockingPageDelegate or something like that, which can
> own the CaptivePortalStatsRecorder.
> 
> </rant>

I liked this idea, so I went ahead and did it. The downside is that a number of
helper methods are exposed in the namespace, too, for the sake of testing. The
upside is that I think the header file is actually cleaner and clearer now, and
better suited for how i anticipate this file to grow in the future. But, take a
look yourself -- what do you think?

[estark, 2015-10-02 22:17:22]

I propose that, either in this CL or in a future CL (which I'd be happy to do as
well), we switch the unit tests to test the public functions from this namespace
rather than the helper functions. That'll help us clean up the .h file even more
and will be more rigorous. (For example, right now if we accidentally dropped an
IsHostNameKnownTLD() call from the body of RecordUMAStatistics(), I'm not sure
we have any tests that would catch that.)

Other than that, mostly just nits below.

https://codereview.chromium.org/1355413003/diff/60001/chrome/browser/ssl/bad_...
File chrome/browser/ssl/bad_clock_blocking_page.cc (right):

https://codereview.chromium.org/1355413003/diff/60001/chrome/browser/ssl/bad_...
chrome/browser/ssl/bad_clock_blocking_page.cc:202:
ssl_errors::RecordUMAStatistics(false, time_triggered_, request_url,
nit: is the 'false' overridable? Can you either make this argument an enum or
add /* overridable */?

https://codereview.chromium.org/1355413003/diff/60001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (left):

https://codereview.chromium.org/1355413003/diff/60001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:234: load_time_data->SetString(
What happened to this bit?

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
File components/ssl_errors/error_classification.cc (right):

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:90:
std::vector<std::vector<std::string>> dns_name_tokens;
std::vector<UrlTokens>

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:91: for (size_t i = 0; i <
dns_names.size(); ++i) {
optional nit: could do `for (const auto& dns_name : dns_names)` here

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:92: std::vector<std::string>
dns_name_token_single;
UrlTokens

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:104: size_t
FindSubDomainDifference(const UrlTokens& potential_subdomain,
nit: I think we usually capitalize it as "Subdomain" rather than "Subdomain"

Also even though this is an internal helper function I think it could benefit
from a comment, maybe something like "If |potential_subdomain| is a subdomain of
|parent|, returns the number of labels that are different. Otherwise returns 0."

Though it's also kind of weird that potential_subdomain == parent has the same
return value as when they are completely unrelated. Maybe the signature should
be `bool FindSubdomainDifference(potential_subdomain, parent, size_t*
difference)` and returns false if it's not a subdomain of the parent.

(Sorry, I know this is pre-existing code)

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:120: }
I think this could be written more simply as:

for (size_t i = 0; i < parent.size(); i++) {
  if (parent[i] != potential_subdomain[i + diff_size])
    return 0;
}
return potential_subdomain.size() - parent.size();

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:244: bool
IsWWWSubDomainMatch(const GURL& request_url,
To get this out of the header file, I think you could put this in the anonymous
namespace and test GetWWWSubdomainMatch() in the unit tests instead of this
function. That actually seems like it would better because
GetWWWSubdomainMatch() isn't currently unit-tested and it's the thing that
actually gets used outside this file.

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:294: result = result || true;
optional nit: I think this can be simplified a little bit though it's not all
that much simpler:

for (size_t i = 0; i < potential_parents.size(); ++i) {
  if (potential_parents[i].empty() || potential_parents[i].size >= child.size())
    continue;
  size_t domain_diff = ...;
  if (domain_diff == 1 && child[0] != "www") return true;
}
return false;

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:311: if (domain_diff == 1 &&
potential_children[i][0] != "www")
same optional minor simplification as above

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
File components/ssl_errors/error_classification.h (right):

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.h:18: typedef
std::vector<std::string> UrlTokens;
Not quite sure yet what this will contain but presumably this will be the result
of tokenizing a hostname/DNS name rather than a full URL, so maybe
HostnameTokens or DNSNameTokens?

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.h:76: UrlTokens Tokenize(const
std::string& name);
This doesn't look like it's unit-tested so it shouldn't need to be in the .h

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.h:89: bool
IsHostnameNonUniqueOrDotless(const std::string& hostname);
Suggestion for (possibly future?) cleanup: this function and the two below it
are only called from one place in non-test code, so maybe we should be
unit-testing the public function (RecordUMAStatistics()) instead of these helper
functions. That would allow us to remove them from the .h file.

And we probably should be unit-testing RecordUMAStatistics() anyway. And if we
do so, we can probably move more functions out of the .h, like
IsSubdomainOutsideWildcard(). I guess what I'm saying is that we should be
testing that RecordUMAStatistics() behaves correctly rather than that all of its
miscellaneous helper functions test correctly. But I'd be fine doing that in a
future cleanup CL if you don't want to do it now.

[felt, 2015-10-05 05:26:40]

Before i get any farther: a lot of these comments are really about cleanup of
existing code. I'm happy to do the cleanup but I don't want to do two things
(lots of cleanup + move the class) in the same CL in case something breaks.

Since moving this is blocking other work, I'd prefer to move it over and then
send a follow-up CL. Is that OK?

If not, I'll stop this CL and do the cleanup first and then send a new version
of this CL.

WDYT?

https://codereview.chromium.org/1355413003/diff/60001/chrome/browser/ssl/bad_...
File chrome/browser/ssl/bad_clock_blocking_page.cc (right):

https://codereview.chromium.org/1355413003/diff/60001/chrome/browser/ssl/bad_...
chrome/browser/ssl/bad_clock_blocking_page.cc:202:
ssl_errors::RecordUMAStatistics(false, time_triggered_, request_url,
On 2015/10/02 22:17:21, estark wrote:
> nit: is the 'false' overridable? Can you either make this argument an enum or
> add /* overridable */?

I really hate putting comments inline, but this bool now appears in enough
places that an enum would make sense. But I also don't want to do a refactor for
that in this CL (there are many places where overridable status is passed), so
I'll do this in a followup cleanup pass.

https://codereview.chromium.org/1355413003/diff/60001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (left):

https://codereview.chromium.org/1355413003/diff/60001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:234: load_time_data->SetString(
On 2015/10/02 22:17:21, estark wrote:
> What happened to this bit?

We don't need it anymore since Chrome now provides SHA 256 support for Windows
OSes that don't have it. I killed the MaybeWindowsLacksSHA256Support method so
I'm killing the sole caller too.

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
File components/ssl_errors/error_classification.cc (right):

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:104: size_t
FindSubDomainDifference(const UrlTokens& potential_subdomain,
On 2015/10/02 22:17:21, estark wrote:
> nit: I think we usually capitalize it as "Subdomain" rather than "Subdomain"
> 
> Also even though this is an internal helper function I think it could benefit
> from a comment, maybe something like "If |potential_subdomain| is a subdomain
of
> |parent|, returns the number of labels that are different. Otherwise returns
0."
> 
> Though it's also kind of weird that potential_subdomain == parent has the same
> return value as when they are completely unrelated. Maybe the signature should
> be `bool FindSubdomainDifference(potential_subdomain, parent, size_t*
> difference)` and returns false if it's not a subdomain of the parent.
> 
> (Sorry, I know this is pre-existing code)

I'll change the signature in a followup CL.

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:120: }
On 2015/10/02 22:17:21, estark wrote:
> I think this could be written more simply as:
> 
> for (size_t i = 0; i < parent.size(); i++) {
>   if (parent[i] != potential_subdomain[i + diff_size])
>     return 0;
> }
> return potential_subdomain.size() - parent.size();

Follow-up CL

[estark, 2015-10-05 14:07:47]

On 2015/10/05 05:26:40, felt wrote:
> Before i get any farther: a lot of these comments are really about cleanup of
> existing code. I'm happy to do the cleanup but I don't want to do two things
> (lots of cleanup + move the class) in the same CL in case something breaks.
> 
> Since moving this is blocking other work, I'd prefer to move it over and then
> send a follow-up CL. Is that OK?
> 
> If not, I'll stop this CL and do the cleanup first and then send a new version
> of this CL.
> 
> WDYT?

Yeah I think doing the cleanup in a later CL are this one is fine too. I'm happy
to do some of the cleanup too if you want to split up the work.

[felt, 2015-10-05 18:23:07]

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
File components/ssl_errors/error_classification.cc (right):

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:90:
std::vector<std::vector<std::string>> dns_name_tokens;
On 2015/10/02 22:17:21, estark wrote:
> std::vector<UrlTokens>

Done.

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:91: for (size_t i = 0; i <
dns_names.size(); ++i) {
On 2015/10/02 22:17:21, estark wrote:
> optional nit: could do `for (const auto& dns_name : dns_names)` here

Done.

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:92: std::vector<std::string>
dns_name_token_single;
On 2015/10/02 22:17:21, estark wrote:
> UrlTokens

Done.

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:244: bool
IsWWWSubDomainMatch(const GURL& request_url,
On 2015/10/02 22:17:21, estark wrote:
> To get this out of the header file, I think you could put this in the
anonymous
> namespace and test GetWWWSubdomainMatch() in the unit tests instead of this
> function. That actually seems like it would better because
> GetWWWSubdomainMatch() isn't currently unit-tested and it's the thing that
> actually gets used outside this file.

Follow-up CL

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:294: result = result || true;
On 2015/10/02 22:17:21, estark wrote:
> optional nit: I think this can be simplified a little bit though it's not all
> that much simpler:
> 
> for (size_t i = 0; i < potential_parents.size(); ++i) {
>   if (potential_parents[i].empty() || potential_parents[i].size >=
child.size())
>     continue;
>   size_t domain_diff = ...;
>   if (domain_diff == 1 && child[0] != "www") return true;
> }
> return false;

Follow-up CL

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:311: if (domain_diff == 1 &&
potential_children[i][0] != "www")
On 2015/10/02 22:17:21, estark wrote:
> same optional minor simplification as above

Follow-up CL

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
File components/ssl_errors/error_classification.h (right):

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.h:18: typedef
std::vector<std::string> UrlTokens;
On 2015/10/02 22:17:21, estark wrote:
> Not quite sure yet what this will contain but presumably this will be the
result
> of tokenizing a hostname/DNS name rather than a full URL, so maybe
> HostnameTokens or DNSNameTokens?

It can hold either. I was struggling to come up with a name for it (since before
it was just "Tokens" which is really broad). I've gone with HostnameTokens for
now.

Also, ideally this should be moved out of the header file in a follow-up CL.

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.h:76: UrlTokens Tokenize(const
std::string& name);
On 2015/10/02 22:17:21, estark wrote:
> This doesn't look like it's unit-tested so it shouldn't need to be in the .h

Hmmm, it seems weird to expose UrlTokens but not Tokenize tho.

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.h:89: bool
IsHostnameNonUniqueOrDotless(const std::string& hostname);
On 2015/10/02 22:17:22, estark wrote:
> Suggestion for (possibly future?) cleanup: this function and the two below it
> are only called from one place in non-test code, so maybe we should be
> unit-testing the public function (RecordUMAStatistics()) instead of these
helper
> functions. That would allow us to remove them from the .h file.
> 
> And we probably should be unit-testing RecordUMAStatistics() anyway. And if we
> do so, we can probably move more functions out of the .h, like
> IsSubdomainOutsideWildcard(). I guess what I'm saying is that we should be
> testing that RecordUMAStatistics() behaves correctly rather than that all of
its
> miscellaneous helper functions test correctly. But I'd be fine doing that in a
> future cleanup CL if you don't want to do it now.

Follow-up CL

[felt, 2015-10-05 18:28:24]

On 2015/10/05 14:07:47, estark wrote:
> On 2015/10/05 05:26:40, felt wrote:
> > Before i get any farther: a lot of these comments are really about cleanup
of
> > existing code. I'm happy to do the cleanup but I don't want to do two things
> > (lots of cleanup + move the class) in the same CL in case something breaks.
> > 
> > Since moving this is blocking other work, I'd prefer to move it over and
then
> > send a follow-up CL. Is that OK?
> > 
> > If not, I'll stop this CL and do the cleanup first and then send a new
version
> > of this CL.
> > 
> > WDYT?
> 
> Yeah I think doing the cleanup in a later CL are this one is fine too. I'm
happy
> to do some of the cleanup too if you want to split up the work.

I fixed the really really trivial ones here, but I'm leaving the other things
for follow-up CLs. Take a look at how I've marked each?

This code has been written and then subsequently modified by a series of
interns, so here we are. :)

[estark, 2015-10-06 02:35:41]

lgtm. Left a couple more nits but any of them can be deferred to follow-up CL if
you'd rather.

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
File components/ssl_errors/error_classification.h (right):

https://codereview.chromium.org/1355413003/diff/60001/components/ssl_errors/e...
components/ssl_errors/error_classification.h:76: UrlTokens Tokenize(const
std::string& name);
On 2015/10/05 18:23:07, felt wrote:
> On 2015/10/02 22:17:21, estark wrote:
> > This doesn't look like it's unit-tested so it shouldn't need to be in the .h
> 
> Hmmm, it seems weird to expose UrlTokens but not Tokenize tho.

Ah, yeah, that makes sense. So maybe we can get rid of this once we move the
other helper functions that takes UrlTokens as arguments out of the header.

https://codereview.chromium.org/1355413003/diff/80001/components/ssl_errors/e...
File components/ssl_errors/error_classification.cc (right):

https://codereview.chromium.org/1355413003/diff/80001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:249: return
(GetWWWSubDomainMatch(request_url, dns_names, &www_host));
nit: unnecessary parens

https://codereview.chromium.org/1355413003/diff/80001/components/ssl_errors/e...
File components/ssl_errors/error_classification.h (right):

https://codereview.chromium.org/1355413003/diff/80001/components/ssl_errors/e...
components/ssl_errors/error_classification.h:11: #include
"base/gtest_prod_util.h"
not needed?

https://codereview.chromium.org/1355413003/diff/80001/components/ssl_errors/e...
components/ssl_errors/error_classification.h:12: #include "base/time/time.h"
nit: I think you could forward declare base::Time and move this to the .cc. Same
for the following two #includes.

https://codereview.chromium.org/1355413003/diff/80001/components/ssl_errors/e...
components/ssl_errors/error_classification.h:20: // Methods for identifying
specific error causes. ------------------------------
whoa what are these mysterious dashes

[felt, 2015-10-06 03:15:10]

https://codereview.chromium.org/1355413003/diff/80001/components/ssl_errors/e...
File components/ssl_errors/error_classification.cc (right):

https://codereview.chromium.org/1355413003/diff/80001/components/ssl_errors/e...
components/ssl_errors/error_classification.cc:249: return
(GetWWWSubDomainMatch(request_url, dns_names, &www_host));
On 2015/10/06 02:35:41, estark wrote:
> nit: unnecessary parens

Done.

https://codereview.chromium.org/1355413003/diff/80001/components/ssl_errors/e...
File components/ssl_errors/error_classification.h (right):

https://codereview.chromium.org/1355413003/diff/80001/components/ssl_errors/e...
components/ssl_errors/error_classification.h:11: #include
"base/gtest_prod_util.h"
On 2015/10/06 02:35:41, estark wrote:
> not needed?

Done.

https://codereview.chromium.org/1355413003/diff/80001/components/ssl_errors/e...
components/ssl_errors/error_classification.h:12: #include "base/time/time.h"
On 2015/10/06 02:35:41, estark wrote:
> nit: I think you could forward declare base::Time and move this to the .cc.
Same
> for the following two #includes.

Done.

https://codereview.chromium.org/1355413003/diff/80001/components/ssl_errors/e...
components/ssl_errors/error_classification.h:20: // Methods for identifying
specific error causes. ------------------------------
On 2015/10/06 02:35:41, estark wrote:
> whoa what are these mysterious dashes

     o   o
      )-(
     (O O)
      \=/
     .-"-.
    //\ /\\
  _// / \ \\_
 =./ {,-.} \.=
     || ||
     || ||    
   __|| ||__  
  `---" "---'

Actually they are pretty common in chromium for splitting up header files; but
I'm hoping we won't need them once the helper methods for testing have been
safely tucked away.

[felt, 2015-10-06 03:16:19]

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

[felt, 2015-10-06 03:17:11]

blundell: I'm TBRing you for a minor change to components/ssl_errors.gypi. 

TODO(felt): I need to update the components/OWNERS file to add myself, estark,
and other security folks as reviewers for the ssl_errors and
security_interstitials gypi files.

[felt, 2015-10-06 03:17:16]

The CQ bit was checked by felt@chromium.org

[felt, 2015-10-06 03:17:16]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org
Link to the patchset: https://codereview.chromium.org/1355413003/#ps100001
(title: "Updates for estark")

[commit-bot: I haz the power, 2015-10-06 03:17:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1355413003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1355413003/100001

[commit-bot: I haz the power, 2015-10-06 03:20:52]

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

[commit-bot: I haz the power, 2015-10-06 03:20:52]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[felt, 2015-10-07 21:15:20]

The CQ bit was checked by felt@chromium.org

[felt, 2015-10-07 21:15:21]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org
Link to the patchset: https://codereview.chromium.org/1355413003/#ps120001
(title: "Rebase merge fixes")

[commit-bot: I haz the power, 2015-10-07 21:15:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1355413003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1355413003/120001

[felt, 2015-10-07 21:43:37]

The CQ bit was unchecked by felt@chromium.org

[blundell, 2015-10-08 07:34:09]

lgtm

[blundell, 2015-11-10 12:23:58]

Hi Adrienne,

Is this CL still relevant and good to go?

[felt, 2015-11-10 18:43:37]

The CQ bit was checked by felt@chromium.org

[felt, 2015-11-10 18:43:37]

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

[commit-bot: I haz the power, 2015-11-10 18:44:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1355413003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1355413003/140001

[felt, 2015-11-10 19:31:26]

The CQ bit was unchecked by felt@chromium.org

[felt, 2015-11-10 19:32:46]

The CQ bit was checked by felt@chromium.org

[felt, 2015-11-10 19:32:46]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org,
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1355413003/#ps160001
(title: "remove size_t / int mixing")

[commit-bot: I haz the power, 2015-11-10 19:36:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1355413003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1355413003/160001

[commit-bot: I haz the power, 2015-11-10 21:57:31]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-11-10 21:58:22]

Patchset 8 (id:??) landed as
https://crrev.com/70127b44581879eee5a417b2c47d294df782ad38
Cr-Commit-Position: refs/heads/master@{#358918}

[Ken Rockot(use gerrit already), 2015-11-11 17:56:08]

On 2015/11/10 at 21:58:22, commit-bot wrote:
> Patchset 8 (id:??) landed as
https://crrev.com/70127b44581879eee5a417b2c47d294df782ad38
> Cr-Commit-Position: refs/heads/master@{#358918}

Drive-by comment: the new BUILD.gn incorrectly references
"error_classification_unittests.cc" (plural). Isn't breaking bots because I
guess nothing builds the unit_tests target now, but it does break when building
all targets.

[felt, 2015-11-11 17:57:46]

On 2015/11/11 17:56:08, Ken Rockot wrote:
> On 2015/11/10 at 21:58:22, commit-bot wrote:
> > Patchset 8 (id:??) landed as
> https://crrev.com/70127b44581879eee5a417b2c47d294df782ad38
> > Cr-Commit-Position: refs/heads/master@{#358918}
> 
> Drive-by comment: the new BUILD.gn incorrectly references
> "error_classification_unittests.cc" (plural). Isn't breaking bots because I
> guess nothing builds the unit_tests target now, but it does break when
building
> all targets.

Thanks for letting me know, will mail a fix right away.