Componentizing SafeBrowsingBlockingPage Part 1

This is one of several CLs to componentize SafeBrowsingBlockingPage.
(1) Create SafeBrowsingErrorUI in security_interstitials component. Move
display and command handling logic from SafeBrowsingBlockingPage to
SafeBrowsingErrorUI.

(2) Move all SB interstitial related strings from
c/a/generated_reources.grd to
components/security_interstitials_strings.grdp

(3) Deprecate all interstitial related rappor metrics, since nobody is
using them.

BUG=666100
Committed: https://crrev.com/68ea739f83af7661f140802bd8941a577f02eb30
Cr-Commit-Position: refs/heads/master@{#439163}

[Jialiu Lin, 2016-12-13 22:28:49]

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

[commit-bot: I haz the power, 2016-12-13 22:29:22]

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

[Jialiu Lin, 2016-12-13 22:30:12]

Description was changed from

==========
Create SafeBrowsingErrorUI in security_interstitials component. Move
display and command handling logic from SafeBrowsingBlockingPage to
SafeBrowsingErrorUI.

This is one of several CLs to componentize SafeBrowsingBlockingPage.
BUG=666100
==========

to

==========
Create SafeBrowsingErrorUI in security_interstitials component. Move
display and command handling logic from SafeBrowsingBlockingPage to
SafeBrowsingErrorUI.

Also move all SB interstitial related strings from 
c/a/generated_reources.grd to 
components/security_interstitials_strings.grdp

This is one of several CLs to componentize SafeBrowsingBlockingPage.
BUG=666100
==========

[commit-bot: I haz the power, 2016-12-13 22:33:13]

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

[commit-bot: I haz the power, 2016-12-13 22:33:14]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Jialiu Lin, 2016-12-14 02:07:29]

Description was changed from

==========
Create SafeBrowsingErrorUI in security_interstitials component. Move
display and command handling logic from SafeBrowsingBlockingPage to
SafeBrowsingErrorUI.

Also move all SB interstitial related strings from 
c/a/generated_reources.grd to 
components/security_interstitials_strings.grdp

This is one of several CLs to componentize SafeBrowsingBlockingPage.
BUG=666100
==========

to

==========
Create SafeBrowsingErrorUI in security_interstitials component. Move
display and command handling logic from SafeBrowsingBlockingPage to
SafeBrowsingErrorUI.

Also move all SB interstitial related strings from 
c/a/generated_reources.grd to 
components/security_interstitials_strings.grdp

Deprecate all interstitial related rappor metrics, since nobody is
using them. 

This is one of several CLs to componentize SafeBrowsingBlockingPage.
BUG=666100
==========

[Jialiu Lin, 2016-12-14 02:24:14]

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

[commit-bot: I haz the power, 2016-12-14 02:24:51]

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-14 02:31:15]

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

[commit-bot: I haz the power, 2016-12-14 02:31:16]

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

[Jialiu Lin, 2016-12-14 04:36:10]

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

[commit-bot: I haz the power, 2016-12-14 04:36:20]

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-14 05:37:30]

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

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

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2016-12-14 06:25:47]

Description was changed from

==========
Create SafeBrowsingErrorUI in security_interstitials component. Move
display and command handling logic from SafeBrowsingBlockingPage to
SafeBrowsingErrorUI.

Also move all SB interstitial related strings from 
c/a/generated_reources.grd to 
components/security_interstitials_strings.grdp

Deprecate all interstitial related rappor metrics, since nobody is
using them. 

This is one of several CLs to componentize SafeBrowsingBlockingPage.
BUG=666100
==========

to

==========
This is one of several CLs to componentize SafeBrowsingBlockingPage.
(1) Create SafeBrowsingErrorUI in security_interstitials component. Move
display and command handling logic from SafeBrowsingBlockingPage to
SafeBrowsingErrorUI.

(2) Move all SB interstitial related strings from 
c/a/generated_reources.grd to 
components/security_interstitials_strings.grdp

(3) Deprecate all interstitial related rappor metrics, since nobody is
using them. 


BUG=666100
==========

[Jialiu Lin, 2016-12-14 06:34:31]

Patchset #2 (id:20001) has been deleted

[Jialiu Lin, 2016-12-14 06:34:38]

Patchset #2 (id:40001) has been deleted

[Jialiu Lin, 2016-12-14 06:37:07]

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

[Jialiu Lin, 2016-12-14 06:37:08]

Hi estark@ and nparker@, 
Could you take a look at this refactoring CL? 
No change in logic flow, except deprecating all the interstitial.* rappor
metrics.

Thanks,

