Plumbing for safe browsing reporting for Android WebView.

Plumbing for safe browsing reporting for Android WebView.

Provide the necessary hooks for the reporting to work in
Android WebView. In particular
- implement AwSafeBrowsingUIManager::SendSerializedThreatDetails,
- create ping manager and url request context getter on IO/UI threads,
- create trigger_manager and expose in AwBrowserContext,
- add/create DIR_SAFE_BROWSING for storing safe browsing cookies,
- provide Android WebView specific protocol config client name (crbug.com/732373),
- move the relevant constants from safebrowsing_service to component.

TODO (in a follow-up patch):
- invoke {Start,Finish}CollectingThreatDetails() from the AwSafeBrowsingBlockingPage (crbug.com/731747)

Design doc: https://goo.gl/TfoY9K, section 3.

BUG=700351, 688629, 732373
Review-Url: https://codereview.chromium.org/2932703003
Cr-Commit-Position: refs/heads/master@{#479468}
Committed: https://chromium.googlesource.com/chromium/src/+/2ef5d746246fd46bccc0035070da7a2988c1772f

[timvolodine, 2017-06-08 16:09:06]

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

[commit-bot: I haz the power, 2017-06-08 16:09: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-08 16:15:43]

Description was changed from

==========
Plumbing for safe browsing reporting for Android WebView.

Provide the necessary hooks for the reporting to work in
Android WebView. In particular
- implement AwSafeBrowsingUIManager::SendSerializedThreatDetails
- create ping manager and url request context getter on IO/UI threads.
- add DIR_SAFE_BROWSING for storing safe browsing cookies
- move relevant constant from safebrowsing_service to component

Design doc: https://goo.gl/TfoY9K, section 3.

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

to

==========
Plumbing for safe browsing reporting for Android WebView.

Provide the necessary hooks for the reporting to work in
Android WebView. In particular
- implement AwSafeBrowsingUIManager::SendSerializedThreatDetails
- create ping manager and url request context getter on IO/UI threads.
- add DIR_SAFE_BROWSING for storing safe browsing cookies
- move the relevant constants from safebrowsing_service to component

TODO:
- invoke StartCollectingThreatDetails() from the AwSafeBrowsingBlockingPage
(requires trigger manager)

Design doc: https://goo.gl/TfoY9K, section 3.

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

[timvolodine, 2017-06-08 16:30:37]

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

[timvolodine, 2017-06-08 16:30:38]

still a TODO left, but thought I could get some feedback already on the current
code.

jialiul@: PTAL, I've added a section 3 to the design doc with some more details
(https://goo.gl/TfoY9K, section 3).

sgurun@: for the webview bits, in particular the aw_paths DIR_SAFE_BROWSING
changes.

[timvolodine, 2017-06-08 16:33:13]

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

[commit-bot: I haz the power, 2017-06-08 16:33:36]

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-08 18:19:29]

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

[commit-bot: I haz the power, 2017-06-08 18:19:31]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-06-08 18:19:38]

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

[Jialiu Lin, 2017-06-08 18:19:39]

+lpz@, since we recently added TriggerManager (already in component) to handle
the triggering of ThreatDetails collection.

[sgurun-gerrit only, 2017-06-08 21:52:55]

sgurun@chromium.org changed reviewers:
+ ntfschr@chromium.org

[sgurun-gerrit only, 2017-06-08 21:53:36]

On 2017/06/08 18:19:39, Jialiu Lin wrote:
> +lpz@, since we recently added TriggerManager (already in component) to handle
> the triggering of ThreatDetails collection.

nate, for initial review.

[Nate Fischer, 2017-06-08 22:21:47]

Initial changes LGTM % comments

https://codereview.chromium.org/2932703003/diff/20001/android_webview/browser...
File android_webview/browser/aw_safe_browsing_ui_manager.cc (right):

https://codereview.chromium.org/2932703003/diff/20001/android_webview/browser...
android_webview/browser/aw_safe_browsing_ui_manager.cc:30: client_name =
"chromium";
Do these names still make sense for webview? Not sure if this matters to Safe
Browsing team. If we change the names, we can remove the "OS_ANDROID" section
below.

https://codereview.chromium.org/2932703003/diff/20001/android_webview/browser...
File android_webview/browser/aw_safe_browsing_ui_manager.h (right):

