Simplify security_interstitials::ControllerClient and other related classes

Simplify security_interstitials::ControllerClient and other related classes.

- Remove set_metrics_helper from ControllerClient and passes the MetricHelper in the constructor.
- Use the metric helper from the ControllerClient instead of doing it through SecurityInterstitialPage methods.
- Remove obsolete methods from SecurityInterstitialPage.
- Convert a bunch of methods to static, group some code into separate functions.

BUG=488673
Committed: https://crrev.com/d19deef47a83440e0b5803283a4075716ca1b9eb
Cr-Commit-Position: refs/heads/master@{#421718}

[meacer, 2016-09-02 18:21:44]

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

[commit-bot: I haz the power, 2016-09-02 18:22:22]

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-09-02 18:33:32]

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

[commit-bot: I haz the power, 2016-09-02 18:33:34]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[meacer, 2016-09-02 18:55:35]

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

[commit-bot: I haz the power, 2016-09-02 18:56:12]

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

[meacer, 2016-09-02 19:12:53]

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

[commit-bot: I haz the power, 2016-09-02 19:13:19]

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

[meacer, 2016-09-02 19:50:28]

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

[commit-bot: I haz the power, 2016-09-02 19:51:13]

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-09-02 20:42:40]

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

[commit-bot: I haz the power, 2016-09-02 20:42:41]

Dry run: This issue passed the CQ dry run.

[meacer, 2016-09-02 23:12:05]

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

[commit-bot: I haz the power, 2016-09-02 23:12:33]

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

[meacer, 2016-09-02 23:24:57]

Description was changed from

==========
Simplify SecurityInterstitialPage

BUG=
==========

to

==========
Simplify security_interstitials::ControllerClient and other related classes.

- Remove set_metrics_helper from ControllerClient and passes the MetricHelper in
the constructor.
- Use the metric helper from the ControllerClient instead of doing it through
SecurityInterstitialPage methods, remove those methods.
- Remove code in AfterShow() in classes derived from SecurityInterstitialPage.
SecurityInterstitialPage already does it.
- Convert a bunch of methods to static, group some code into separate functions.

 
BUG=488673
==========

[meacer, 2016-09-02 23:27:09]

Description was changed from

==========
Simplify security_interstitials::ControllerClient and other related classes.

- Remove set_metrics_helper from ControllerClient and passes the MetricHelper in
the constructor.
- Use the metric helper from the ControllerClient instead of doing it through
SecurityInterstitialPage methods, remove those methods.
- Remove code in AfterShow() in classes derived from SecurityInterstitialPage.
SecurityInterstitialPage already does it.
- Convert a bunch of methods to static, group some code into separate functions.

 
BUG=488673
==========

to

==========
Simplify security_interstitials::ControllerClient and other related classes.

- Remove set_metrics_helper from ControllerClient and passes the MetricHelper in
the constructor.
- Use the metric helper from the ControllerClient instead of doing it through
SecurityInterstitialPage methods.
- Remove obsolete methods from SecurityInterstitialPage.
- Convert a bunch of methods to static, group some code into separate functions.

 
BUG=488673
==========

[meacer, 2016-09-02 23:27:23]

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

[meacer, 2016-09-02 23:27:48]

felt: PTAL? This is a follow up to https://codereview.chromium.org/2298613007/

[meacer, 2016-09-02 23:27:48]

felt: PTAL? This is a follow up to https://codereview.chromium.org/2298613007/

[felt, 2016-09-03 15:49:29]

"Patch Set 2 : Bravely attempt fixing iOS without a Mac" 👍 😂

[felt, 2016-09-06 20:33:19]

https://codereview.chromium.org/2303413002/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2303413002/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:129:
SafeBrowsingBlockingPage::GetInterstitialReason(unsafe_resources);
Doesn't this mean that now GetInterstitialReason is called twice in the same
constructor, in two different places?

https://codereview.chromium.org/2303413002/diff/160001/chrome/browser/ssl/cap...
File chrome/browser/ssl/captive_portal_blocking_page.cc (right):

