Split captive portal metrics out of SSLErrorClassification

Split captive portal metrics out of SSLErrorClassification

SSLErrorClassification can be shared cross-platform if the Chrome-specific
captive portal logic is moved to its own helper. This also is a nice clean
separation of purposes, since the captive portal logic was fairly disjoint
from the other work being done in SSLErrorClassification. Now the captive
portal logic is handled as a metrics helper.

I also made the ownership of the metrics helper clearer by changing to use a
scoped_ptr.

BUG=488673
R=estark@chromium.org
TBR=mattm@chromium.org

Committed: https://crrev.com/5a31c78de2d5a1829910ec09d86ec10ae16e5c4c
Cr-Commit-Position: refs/heads/master@{#351195}

Committed: https://crrev.com/c0a91fac22f1d3ff3f8d052eb5dbffa70ef9835f
Cr-Commit-Position: refs/heads/master@{#351246}

Committed: https://crrev.com/6913b84e23405fc529cf9a902700ac30a9de7403
Cr-Commit-Position: refs/heads/master@{#351358}

[felt, 2015-09-25 18:27:41]

estark, ptal

[estark, 2015-09-25 23:53:35]

lgtm with a few nits

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/intersti...
File chrome/browser/interstitials/chrome_metrics_helper.cc (right):

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/intersti...
chrome/browser/interstitials/chrome_metrics_helper.cc:47: if
(captive_portal_helper_.get())
no `.get()` necessary

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/intersti...
File chrome/browser/interstitials/chrome_metrics_helper.h (right):

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/intersti...
chrome/browser/interstitials/chrome_metrics_helper.h:27: // This class is meant
to be used on the UI thread for captive portal metrics.
nit: at a glance this reads like this class's main purpose is for captive portal
metrics. Maybe "This class must be used on the UI thread because of the captive
portal metrics"?

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

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:176:
scoped_ptr<ChromeMetricsHelper>(
optional nit: you can also use make_scoped_ptr() to save a few chars

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

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/ssl/bad_...
chrome/browser/ssl/bad_clock_blocking_page.cc:188:
chrome_metrics_helper->StartRecordingCaptivePortalMetrics(false);
For the sake of my understanding: is this measuring cases where the user's clock
is wrong and they see a clock interstitial, but they might also be in a captive
portal?

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/ssl/capt...
File chrome/browser/ssl/captive_portal_metrics_helper.h (right):

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/ssl/capt...
chrome/browser/ssl/captive_portal_metrics_helper.h:25: class
CaptivePortalMetricsHelper : public content::NotificationObserver {
nit: What do you think about naming it CaptivePortalMetricsRecorder or something
like that? (Because "helper" is a little vague.)

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_classification.h (right):

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.h:15: class SSLErrorClassification {
Are you planning to leave this as a class, or turn it into a namespace of static
methods? (If the former, maybe leave some comment on it.)

[felt, 2015-09-27 20:08:15]

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/intersti...
File chrome/browser/interstitials/chrome_metrics_helper.cc (right):

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/intersti...
chrome/browser/interstitials/chrome_metrics_helper.cc:47: if
(captive_portal_helper_.get())
On 2015/09/25 23:53:34, estark wrote:
> no `.get()` necessary

Done.

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

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:176:
scoped_ptr<ChromeMetricsHelper>(
On 2015/09/25 23:53:34, estark wrote:
> optional nit: you can also use make_scoped_ptr() to save a few chars

Done.

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

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/ssl/bad_...
chrome/browser/ssl/bad_clock_blocking_page.cc:188:
chrome_metrics_helper->StartRecordingCaptivePortalMetrics(false);
On 2015/09/25 23:53:35, estark wrote:
> For the sake of my understanding: is this measuring cases where the user's
clock
> is wrong and they see a clock interstitial, but they might also be in a
captive
> portal?

Yes. I suppose we don't need to, although we always used to so I figure we ought
to continue for consistency. I'll cc meacer@ and ask him whether he has any
opinion.

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/ssl/capt...
File chrome/browser/ssl/captive_portal_metrics_helper.h (right):

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/ssl/capt...
chrome/browser/ssl/captive_portal_metrics_helper.h:25: class
CaptivePortalMetricsHelper : public content::NotificationObserver {
On 2015/09/25 23:53:35, estark wrote:
> nit: What do you think about naming it CaptivePortalMetricsRecorder or
something
> like that? (Because "helper" is a little vague.)

Sure.

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_classification.h (right):

https://codereview.chromium.org/1365733005/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_classification.h:15: class SSLErrorClassification {
On 2015/09/25 23:53:35, estark wrote:
> Are you planning to leave this as a class, or turn it into a namespace of
static
> methods? (If the former, maybe leave some comment on it.)

I'm going to turn it into a namespace of static methods in my next CL. Also, I
don't think the comment adds anything: it's named SSLErrorClassification and the
comment says it "classifies characteristics of SSL errors," which is the same
words in a different order.

[felt, 2015-09-27 20:08:37]

felt@chromium.org changed reviewers:
+ isherman@chromium.org, mattm@chromium.org

[felt, 2015-09-27 20:11:47]

Can I please have OWNERS reviews for...

mattm: safe_browsing/
isherman: histograms.xml

And meacer@, a question arose for you: we've been recording the captive portal
metrics for bad clock interstitials as well as ssl interstitials. We can now
revisit that decision. Should we stop recording captive portal metrics for bad
clocks (because it's unnecessary), or should we continue (for consistency)?

[Ilya Sherman, 2015-09-28 01:55:57]

histograms.xml lgtm

[meacer, 2015-09-28 17:50:12]

> Should we stop recording captive portal metrics for bad
> clocks (because it's unnecessary), or should we continue (for consistency)?

Unless it's adding extra complexity, it would be good to keep recording for
consistency. I think the number of missed/late pings is still relevant even in
bad clocks.

[felt, 2015-09-28 17:58:27]

On 2015/09/28 17:50:12, Mustafa Emre Acer wrote:
> > Should we stop recording captive portal metrics for bad
> > clocks (because it's unnecessary), or should we continue (for consistency)?
> 
> Unless it's adding extra complexity, it would be good to keep recording for
> consistency. I think the number of missed/late pings is still relevant even in
> bad clocks.

It's only an extra two lines of code, we can keep it if you think it's still
useful.

[felt, 2015-09-28 19:47:04]

Matt, if you don't object, I'm going to mark you as a TBR since the
safe_browsing change is a 2 line purely mechanical edit.

[felt, 2015-09-28 19:47:14]

The CQ bit was checked by felt@chromium.org

[felt, 2015-09-28 19:47:15]

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/1365733005/#ps40001
(title: "Changes for estark")

[commit-bot: I haz the power, 2015-09-28 19:48:30]

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

[mattm, 2015-09-28 22:54:15]

safe_browsing lgtm

[commit-bot: I haz the power, 2015-09-28 23:06:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-28 23:07:30]

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

[dcheng, 2015-09-28 23:52:36]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2015-09-28 23:52:37]

I'm not sure why, but the Android bots are not happy with this patch:
In file included from
../../chrome/browser/ssl/captive_portal_metrics_recorder.cc:5:
../../chrome/browser/ssl/captive_portal_metrics_recorder.h:42:8: error: private
field 'overridable_' is not used [-Werror,-Wunused-private-field]
  bool overridable_;
       ^
../../chrome/browser/ssl/captive_portal_metrics_recorder.h:43:8: error: private
field 'captive_portal_detection_enabled_' is not used
[-Werror,-Wunused-private-field]
  bool captive_portal_detection_enabled_;
       ^
../../chrome/browser/ssl/captive_portal_metrics_recorder.h:45:8: error: private
field 'captive_portal_probe_completed_' is not used
[-Werror,-Wunused-private-field]
  bool captive_portal_probe_completed_;
       ^
../../chrome/browser/ssl/captive_portal_metrics_recorder.h:47:8: error: private
field 'captive_portal_no_response_' is not used [-Werror,-Wunused-private-field]
  bool captive_portal_no_response_;
       ^
../../chrome/browser/ssl/captive_portal_metrics_recorder.h:48:8: error: private
field 'captive_portal_detected_' is not used [-Werror,-Wunused-private-field]
  bool captive_portal_detected_;
       ^
5 errors generated.

[felt, 2015-09-29 00:45:44]

On 2015/09/28 23:52:37, dcheng wrote:
> I'm not sure why, but the Android bots are not happy with this patch:
> In file included from
> ../../chrome/browser/ssl/captive_portal_metrics_recorder.cc:5:
> ../../chrome/browser/ssl/captive_portal_metrics_recorder.h:42:8: error:
private
> field 'overridable_' is not used [-Werror,-Wunused-private-field]
>   bool overridable_;
>        ^
> ../../chrome/browser/ssl/captive_portal_metrics_recorder.h:43:8: error:
private
> field 'captive_portal_detection_enabled_' is not used
> [-Werror,-Wunused-private-field]
>   bool captive_portal_detection_enabled_;
>        ^
> ../../chrome/browser/ssl/captive_portal_metrics_recorder.h:45:8: error:
private
> field 'captive_portal_probe_completed_' is not used
> [-Werror,-Wunused-private-field]
>   bool captive_portal_probe_completed_;
>        ^
> ../../chrome/browser/ssl/captive_portal_metrics_recorder.h:47:8: error:
private
> field 'captive_portal_no_response_' is not used
[-Werror,-Wunused-private-field]
>   bool captive_portal_no_response_;
>        ^
> ../../chrome/browser/ssl/captive_portal_metrics_recorder.h:48:8: error:
private
> field 'captive_portal_detected_' is not used [-Werror,-Wunused-private-field]
>   bool captive_portal_detected_;
>        ^
> 5 errors generated.

There's a variable that's only used inside if def-ed code, which causes the
variable to appear unused to the Android compiler. I'll fix it.

[felt, 2015-09-29 01:00:54]

The CQ bit was checked by felt@chromium.org

[felt, 2015-09-29 01:00:54]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org,
isherman@chromium.org, mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/1365733005/#ps60001
(title: "Compiler error fix")

[commit-bot: I haz the power, 2015-09-29 01:03:16]

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

[commit-bot: I haz the power, 2015-09-29 02:49:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-29 02:50:38]

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

[yosin_UTC9, 2015-09-29 10:48:24]

On 2015/09/29 02:50:38, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/c0a91fac22f1d3ff3f8d052eb5dbffa70ef9835f
> Cr-Commit-Position: refs/heads/master@{#351246}

Sorry reverting... by http://crrev.com/1380513003

android_clang_dbg_recipe bots failed compilation:
   
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...
    
    In file included from
../../chrome/browser/ssl/captive_portal_metrics_recorder.cc:5:
        ../../chrome/browser/ssl/captive_portal_metrics_recorder.h:43:8: error:
private field 'captive_portal_detection_enabled_' is not used
[-Werror,-Wunused-private-field]
          bool captive_portal_detection_enabled_;
               ^
        ../../chrome/browser/ssl/captive_portal_metrics_recorder.h:45:8: error:
private field 'captive_portal_probe_completed_' is not used
[-Werror,-Wunused-private-field]
          bool captive_portal_probe_completed_;
               ^
        ../../chrome/browser/ssl/captive_portal_metrics_recorder.h:47:8: error:
private field 'captive_portal_no_response_' is not used
[-Werror,-Wunused-private-field]
          bool captive_portal_no_response_;
               ^
        ../../chrome/browser/ssl/captive_portal_metrics_recorder.h:48:8: error:
private field 'captive_portal_detected_' is not used
[-Werror,-Wunused-private-field]
          bool captive_portal_detected_;
               ^
        4 errors generated.

[felt, 2015-09-29 17:15:02]

The CQ bit was checked by felt@chromium.org

[felt, 2015-09-29 17:15:02]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org,
isherman@chromium.org, mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/1365733005/#ps80001
(title: "Compiler error fix possibly for realsies")

[commit-bot: I haz the power, 2015-09-29 17:15:34]

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

[commit-bot: I haz the power, 2015-09-29 18:31:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-29 18:32:58]

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

[mdempsky, 2015-09-29 19:54:43]

On 2015/09/29 10:48:24, Yosi_UTC9 wrote:
> android_clang_dbg_recipe bots failed compilation:
>    
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...

Why were these build failures not caught by the CQ?  android_clang_dbg_recipe is
green for every patch set.

[felt, 2015-09-29 19:58:39]

On 2015/09/29 19:54:43, mdempsky wrote:
> On 2015/09/29 10:48:24, Yosi_UTC9 wrote:
> > android_clang_dbg_recipe bots failed compilation:
> >    
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...
> 
> Why were these build failures not caught by the CQ?  android_clang_dbg_recipe
is
> green for every patch set.

Apparently different flags are being passed to the trybots vs the waterfall
bots, to make my life really difficult. See
https://code.google.com/p/chromium/issues/detail?id=529798

[Ted C, 2015-09-29 20:36:53]

On 2015/09/29 19:58:39, felt wrote:
> On 2015/09/29 19:54:43, mdempsky wrote:
> > On 2015/09/29 10:48:24, Yosi_UTC9 wrote:
> > > android_clang_dbg_recipe bots failed compilation:
> > >    
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...
> > 
> > Why were these build failures not caught by the CQ? 
android_clang_dbg_recipe
> is
> > green for every patch set.
> 
> Apparently different flags are being passed to the trybots vs the waterfall
> bots, to make my life really difficult. See
> https://code.google.com/p/chromium/issues/detail?id=529798

Fix in flight:
https://codereview.chromium.org/1377943004/

Scream loudly or forever hold your peace...and it might already be submitted by
the time this is updated.

[mdempsky, 2015-09-29 20:53:56]

On 2015/09/29 19:58:39, felt wrote:
> Apparently different flags are being passed to the trybots vs the waterfall
> bots, to make my life really difficult. See
> https://code.google.com/p/chromium/issues/detail?id=529798

I don't think it's a trybot vs waterfall issue.  The android_clang_dbg_recipe
trybots have been failing for me at https://codereview.chromium.org/1372313004/
seemingly because of this CL.  :(

[felt, 2015-09-29 20:56:16]

On 2015/09/29 20:53:56, mdempsky wrote:
> On 2015/09/29 19:58:39, felt wrote:
> > Apparently different flags are being passed to the trybots vs the waterfall
> > bots, to make my life really difficult. See
> > https://code.google.com/p/chromium/issues/detail?id=529798
> 
> I don't think it's a trybot vs waterfall issue.  The android_clang_dbg_recipe
> trybots have been failing for me at
https://codereview.chromium.org/1372313004/
> seemingly because of this CL.  :(

I if-def'ed out the offending code, how is it still causing failures??