https://codereview.chromium.org/2932703003/diff/20001/android_webview/browser...
android_webview/browser/aw_safe_browsing_ui_manager.h:45: // BaseUIManager:
Any reason why we're mentioning the base class here?

[timvolodine, 2017-06-09 11:52:10]

+cc:tobiasjs@ : as the original author of aw_paths, in case there are any
comments/thoughts regarding DIR_SAFE_BROWSING changes.

https://codereview.chromium.org/2932703003/diff/20001/android_webview/browser...
File android_webview/browser/aw_safe_browsing_ui_manager.h (right):

https://codereview.chromium.org/2932703003/diff/20001/android_webview/browser...
android_webview/browser/aw_safe_browsing_ui_manager.h:45: // BaseUIManager:
On 2017/06/08 22:21:47, Nate Fischer wrote:
> Any reason why we're mentioning the base class here?

This is sort of standard practice to indicate the methods below are overridden
from the BaseUIManager class (we have similar comment e.g. in base_blocking_page
and base_ping_manager). I've modified the comment to "BaseUIManager methods:" to
make it clearer and also moved the DisplayBlockingPage() method in this section.

[timvolodine, 2017-06-09 11:53:21]

https://codereview.chromium.org/2932703003/diff/20001/android_webview/browser...
File android_webview/browser/aw_safe_browsing_ui_manager.cc (right):

https://codereview.chromium.org/2932703003/diff/20001/android_webview/browser...
android_webview/browser/aw_safe_browsing_ui_manager.cc:30: client_name =
"chromium";
On 2017/06/08 22:21:47, Nate Fischer wrote:
> Do these names still make sense for webview? Not sure if this matters to Safe
> Browsing team. If we change the names, we can remove the "OS_ANDROID" section
> below.

good point, will try to discuss this during the meeting today.

[lpz, 2017-06-09 14:54:51]

lgtm. I left a similar comment in your design doc. Lets also maybe do a quick
sync to make sure there are no sharp edged wrt trigger manager going forward, I
can schedule something.

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

https://codereview.chromium.org/2932703003/diff/40001/android_webview/browser...
android_webview/browser/aw_safe_browsing_blocking_page.cc:47: //
TriggerManager::StartCollectingThreatDetails().
note also that after https://chromium-review.googlesource.com/c/525854 you'll
need to call TriggerManager::FinishCollectingThreatDetails from somewhere

[Tobias Sargeant, 2017-06-09 14:57:01]

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

[Tobias Sargeant, 2017-06-09 14:57:02]

https://codereview.chromium.org/2932703003/diff/40001/android_webview/common/...
File android_webview/common/aw_paths.cc (right):

https://codereview.chromium.org/2932703003/diff/40001/android_webview/common/...
android_webview/common/aw_paths.cc:28:
.Append(FILE_PATH_LITERAL("SafeBrowsing"));
is that directory going to get created by existing code?

[selim, 2017-06-09 15:21:27]

On 2017/06/09 14:57:02, Tobias Sargeant wrote:
>
https://codereview.chromium.org/2932703003/diff/40001/android_webview/common/...
> File android_webview/common/aw_paths.cc (right):
> 
>
https://codereview.chromium.org/2932703003/diff/40001/android_webview/common/...
> android_webview/common/aw_paths.cc:28:
> .Append(FILE_PATH_LITERAL("SafeBrowsing"));
> is that directory going to get created by existing code?

thanks for working on this.

For TODOs please add crbugs that are blocking the root reporting bug. TODOs are
impossible to track from project release perspective.

[timvolodine, 2017-06-09 16:25:27]

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

[Jialiu Lin, 2017-06-09 16:25:33]

LGTM.

[commit-bot: I haz the power, 2017-06-09 16:25:46]

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-09 16:28:43]

Thanks for the comments everybody,

- added trigger_manager to AwBrowserContext
- filed two blocking bugs to keep track of TODOs (crbug.com/731744,
crbug.com/731747)

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

https://codereview.chromium.org/2932703003/diff/40001/android_webview/browser...
android_webview/browser/aw_safe_browsing_blocking_page.cc:47: //
TriggerManager::StartCollectingThreatDetails().
On 2017/06/09 14:54:50, lpz wrote:
> note also that after https://chromium-review.googlesource.com/c/525854 you'll
> need to call TriggerManager::FinishCollectingThreatDetails from somewhere