https://codereview.chromium.org/2303413002/diff/160001/chrome/browser/ssl/cap...
chrome/browser/ssl/captive_portal_blocking_page.cc:60: :
SecurityInterstitialPage(web_contents, request_url, nullptr),
did you intend to add a metrics helper here (since you added the include above)?

[meacer, 2016-09-06 20:40:16]

https://codereview.chromium.org/2303413002/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2303413002/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:129:
SafeBrowsingBlockingPage::GetInterstitialReason(unsafe_resources);
On 2016/09/06 20:33:19, felt wrote:
> Doesn't this mean that now GetInterstitialReason is called twice in the same
> constructor, in two different places?

Yes, unfortunately that's true since we can't compute these values before
calling the base class constructor.

Well, we actually can, by adding a static factory method, like I added for
SSLBlockingPage::Create. But the reason I did it there and not here is that
SSLBlockingPage requires the return value of IsOverridable function, and that
function does a preference lookup which I don't want to do twice. Whereas here,
these static calls are fast so it doesn't hurt to do them twice.

https://codereview.chromium.org/2303413002/diff/160001/chrome/browser/ssl/cap...
File chrome/browser/ssl/captive_portal_blocking_page.cc (right):

https://codereview.chromium.org/2303413002/diff/160001/chrome/browser/ssl/cap...
chrome/browser/ssl/captive_portal_blocking_page.cc:60: :
SecurityInterstitialPage(web_contents, request_url, nullptr),
On 2016/09/06 20:33:19, felt wrote:
> did you intend to add a metrics helper here (since you added the include
above)?

I'm doing that in a follow up CL: https://codereview.chromium.org/2302623002/

[felt, 2016-09-07 00:06:12]

lgtm % nit

https://codereview.chromium.org/2303413002/diff/160001/chrome/browser/ssl/cap...
File chrome/browser/ssl/captive_portal_blocking_page.cc (right):

https://codereview.chromium.org/2303413002/diff/160001/chrome/browser/ssl/cap...
chrome/browser/ssl/captive_portal_blocking_page.cc:60: :
SecurityInterstitialPage(web_contents, request_url, nullptr),
On 2016/09/06 20:40:16, Mustafa Emre Acer wrote:
> On 2016/09/06 20:33:19, felt wrote:
> > did you intend to add a metrics helper here (since you added the include
> above)?
> 
> I'm doing that in a follow up CL: https://codereview.chromium.org/2302623002/

can you remove the unnecessary include then from this CL? thx

[meacer, 2016-09-12 20:40:52]

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

[commit-bot: I haz the power, 2016-09-12 20:41:23]

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-09-12 21:36:16]

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

[commit-bot: I haz the power, 2016-09-12 21:36:17]

Dry run: This issue passed the CQ dry run.

[meacer, 2016-09-27 20:52:39]

On 2016/09/07 00:06:12, felt wrote:
> lgtm % nit
> 
>
https://codereview.chromium.org/2303413002/diff/160001/chrome/browser/ssl/cap...
> File chrome/browser/ssl/captive_portal_blocking_page.cc (right):
> 
>
https://codereview.chromium.org/2303413002/diff/160001/chrome/browser/ssl/cap...
> chrome/browser/ssl/captive_portal_blocking_page.cc:60: :
> SecurityInterstitialPage(web_contents, request_url, nullptr),
> On 2016/09/06 20:40:16, Mustafa Emre Acer wrote:
> > On 2016/09/06 20:33:19, felt wrote:
> > > did you intend to add a metrics helper here (since you added the include
> > above)?
> > 
> > I'm doing that in a follow up CL:
https://codereview.chromium.org/2302623002/
> 
> can you remove the unnecessary include then from this CL? thx

Unfortunately, no. Build fails with incomplete type error for MetricsHelper
without this header.

[meacer, 2016-09-27 20:56:58]

meacer@chromium.org changed reviewers:
+ dbeam@chromium.org, nparker@chromium.org, rohitrao@chromium.org

[meacer, 2016-09-27 20:56:58]

