Implement code for Start/Finish threat details collection in WebView.

Implement code for Start/Finish threat details collection in WebView.

Provide calls to start/finish threat details collection
in Android WebView. In particular:
- implement calls to {Start,Finish}CollectThreadDetails in
aw_safe_browsing_blocking_page and related code,
- add crbug reference regarding HistoryService,
- refactor common parts into base blocking page class.

BUG=700351, 688629, 731747
Review-Url: https://codereview.chromium.org/2950863002
Cr-Commit-Position: refs/heads/master@{#483751}
Committed: https://chromium.googlesource.com/chromium/src/+/840e30ceebc2e8c39657d144f2aea24db1124256

[timvolodine, 2017-06-20 16:23:28]

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

[commit-bot: I haz the power, 2017-06-20 16:24:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[timvolodine, 2017-06-20 16:24:27]

Description was changed from

==========
Enable threat details collection in WebView.

Provide calls to start/finish threat details collection
in Android WebView. In particular:
- implement calls to {Start,Finish}CollectThreadDetails in
aw_safe_browsing_blocking_page and related code,
- add crbug reference regarding HistoryService
- refactor common parts into base blocking page class.

TODO:
pending resolution with
https://chromium-review.googlesource.com/c/537392/4

BUG=700351, 688629, 731747
==========

to

==========
Enable threat details collection in WebView.

WIP

Provide calls to start/finish threat details collection
in Android WebView. In particular:
- implement calls to {Start,Finish}CollectThreadDetails in
aw_safe_browsing_blocking_page and related code,
- add crbug reference regarding HistoryService
- refactor common parts into base blocking page class.

TODO:
pending resolution with
https://chromium-review.googlesource.com/c/537392/4

BUG=700351, 688629, 731747
==========

[commit-bot: I haz the power, 2017-06-20 16:37:15]

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

[commit-bot: I haz the power, 2017-06-20 16:37:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[timvolodine, 2017-06-23 14:39:37]

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

[commit-bot: I haz the power, 2017-06-23 14:39:42]

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, 2017-06-23 15:45:46]

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

[commit-bot: I haz the power, 2017-06-23 15:45:47]

Dry run: This issue passed the CQ dry run.

[timvolodine, 2017-06-29 19:13:27]

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

[commit-bot: I haz the power, 2017-06-29 19:14:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[timvolodine, 2017-06-29 19:14:04]

Description was changed from

==========
Enable threat details collection in WebView.

WIP

Provide calls to start/finish threat details collection
in Android WebView. In particular:
- implement calls to {Start,Finish}CollectThreadDetails in
aw_safe_browsing_blocking_page and related code,
- add crbug reference regarding HistoryService
- refactor common parts into base blocking page class.

TODO:
pending resolution with
https://chromium-review.googlesource.com/c/537392/4

BUG=700351, 688629, 731747
==========

to

==========
Enable threat details collection in WebView.

Provide calls to start/finish threat details collection
in Android WebView. In particular:
- implement calls to {Start,Finish}CollectThreadDetails in
aw_safe_browsing_blocking_page and related code,
- add crbug reference regarding HistoryService
- refactor common parts into base blocking page class.

TODO:
pending resolution with
https://chromium-review.googlesource.com/c/537392/4

BUG=700351, 688629, 731747
==========

[timvolodine, 2017-06-29 19:16:34]

timvolodine@chromium.org changed reviewers:
+ jialiul@chromium.org, lpz@chromium.org, sgurun@chromium.org

[timvolodine, 2017-06-29 19:16:35]

jialiul@, lpz@ : for the safe_browsing bits
sgurun@ : for the webview specific bits

please take a look!

[commit-bot: I haz the power, 2017-06-29 19:17:44]

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

[commit-bot: I haz the power, 2017-06-29 19:17:45]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jialiu Lin, 2017-06-29 19:31:16]

LGTM

https://codereview.chromium.org/2950863002/diff/40001/android_webview/browser...
File android_webview/browser/aw_safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2950863002/diff/40001/android_webview/browser...
android_webview/browser/aw_safe_browsing_blocking_page.cc:128: if (report_sent)
{
If I remember correct, WebView doesn't have UMA.  We probably don't need to
record user interaction here.

[sgurun-gerrit only, 2017-06-30 03:15:19]

On 2017/06/29 19:31:16, Jialiu Lin wrote:
> LGTM
> 
>
https://codereview.chromium.org/2950863002/diff/40001/android_webview/browser...
> File android_webview/browser/aw_safe_browsing_blocking_page.cc (right):
> 
>
https://codereview.chromium.org/2950863002/diff/40001/android_webview/browser...
> android_webview/browser/aw_safe_browsing_blocking_page.cc:128: if
(report_sent)
> {
> If I remember correct, WebView doesn't have UMA.  We probably don't need to
> record user interaction here.

webview does have UMA now.

[Jialiu Lin, 2017-06-30 03:19:01]

On 2017/06/30 03:15:19, sgurun wrote:
> On 2017/06/29 19:31:16, Jialiu Lin wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/2950863002/diff/40001/android_webview/browser...
> > File android_webview/browser/aw_safe_browsing_blocking_page.cc (right):
> > 
> >
>
https://codereview.chromium.org/2950863002/diff/40001/android_webview/browser...
> > android_webview/browser/aw_safe_browsing_blocking_page.cc:128: if
> (report_sent)
> > {
> > If I remember correct, WebView doesn't have UMA.  We probably don't need to
> > record user interaction here.
> 
> webview does have UMA now.

That's so great! Good to know. Thanks sgurun! :-)

[sgurun-gerrit only, 2017-06-30 03:25:45]

that actually does not enable threat details, right, because we still don't show
the reporting option to the user:

https://cs.chromium.org/chromium/src/android_webview/browser/aw_safe_browsing...

[timvolodine, 2017-06-30 13:06:50]

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

[commit-bot: I haz the power, 2017-06-30 13:07:01]

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, 2017-06-30 13:09:31]

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

[commit-bot: I haz the power, 2017-06-30 13:09:32]

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

[timvolodine, 2017-06-30 13:55:26]

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

[commit-bot: I haz the power, 2017-06-30 13:55:48]

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, 2017-06-30 13:57:52]

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

[commit-bot: I haz the power, 2017-06-30 13:57:53]

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-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lpz, 2017-06-30 14:01:23]

LGTM

One nit: the TODO in the CL description no longer applies, i think?

https://codereview.chromium.org/2950863002/diff/80001/android_webview/browser...
File android_webview/browser/aw_safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2950863002/diff/80001/android_webview/browser...
android_webview/browser/aw_safe_browsing_blocking_page.cc:116: // Not all
interstitials collect threat details (eg., incognito mode).
is this comment accurate (eg: is there incognito in webview)?

[timvolodine, 2017-06-30 14:04:07]

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

[commit-bot: I haz the power, 2017-06-30 14:04:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[timvolodine, 2017-06-30 14:11:17]

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

[commit-bot: I haz the power, 2017-06-30 14:11:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[timvolodine, 2017-06-30 14:13:13]

Description was changed from

==========
Enable threat details collection in WebView.

Provide calls to start/finish threat details collection
in Android WebView. In particular:
- implement calls to {Start,Finish}CollectThreadDetails in
aw_safe_browsing_blocking_page and related code,
- add crbug reference regarding HistoryService
- refactor common parts into base blocking page class.

TODO:
pending resolution with
https://chromium-review.googlesource.com/c/537392/4

BUG=700351, 688629, 731747
==========

to

==========
Enable threat details collection in WebView.

Provide calls to start/finish threat details collection
in Android WebView. In particular:
- implement calls to {Start,Finish}CollectThreadDetails in
aw_safe_browsing_blocking_page and related code,
- add crbug reference regarding HistoryService
- refactor common parts into base blocking page class.

BUG=700351, 688629, 731747
==========

[timvolodine, 2017-06-30 14:13:32]

Description was changed from

==========
Enable threat details collection in WebView.

Provide calls to start/finish threat details collection
in Android WebView. In particular:
- implement calls to {Start,Finish}CollectThreadDetails in
aw_safe_browsing_blocking_page and related code,
- add crbug reference regarding HistoryService
- refactor common parts into base blocking page class.

BUG=700351, 688629, 731747
==========

to

==========
Enable threat details collection in WebView.

Provide calls to start/finish threat details collection
in Android WebView. In particular:
- implement calls to {Start,Finish}CollectThreadDetails in
aw_safe_browsing_blocking_page and related code,
- add crbug reference regarding HistoryService,
- refactor common parts into base blocking page class.

BUG=700351, 688629, 731747
==========

[timvolodine, 2017-06-30 14:14:25]

Description was changed from

==========
Enable threat details collection in WebView.

Provide calls to start/finish threat details collection
in Android WebView. In particular:
- implement calls to {Start,Finish}CollectThreadDetails in
aw_safe_browsing_blocking_page and related code,
- add crbug reference regarding HistoryService,
- refactor common parts into base blocking page class.

BUG=700351, 688629, 731747
==========

to

==========
Implement code for Start/Finish threat details collection in WebView.

Provide calls to start/finish threat details collection
in Android WebView. In particular:
- implement calls to {Start,Finish}CollectThreadDetails in
aw_safe_browsing_blocking_page and related code,
- add crbug reference regarding HistoryService,
- refactor common parts into base blocking page class.

BUG=700351, 688629, 731747
==========

[timvolodine, 2017-06-30 14:15:11]

Thanks for the comments!

https://codereview.chromium.org/2950863002/diff/40001/android_webview/browser...
File android_webview/browser/aw_safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2950863002/diff/40001/android_webview/browser...
android_webview/browser/aw_safe_browsing_blocking_page.cc:128: if (report_sent)
{
On 2017/06/29 19:31:16, Jialiu Lin wrote:
> If I remember correct, WebView doesn't have UMA.  We probably don't need to
> record user interaction here.

We actually have UMA in webview now, so this is still useful :)

https://codereview.chromium.org/2950863002/diff/80001/android_webview/browser...
File android_webview/browser/aw_safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2950863002/diff/80001/android_webview/browser...
android_webview/browser/aw_safe_browsing_blocking_page.cc:116: // Not all
interstitials collect threat details (eg., incognito mode).
On 2017/06/30 14:01:23, lpz wrote:
> is this comment accurate (eg: is there incognito in webview)?

Right, no incognito for webview. I've amended the comment to
"Not all interstitials collect threat details, e.g. when not opted in"

[timvolodine, 2017-06-30 14:19:14]

On 2017/06/30 03:25:45, sgurun wrote:
> that actually does not enable threat details, right, because we still don't
show
> the reporting option to the user:
> 
>
https://cs.chromium.org/chromium/src/android_webview/browser/aw_safe_browsing...

That's correct, we'll need to set kSafeBrowsingExtendedReportingOptInAllowed to
true if we want the user to be able to opt in. I've modified the title of this
patch to better reflect its purpose. I'll prepare a follow-up with a PrefService
(in WebView), we can also discuss the safe-browsing defaults we would like to
have in the case of webview.

[timvolodine, 2017-06-30 14:19:46]

On 2017/06/30 14:01:23, lpz wrote:
> LGTM
> 
> One nit: the TODO in the CL description no longer applies, i think?

yes, thanks, removed the TODO. done.

[commit-bot: I haz the power, 2017-06-30 15:12:26]

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

[commit-bot: I haz the power, 2017-06-30 15:12:26]

Dry run: This issue passed the CQ dry run.

[sgurun-gerrit only, 2017-06-30 17:05:03]

On 2017/06/30 14:19:46, timvolodine wrote:
> On 2017/06/30 14:01:23, lpz wrote:
> > LGTM
> > 
> > One nit: the TODO in the CL description no longer applies, i think?
> 
> yes, thanks, removed the TODO. done.

lgtm

[timvolodine, 2017-06-30 17:46:56]

Thanks for the reviews!

[timvolodine, 2017-06-30 17:47:02]

The CQ bit was checked by timvolodine@chromium.org

[timvolodine, 2017-06-30 17:47:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
jialiul@chromium.org, lpz@chromium.org
Link to the patchset: https://codereview.chromium.org/2950863002/#ps120001
(title: "fix comment")

[commit-bot: I haz the power, 2017-06-30 17:47:27]

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, 2017-06-30 17:51:45]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1498844822696960,
"parent_rev": "817370ae855c288809072aecb07e6063a13eda79", "commit_rev":
"746fd000745ab48fae5cb7c7bc6251bf4a8b92d7"}

[commit-bot: I haz the power, 2017-06-30 17:51:59]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1498844822696960,
"parent_rev": "4fd794c35351c388f49dd111d6c6c6432312d1fe", "commit_rev":
"840e30ceebc2e8c39657d144f2aea24db1124256"}

[commit-bot: I haz the power, 2017-06-30 17:52:17]

Description was changed from

==========
Implement code for Start/Finish threat details collection in WebView.

Provide calls to start/finish threat details collection
in Android WebView. In particular:
- implement calls to {Start,Finish}CollectThreadDetails in
aw_safe_browsing_blocking_page and related code,
- add crbug reference regarding HistoryService,
- refactor common parts into base blocking page class.

BUG=700351, 688629, 731747
==========

to

==========
Implement code for Start/Finish threat details collection in WebView.

Provide calls to start/finish threat details collection
in Android WebView. In particular:
- implement calls to {Start,Finish}CollectThreadDetails in
aw_safe_browsing_blocking_page and related code,
- add crbug reference regarding HistoryService,
- refactor common parts into base blocking page class.

BUG=700351, 688629, 731747

Review-Url: https://codereview.chromium.org/2950863002
Cr-Commit-Position: refs/heads/master@{#483751}
Committed:
https://chromium.googlesource.com/chromium/src/+/840e30ceebc2e8c39657d144f2ae...
==========

[commit-bot: I haz the power, 2017-06-30 17:52:18]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/840e30ceebc2e8c39657d144f2ae...