Ah thanks for pointing out, filed crbug.com/731747 to address that. Any idea
when your patch is going to land?

https://codereview.chromium.org/2932703003/diff/40001/android_webview/common/...
File android_webview/common/aw_paths.cc (right):

https://codereview.chromium.org/2932703003/diff/40001/android_webview/common/...
android_webview/common/aw_paths.cc:28:
.Append(FILE_PATH_LITERAL("SafeBrowsing"));
On 2017/06/09 14:57:02, Tobias Sargeant wrote:
> is that directory going to get created by existing code?

Good point thanks for noticing! I guess I'll need to add some code to
aw_browser_main_parts.cc to create it. Another option could be to use
base::DIR_ANDROID_APP_DATA instead..

[timvolodine, 2017-06-09 17:45:49]

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

[commit-bot: I haz the power, 2017-06-09 17:46:47]

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-09 17:48:49]

tobiasjs@: added directory creation (see below), ptal.

https://codereview.chromium.org/2932703003/diff/40001/android_webview/common/...
File android_webview/common/aw_paths.cc (right):

https://codereview.chromium.org/2932703003/diff/40001/android_webview/common/...
android_webview/common/aw_paths.cc:28:
.Append(FILE_PATH_LITERAL("SafeBrowsing"));
On 2017/06/09 16:28:43, timvolodine wrote:
> On 2017/06/09 14:57:02, Tobias Sargeant wrote:
> > is that directory going to get created by existing code?
> 
> Good point thanks for noticing! I guess I'll need to add some code to
> aw_browser_main_parts.cc to create it. Another option could be to use
> base::DIR_ANDROID_APP_DATA instead..

After the meeting today, went with DIR_SAFE_BROWSING (created in
aw_browser_main_parts).

Done.

[timvolodine, 2017-06-09 17:55:16]

On 2017/06/09 14:54:51, lpz wrote:
> lgtm. I left a similar comment in your design doc. Lets also maybe do a quick
> sync to make sure there are no sharp edged wrt trigger manager going forward,
I
> can schedule something.
> 
>
https://codereview.chromium.org/2932703003/diff/40001/android_webview/browser...
> File android_webview/browser/aw_safe_browsing_blocking_page.cc (right):
> 
>
https://codereview.chromium.org/2932703003/diff/40001/android_webview/browser...
> android_webview/browser/aw_safe_browsing_blocking_page.cc:47: //
> TriggerManager::StartCollectingThreatDetails().
> note also that after https://chromium-review.googlesource.com/c/525854 you'll
> need to call TriggerManager::FinishCollectingThreatDetails from somewhere

lpz@: Thanks for the comment on the design doc. Happy to catch-up, next week is
fine.

While I have some initial code for StartCollectingThreatDetails() I think it's
best addressed in a follow-up patch (because it requires some refactoring),
probably together with FinishCollectingThreatDetails().. this is tracked in
crbug.com/731747.

[timvolodine, 2017-06-09 17:56:58]

Description was changed from

==========
Plumbing for safe browsing reporting for Android WebView.

Provide the necessary hooks for the reporting to work in
Android WebView. In particular
- implement AwSafeBrowsingUIManager::SendSerializedThreatDetails
- create ping manager and url request context getter on IO/UI threads.
- add DIR_SAFE_BROWSING for storing safe browsing cookies
- move the relevant constants from safebrowsing_service to component

TODO:
- invoke StartCollectingThreatDetails() from the AwSafeBrowsingBlockingPage
(requires trigger manager)

Design doc: https://goo.gl/TfoY9K, section 3.

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

to

==========
Plumbing for safe browsing reporting for Android WebView.

Provide the necessary hooks for the reporting to work in
Android WebView. In particular
- implement AwSafeBrowsingUIManager::SendSerializedThreatDetails,
- create ping manager and url request context getter on IO/UI threads,
- create trigger_manager and expose in AwBrowserContext,
- add/create DIR_SAFE_BROWSING for storing safe browsing cookies,
- move the relevant constants from safebrowsing_service to component