[estark, 2016-12-15 21:16:34]

lgtm with a few nits

https://codereview.chromium.org/2575623002/diff/80001/chrome/browser/intersti...
File chrome/browser/interstitials/chrome_controller_client.cc (right):

https://codereview.chromium.org/2575623002/diff/80001/chrome/browser/intersti...
chrome/browser/interstitials/chrome_controller_client.cc:164: // closing th
error page. This error page will be closed when the new page
typo: th => the

https://codereview.chromium.org/2575623002/diff/80001/components/security_int...
File components/security_interstitials/core/safe_browsing_error_ui.h (right):

https://codereview.chromium.org/2575623002/diff/80001/components/security_int...
components/security_interstitials/core/safe_browsing_error_ui.h:16: class
ControllerClient;
not needed, you have the #include above

https://codereview.chromium.org/2575623002/diff/80001/components/security_int...
components/security_interstitials/core/safe_browsing_error_ui.h:29: enum
SBErrorOptionsMask {
optional nit: this might be more readable as a struct with a bunch of booleans
than a bitmask field. I know SSLBlockingPage uses an options mask like this...
but I'm not really sure why. (Just for memory savings, I guess?)

https://codereview.chromium.org/2575623002/diff/80001/components/security_int...
components/security_interstitials/core/safe_browsing_error_ui.h:33: // i.e.
!incognito && http && kSafeBrowsingExtendedReportingOptInAllowed.
Does this mean we only show the extended reporting checkbox on http (not https)
pages? Why is that?

https://codereview.chromium.org/2575623002/diff/80001/components/security_int...
components/security_interstitials/core/safe_browsing_error_ui.h:33: // i.e.
!incognito && http && kSafeBrowsingExtendedReportingOptInAllowed.
nit: Line 33 seems likely to get out of date (i.e. the logic for setting
CAN_SHOW_THREAT_DETAILS_OPTION will change but this comment won't get updated).
Maybe just delete it?

[Jialiu Lin, 2016-12-15 22:12:53]

Thanks estark@!
Your comments are all addressed.

https://codereview.chromium.org/2575623002/diff/80001/chrome/browser/intersti...
File chrome/browser/interstitials/chrome_controller_client.cc (right):

https://codereview.chromium.org/2575623002/diff/80001/chrome/browser/intersti...
chrome/browser/interstitials/chrome_controller_client.cc:164: // closing th
error page. This error page will be closed when the new page
On 2016/12/15 21:16:34, estark wrote:
> typo: th => the
Oops. Thanks for catching this.

https://codereview.chromium.org/2575623002/diff/80001/components/security_int...
File components/security_interstitials/core/safe_browsing_error_ui.h (right):

https://codereview.chromium.org/2575623002/diff/80001/components/security_int...
components/security_interstitials/core/safe_browsing_error_ui.h:16: class
ControllerClient;
On 2016/12/15 21:16:34, estark wrote:
> not needed, you have the #include above

Done.

https://codereview.chromium.org/2575623002/diff/80001/components/security_int...
components/security_interstitials/core/safe_browsing_error_ui.h:29: enum
SBErrorOptionsMask {
On 2016/12/15 21:16:34, estark wrote:
> optional nit: this might be more readable as a struct with a bunch of booleans
> than a bitmask field. I know SSLBlockingPage uses an options mask like this...
> but I'm not really sure why. (Just for memory savings, I guess?)
Agree. Changed into struct of booleans instead.

https://codereview.chromium.org/2575623002/diff/80001/components/security_int...
components/security_interstitials/core/safe_browsing_error_ui.h:33: // i.e.
!incognito && http && kSafeBrowsingExtendedReportingOptInAllowed.
On 2016/12/15 21:16:34, estark wrote:
> nit: Line 33 seems likely to get out of date (i.e. the logic for setting
> CAN_SHOW_THREAT_DETAILS_OPTION will change but this comment won't get
updated).
> Maybe just delete it?

Make sense. Done.

https://codereview.chromium.org/2575623002/diff/80001/components/security_int...
components/security_interstitials/core/safe_browsing_error_ui.h:33: // i.e.
!incognito && http && kSafeBrowsingExtendedReportingOptInAllowed.
On 2016/12/15 21:16:34, estark wrote:
> Does this mean we only show the extended reporting checkbox on http (not
https)
> pages? Why is that?

Good question. My understanding is that there is no extended reporting on https
pages due to privacy consideration, thus no need to show the checkbox. That's
what chrome privacy white paper says.

[Jialiu Lin, 2016-12-15 22:16:49]

jialiul@chromium.org changed reviewers:
+ droger@chromium.org, holte@chromium.org

[Jialiu Lin, 2016-12-15 22:16:50]

+droger@ for ios/chrome/browser/*
+holte@ for rappor.xml

Thanks!

[Steven Holte, 2016-12-15 22:24:39]

rappor lgtm

[Nathan Parker, 2016-12-15 23:30:43]

LGTM % nits, and maybe not use the large constructor for The
SBErrorDisplayOptions

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

https://codereview.chromium.org/2575623002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:113:
SafeBrowsingErrorUI::SBErrorDisplayOptions display_options(
Lots of same-type args are easy to mix up. I was going to suggest filling in the
struct one member at a time, though that has the issue that if you add a new
field, it's not easy to find all the places it should be updated.  I still think
it might be less error prone though.  I'll leave it up to you.

https://codereview.chromium.org/2575623002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:118:
sb_error_ui_.reset(new SafeBrowsingErrorUI(
I think the recommended practice is now

sb_error_ui_ = std::make_unique<SafeBrowsingErrorUI>(...)

https://codereview.chromium.org/2575623002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):

https://codereview.chromium.org/2575623002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:165:
std::unique_ptr<security_interstitials::SafeBrowsingErrorUI> sb_error_ui_;
Can you remove the security_interstitials:: since it's typedef'd above?

https://codereview.chromium.org/2575623002/diff/100001/chrome/browser/ssl/cer...
File chrome/browser/ssl/cert_report_helper.cc (right):

https://codereview.chromium.org/2575623002/diff/100001/chrome/browser/ssl/cer...
chrome/browser/ssl/cert_report_helper.cc:25: #include
"components/strings/grit/components_strings.h"
what's this needed for?

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
File components/security_interstitials/core/BUILD.gn (right):

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
components/security_interstitials/core/BUILD.gn:28: "//components/rappor",
Can we remove rappor here?

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
File components/security_interstitials/core/metrics_helper.h (right):

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
components/security_interstitials/core/metrics_helper.h:23: // histograms. The
class will generate the
nit: reflow comment

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
components/security_interstitials/core/metrics_helper.h:26: //   METRICS:
interstitial.<metric_prefix>.interaction[.repeat_visi]
nit:repeat_visit

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
File components/security_interstitials/core/safe_browsing_error_ui.cc (right):

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
components/security_interstitials/core/safe_browsing_error_ui.cc:124: switch
(command) {
I'll assume this is all copy/paste from orig file, ya?

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
File components/security_interstitials/core/safe_browsing_error_ui.h (right):

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
components/security_interstitials/core/safe_browsing_error_ui.h:81:
SBErrorDisplayOptions display_options_;
can this be const as well?

[Nathan Parker, 2016-12-15 23:31:26]

+lpz since it's moving code he just wrote

[Jialiu Lin, 2016-12-16 00:51:43]

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

https://codereview.chromium.org/2575623002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:113:
SafeBrowsingErrorUI::SBErrorDisplayOptions display_options(
On 2016/12/15 23:30:42, Nathan Parker wrote:
> Lots of same-type args are easy to mix up. I was going to suggest filling in
the
> struct one member at a time, though that has the issue that if you add a new
> field, it's not easy to find all the places it should be updated.  I still
think
> it might be less error prone though.  I'll leave it up to you.

ack. I'll probably touch this part of code later in the componentization too.
Let's see which way is better when the code is more stablized.

https://codereview.chromium.org/2575623002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:118:
sb_error_ui_.reset(new SafeBrowsingErrorUI(
On 2016/12/15 23:30:42, Nathan Parker wrote:
> I think the recommended practice is now
> 
> sb_error_ui_ = std::make_unique<SafeBrowsingErrorUI>(...)

Done.

https://codereview.chromium.org/2575623002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):

https://codereview.chromium.org/2575623002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:165:
std::unique_ptr<security_interstitials::SafeBrowsingErrorUI> sb_error_ui_;
On 2016/12/15 23:30:42, Nathan Parker wrote:
> Can you remove the security_interstitials:: since it's typedef'd above?

Right. Done.

https://codereview.chromium.org/2575623002/diff/100001/chrome/browser/ssl/cer...
File chrome/browser/ssl/cert_report_helper.cc (right):

https://codereview.chromium.org/2575623002/diff/100001/chrome/browser/ssl/cer...
chrome/browser/ssl/cert_report_helper.cc:25: #include
"components/strings/grit/components_strings.h"
On 2016/12/15 23:30:42, Nathan Parker wrote:
> what's this needed for?

This is because IDS_SAFE_BROWSING_PRIVACY_POLICY_PAGE and other strings used by
this class is now defined in components/ instead of in generated_resource.grd. 
I should probably remove chrome/grit/generated_resources.h too.

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
File components/security_interstitials/core/BUILD.gn (right):

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
components/security_interstitials/core/BUILD.gn:28: "//components/rappor",
On 2016/12/15 23:30:42, Nathan Parker wrote:
> Can we remove rappor here?

Yes. Done.

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
File components/security_interstitials/core/metrics_helper.h (right):

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
components/security_interstitials/core/metrics_helper.h:23: // histograms. The
class will generate the
On 2016/12/15 23:30:42, Nathan Parker wrote:
> nit: reflow comment

Done.

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
components/security_interstitials/core/metrics_helper.h:26: //   METRICS:
interstitial.<metric_prefix>.interaction[.repeat_visi]
On 2016/12/15 23:30:42, Nathan Parker wrote:
> nit:repeat_visit

Done.

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
File components/security_interstitials/core/safe_browsing_error_ui.cc (right):

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
components/security_interstitials/core/safe_browsing_error_ui.cc:124: switch
(command) {
On 2016/12/15 23:30:42, Nathan Parker wrote:
> I'll assume this is all copy/paste from orig file, ya?

Mostly copy/paste, except for CMD_DO_REPORT, CMD_DONT_REPORT. Since this class
cannot use safe_browsing_prefs.h, I have to record UMAs using a helper function
instead of RecordExtendedReportingPrefChanged(..)

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
File components/security_interstitials/core/safe_browsing_error_ui.h (right):

https://codereview.chromium.org/2575623002/diff/100001/components/security_in...
components/security_interstitials/core/safe_browsing_error_ui.h:81:
SBErrorDisplayOptions display_options_;
On 2016/12/15 23:30:42, Nathan Parker wrote:
> can this be const as well?

check/uncheck extended reporting box will change display_options_

[droger, 2016-12-16 09:27:26]

lgtm

[Jialiu Lin, 2016-12-16 17:26:24]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2016-12-16 17:26:25]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org,
nparker@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2575623002/#ps120001
(title: "address comments from nparker@")

[commit-bot: I haz the power, 2016-12-16 17:27:03]

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 19:15:56]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481909184975950,
"parent_rev": "4ee1eb67fd3c004609d228dfe764cd73155588b9", "commit_rev":
"95c8af2c6ce6fafcaad080c668227702b5e8fb6b"}

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

Description was changed from

==========
This is one of several CLs to componentize SafeBrowsingBlockingPage.
(1) Create SafeBrowsingErrorUI in security_interstitials component. Move
display and command handling logic from SafeBrowsingBlockingPage to
SafeBrowsingErrorUI.

(2) Move all SB interstitial related strings from 
c/a/generated_reources.grd to 
components/security_interstitials_strings.grdp

(3) Deprecate all interstitial related rappor metrics, since nobody is
using them. 


BUG=666100
==========

to

==========
This is one of several CLs to componentize SafeBrowsingBlockingPage.
(1) Create SafeBrowsingErrorUI in security_interstitials component. Move
display and command handling logic from SafeBrowsingBlockingPage to
SafeBrowsingErrorUI.

(2) Move all SB interstitial related strings from 
c/a/generated_reources.grd to 
components/security_interstitials_strings.grdp

(3) Deprecate all interstitial related rappor metrics, since nobody is
using them. 


BUG=666100

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

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

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-12-16 19:20:35]

Description was changed from

==========
This is one of several CLs to componentize SafeBrowsingBlockingPage.
(1) Create SafeBrowsingErrorUI in security_interstitials component. Move
display and command handling logic from SafeBrowsingBlockingPage to
SafeBrowsingErrorUI.

(2) Move all SB interstitial related strings from 
c/a/generated_reources.grd to 
components/security_interstitials_strings.grdp

(3) Deprecate all interstitial related rappor metrics, since nobody is
using them. 


BUG=666100

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

to

==========
This is one of several CLs to componentize SafeBrowsingBlockingPage.
(1) Create SafeBrowsingErrorUI in security_interstitials component. Move
display and command handling logic from SafeBrowsingBlockingPage to
SafeBrowsingErrorUI.

(2) Move all SB interstitial related strings from
c/a/generated_reources.grd to
components/security_interstitials_strings.grdp

(3) Deprecate all interstitial related rappor metrics, since nobody is
using them.

BUG=666100

Committed: https://crrev.com/68ea739f83af7661f140802bd8941a577f02eb30
Cr-Commit-Position: refs/heads/master@{#439163}
==========

[commit-bot: I haz the power, 2016-12-16 19:20:36]

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