nparker for chrome/browser/safe_browsing/
rohitrao for ios/chrome/
dbeam for chrome/browser/ui/webui/interstitials/

Can you PTAL? Thanks.

[rohitrao (ping after 24h), 2016-09-27 21:40:37]

ios lgtm

[Dan Beam, 2016-09-27 22:36:32]

lgtm

[Nathan Parker, 2016-09-29 00:17:51]

nparker@chromium.org changed reviewers:
+ jialiul@chromium.org

[Nathan Parker, 2016-09-29 00:17:52]

-->jialiul for review

[Jialiu Lin, 2016-09-29 00:34:01]

lgtm for chrome/browser/safe_browsing/

https://codereview.chromium.org/2303413002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2303413002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:158: const
SafeBrowsingBlockingPage::UnsafeResourceList& unsafe_resources) {
nit: you probably don't need "SafeBrowsingBlockingPage::" in front of
UnsafeResourceList

https://codereview.chromium.org/2303413002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:162: for
(SafeBrowsingBlockingPage::UnsafeResourceList::const_iterator iter =
same here and other places

[meacer, 2016-09-29 01:26:24]

https://codereview.chromium.org/2303413002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2303413002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:158: const
SafeBrowsingBlockingPage::UnsafeResourceList& unsafe_resources) {
On 2016/09/29 00:34:01, Jialiu Lin wrote:
> nit: you probably don't need "SafeBrowsingBlockingPage::" in front of
> UnsafeResourceList

Done, and also moved these below so that they follow the order in the header.

https://codereview.chromium.org/2303413002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:162: for
(SafeBrowsingBlockingPage::UnsafeResourceList::const_iterator iter =
On 2016/09/29 00:34:01, Jialiu Lin wrote:
> same here and other places

Done.

[meacer, 2016-09-29 01:26:31]

The CQ bit was checked by meacer@chromium.org

[meacer, 2016-09-29 01:26:32]

The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org,
dbeam@chromium.org, jialiul@chromium.org, rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2303413002/#ps220001
(title: "Jialiu comments")

[commit-bot: I haz the power, 2016-09-29 01:27:01]

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-09-29 02:08:36]

Description was changed from

==========
Simplify security_interstitials::ControllerClient and other related classes.

- Remove set_metrics_helper from ControllerClient and passes the MetricHelper in
the constructor.
- Use the metric helper from the ControllerClient instead of doing it through
SecurityInterstitialPage methods.
- Remove obsolete methods from SecurityInterstitialPage.
- Convert a bunch of methods to static, group some code into separate functions.

 
BUG=488673
==========

to

==========
Simplify security_interstitials::ControllerClient and other related classes.

- Remove set_metrics_helper from ControllerClient and passes the MetricHelper in
the constructor.
- Use the metric helper from the ControllerClient instead of doing it through
SecurityInterstitialPage methods.
- Remove obsolete methods from SecurityInterstitialPage.
- Convert a bunch of methods to static, group some code into separate functions.

 
BUG=488673
==========

[commit-bot: I haz the power, 2016-09-29 02:08:37]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-09-29 02:10:38]

Description was changed from

==========
Simplify security_interstitials::ControllerClient and other related classes.

- Remove set_metrics_helper from ControllerClient and passes the MetricHelper in
the constructor.
- Use the metric helper from the ControllerClient instead of doing it through
SecurityInterstitialPage methods.
- Remove obsolete methods from SecurityInterstitialPage.
- Convert a bunch of methods to static, group some code into separate functions.

 
BUG=488673
==========

to

==========
Simplify security_interstitials::ControllerClient and other related classes.

- Remove set_metrics_helper from ControllerClient and passes the MetricHelper in
the constructor.
- Use the metric helper from the ControllerClient instead of doing it through
SecurityInterstitialPage methods.
- Remove obsolete methods from SecurityInterstitialPage.
- Convert a bunch of methods to static, group some code into separate functions.

BUG=488673

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

[commit-bot: I haz the power, 2016-09-29 02:10:41]

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