TODO:
- invoke StartCollectingThreatDetails() from the AwSafeBrowsingBlockingPage
(requires trigger manager)

Design doc: https://goo.gl/TfoY9K, section 3.

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

[commit-bot: I haz the power, 2017-06-09 19:25:07]

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

[commit-bot: I haz the power, 2017-06-09 19:25:08]

Dry run: This issue passed the CQ dry run.

[sgurun-gerrit only, 2017-06-09 21:10:40]

On 2017/06/09 17:55:16, timvolodine wrote:
> On 2017/06/09 14:54:51, lpz wrote:
> > lgtm. I left a similar comment in your design doc. Lets also maybe do a
quick
> > sync to make sure there are no sharp edged wrt trigger manager going
forward,
> I
> > can schedule something.
> > 
> >
>
https://codereview.chromium.org/2932703003/diff/40001/android_webview/browser...
> > File android_webview/browser/aw_safe_browsing_blocking_page.cc (right):
> > 
> >
>
https://codereview.chromium.org/2932703003/diff/40001/android_webview/browser...
> > android_webview/browser/aw_safe_browsing_blocking_page.cc:47: //
> > TriggerManager::StartCollectingThreatDetails().
> > note also that after https://chromium-review.googlesource.com/c/525854
you'll
> > need to call TriggerManager::FinishCollectingThreatDetails from somewhere
> 
> lpz@: Thanks for the comment on the design doc. Happy to catch-up, next week
is
> fine.
> 
> While I have some initial code for StartCollectingThreatDetails() I think it's
> best addressed in a follow-up patch (because it requires some refactoring),
> probably together with FinishCollectingThreatDetails().. this is tracked in
> crbug.com/731747.

lgtm

[Jialiu Lin, 2017-06-12 17:05:40]

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

https://codereview.chromium.org/2932703003/diff/80001/android_webview/browser...
android_webview/browser/aw_safe_browsing_ui_manager.cc:26: std::string
client_name;
Based on awoz@'s email, feel free to return "android_webview" as protocol config
client name here. 
(no need for all these if-defs)

[timvolodine, 2017-06-14 17:56:06]

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

https://codereview.chromium.org/2932703003/diff/80001/android_webview/browser...
android_webview/browser/aw_safe_browsing_ui_manager.cc:26: std::string
client_name;
On 2017/06/12 17:05:40, Jialiu Lin wrote:
> Based on awoz@'s email, feel free to return "android_webview" as protocol
config
> client name here. 
> (no need for all these if-defs)

Done.
Also added a comment and filed crbug.com/732373 with more details on this
decision and to track any potential issues regarding this change..

[timvolodine, 2017-06-14 17:59:09]

Description was changed from

==========
Plumbing for safe browsing reporting for Android WebView.

Provide the necessary hooks for the reporting to work in
Android WebView. In particular
- implement AwSafeBrowsingUIManager::SendSerializedThreatDetails,
- create ping manager and url request context getter on IO/UI threads,
- create trigger_manager and expose in AwBrowserContext,
- add/create DIR_SAFE_BROWSING for storing safe browsing cookies,
- move the relevant constants from safebrowsing_service to component

TODO:
- invoke StartCollectingThreatDetails() from the AwSafeBrowsingBlockingPage
(requires trigger manager)

Design doc: https://goo.gl/TfoY9K, section 3.

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

to

==========
Plumbing for safe browsing reporting for Android WebView.

Provide the necessary hooks for the reporting to work in
Android WebView. In particular
- implement AwSafeBrowsingUIManager::SendSerializedThreatDetails,
- create ping manager and url request context getter on IO/UI threads,
- create trigger_manager and expose in AwBrowserContext,
- add/create DIR_SAFE_BROWSING for storing safe browsing cookies,
- move the relevant constants from safebrowsing_service to component

TODO (in a follow-up patch):
- invoke {Start,Finish}CollectingThreatDetails() from the
AwSafeBrowsingBlockingPage (crbug.com/731747)

Design doc: https://goo.gl/TfoY9K, section 3.

BUG=700351, 688629, 732373
==========

[timvolodine, 2017-06-14 18:00:51]

Description was changed from

==========
Plumbing for safe browsing reporting for Android WebView.

Provide the necessary hooks for the reporting to work in
Android WebView. In particular
- implement AwSafeBrowsingUIManager::SendSerializedThreatDetails,
- create ping manager and url request context getter on IO/UI threads,
- create trigger_manager and expose in AwBrowserContext,
- add/create DIR_SAFE_BROWSING for storing safe browsing cookies,
- move the relevant constants from safebrowsing_service to component

TODO (in a follow-up patch):
- invoke {Start,Finish}CollectingThreatDetails() from the
AwSafeBrowsingBlockingPage (crbug.com/731747)

Design doc: https://goo.gl/TfoY9K, section 3.

BUG=700351, 688629, 732373
==========

to

==========
Plumbing for safe browsing reporting for Android WebView.

Provide the necessary hooks for the reporting to work in
Android WebView. In particular
- implement AwSafeBrowsingUIManager::SendSerializedThreatDetails,
- create ping manager and url request context getter on IO/UI threads,
- create trigger_manager and expose in AwBrowserContext,
- add/create DIR_SAFE_BROWSING for storing safe browsing cookies,
- provide Android WebView specific protocol config client name
(crbug.com/732373),
- move the relevant constants from safebrowsing_service to component.

TODO (in a follow-up patch):
- invoke {Start,Finish}CollectingThreatDetails() from the
AwSafeBrowsingBlockingPage (crbug.com/731747)

Design doc: https://goo.gl/TfoY9K, section 3.

BUG=700351, 688629, 732373
==========

[timvolodine, 2017-06-14 18:02:36]

Thanks for the reviews!

[timvolodine, 2017-06-14 18:03:04]

The CQ bit was checked by timvolodine@chromium.org

[timvolodine, 2017-06-14 18:03:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
ntfschr@chromium.org, lpz@chromium.org, jialiul@chromium.org,
sgurun@chromium.org
Link to the patchset: https://codereview.chromium.org/2932703003/#ps100001
(title: "modify client name to android_webview")

[commit-bot: I haz the power, 2017-06-14 18:03:31]

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-14 19:41:28]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1497463384264060,
"parent_rev": "4062088042578507d64ea2c45dfe22e06bfe793b", "commit_rev":
"2ef5d746246fd46bccc0035070da7a2988c1772f"}

[commit-bot: I haz the power, 2017-06-14 19:41:44]

Description was changed from

==========
Plumbing for safe browsing reporting for Android WebView.

Provide the necessary hooks for the reporting to work in
Android WebView. In particular
- implement AwSafeBrowsingUIManager::SendSerializedThreatDetails,
- create ping manager and url request context getter on IO/UI threads,
- create trigger_manager and expose in AwBrowserContext,
- add/create DIR_SAFE_BROWSING for storing safe browsing cookies,
- provide Android WebView specific protocol config client name
(crbug.com/732373),
- move the relevant constants from safebrowsing_service to component.

TODO (in a follow-up patch):
- invoke {Start,Finish}CollectingThreatDetails() from the
AwSafeBrowsingBlockingPage (crbug.com/731747)

Design doc: https://goo.gl/TfoY9K, section 3.

BUG=700351, 688629, 732373
==========

to

==========
Plumbing for safe browsing reporting for Android WebView.

Provide the necessary hooks for the reporting to work in
Android WebView. In particular
- implement AwSafeBrowsingUIManager::SendSerializedThreatDetails,
- create ping manager and url request context getter on IO/UI threads,
- create trigger_manager and expose in AwBrowserContext,
- add/create DIR_SAFE_BROWSING for storing safe browsing cookies,
- provide Android WebView specific protocol config client name
(crbug.com/732373),
- move the relevant constants from safebrowsing_service to component.

TODO (in a follow-up patch):
- invoke {Start,Finish}CollectingThreatDetails() from the
AwSafeBrowsingBlockingPage (crbug.com/731747)

Design doc: https://goo.gl/TfoY9K, section 3.

BUG=700351, 688629, 732373

Review-Url: https://codereview.chromium.org/2932703003
Cr-Commit-Position: refs/heads/master@{#479468}
Committed:
https://chromium.googlesource.com/chromium/src/+/2ef5d746246fd46bccc0035070da...
==========

[commit-bot: I haz the power, 2017-06-14 19:41:45]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/2ef5d746246fd46bccc0035070da...