Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(539)

Issue 2932703003: Plumbing for safe browsing reporting for Android WebView. (Closed)

Created:
3 years, 6 months ago by timvolodine
Modified:
3 years, 6 months ago
CC:
chromium-reviews, grt+watch_chromium.org, android-webview-reviews_chromium.org, vakh+watch_chromium.org, Tobias Sargeant
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : add TODO #

Total comments: 4

Patch Set 3 : address comment #

Total comments: 5

Patch Set 4 : add trigger_manager and crbugs #

Patch Set 5 : create DIR_SAFE_BROWSING, (plus some cleanup) #

Total comments: 2

Patch Set 6 : modify client name to android_webview #

Unified diffs Side-by-side diffs Delta from patch set Stats (+161 lines, -28 lines) Patch
M android_webview/BUILD.gn View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M android_webview/browser/aw_browser_context.h View 1 2 3 3 chunks +6 lines, -0 lines 0 comments Download
M android_webview/browser/aw_browser_context.cc View 1 2 3 3 chunks +12 lines, -1 line 0 comments Download
M android_webview/browser/aw_browser_main_parts.cc View 1 2 3 4 2 chunks +10 lines, -0 lines 0 comments Download
M android_webview/browser/aw_safe_browsing_blocking_page.cc View 1 2 3 2 chunks +6 lines, -0 lines 0 comments Download
M android_webview/browser/aw_safe_browsing_ui_manager.h View 1 2 2 chunks +23 lines, -3 lines 0 comments Download
M android_webview/browser/aw_safe_browsing_ui_manager.cc View 1 2 3 4 5 2 chunks +58 lines, -1 line 0 comments Download
M android_webview/common/aw_paths.h View 1 chunk +3 lines, -0 lines 0 comments Download
M android_webview/common/aw_paths.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/safe_browsing/safe_browsing_service.cc View 1 chunk +0 lines, -23 lines 0 comments Download
M components/safe_browsing/common/safebrowsing_constants.h View 1 chunk +15 lines, -0 lines 0 comments Download
M components/safe_browsing/common/safebrowsing_constants.cc View 1 chunk +19 lines, -0 lines 0 comments Download

Messages

Total messages: 43 (24 generated)
timvolodine
still a TODO left, but thought I could get some feedback already on the current ...
3 years, 6 months ago (2017-06-08 16:30:38 UTC) #5
Jialiu Lin
+lpz@, since we recently added TriggerManager (already in component) to handle the triggering of ThreatDetails ...
3 years, 6 months ago (2017-06-08 18:19:39 UTC) #11
sgurun-gerrit only
On 2017/06/08 18:19:39, Jialiu Lin wrote: > +lpz@, since we recently added TriggerManager (already in ...
3 years, 6 months ago (2017-06-08 21:53:36 UTC) #13
Nate Fischer
Initial changes LGTM % comments https://codereview.chromium.org/2932703003/diff/20001/android_webview/browser/aw_safe_browsing_ui_manager.cc File android_webview/browser/aw_safe_browsing_ui_manager.cc (right): https://codereview.chromium.org/2932703003/diff/20001/android_webview/browser/aw_safe_browsing_ui_manager.cc#newcode30 android_webview/browser/aw_safe_browsing_ui_manager.cc:30: client_name = "chromium"; Do ...
3 years, 6 months ago (2017-06-08 22:21:47 UTC) #14
timvolodine
+cc:tobiasjs@ : as the original author of aw_paths, in case there are any comments/thoughts regarding ...
3 years, 6 months ago (2017-06-09 11:52:10 UTC) #15
timvolodine
https://codereview.chromium.org/2932703003/diff/20001/android_webview/browser/aw_safe_browsing_ui_manager.cc File android_webview/browser/aw_safe_browsing_ui_manager.cc (right): https://codereview.chromium.org/2932703003/diff/20001/android_webview/browser/aw_safe_browsing_ui_manager.cc#newcode30 android_webview/browser/aw_safe_browsing_ui_manager.cc:30: client_name = "chromium"; On 2017/06/08 22:21:47, Nate Fischer wrote: ...
3 years, 6 months ago (2017-06-09 11:53:21 UTC) #16
lpz
lgtm. I left a similar comment in your design doc. Lets also maybe do a ...
3 years, 6 months ago (2017-06-09 14:54:51 UTC) #17
Tobias Sargeant
https://codereview.chromium.org/2932703003/diff/40001/android_webview/common/aw_paths.cc File android_webview/common/aw_paths.cc (right): https://codereview.chromium.org/2932703003/diff/40001/android_webview/common/aw_paths.cc#newcode28 android_webview/common/aw_paths.cc:28: .Append(FILE_PATH_LITERAL("SafeBrowsing")); is that directory going to get created by ...
3 years, 6 months ago (2017-06-09 14:57:02 UTC) #19
selim
On 2017/06/09 14:57:02, Tobias Sargeant wrote: > https://codereview.chromium.org/2932703003/diff/40001/android_webview/common/aw_paths.cc > File android_webview/common/aw_paths.cc (right): > > https://codereview.chromium.org/2932703003/diff/40001/android_webview/common/aw_paths.cc#newcode28 ...
3 years, 6 months ago (2017-06-09 15:21:27 UTC) #20
Jialiu Lin
LGTM.
3 years, 6 months ago (2017-06-09 16:25:33 UTC) #22
timvolodine
Thanks for the comments everybody, - added trigger_manager to AwBrowserContext - filed two blocking bugs ...
3 years, 6 months ago (2017-06-09 16:28:43 UTC) #24
timvolodine
tobiasjs@: added directory creation (see below), ptal. https://codereview.chromium.org/2932703003/diff/40001/android_webview/common/aw_paths.cc File android_webview/common/aw_paths.cc (right): https://codereview.chromium.org/2932703003/diff/40001/android_webview/common/aw_paths.cc#newcode28 android_webview/common/aw_paths.cc:28: .Append(FILE_PATH_LITERAL("SafeBrowsing")); On ...
3 years, 6 months ago (2017-06-09 17:48:49 UTC) #27
timvolodine
On 2017/06/09 14:54:51, lpz wrote: > lgtm. I left a similar comment in your design ...
3 years, 6 months ago (2017-06-09 17:55:16 UTC) #28
sgurun-gerrit only
On 2017/06/09 17:55:16, timvolodine wrote: > On 2017/06/09 14:54:51, lpz wrote: > > lgtm. I ...
3 years, 6 months ago (2017-06-09 21:10:40 UTC) #32
Jialiu Lin
https://codereview.chromium.org/2932703003/diff/80001/android_webview/browser/aw_safe_browsing_ui_manager.cc File android_webview/browser/aw_safe_browsing_ui_manager.cc (right): https://codereview.chromium.org/2932703003/diff/80001/android_webview/browser/aw_safe_browsing_ui_manager.cc#newcode26 android_webview/browser/aw_safe_browsing_ui_manager.cc:26: std::string client_name; Based on awoz@'s email, feel free to ...
3 years, 6 months ago (2017-06-12 17:05:40 UTC) #33
timvolodine
https://codereview.chromium.org/2932703003/diff/80001/android_webview/browser/aw_safe_browsing_ui_manager.cc File android_webview/browser/aw_safe_browsing_ui_manager.cc (right): https://codereview.chromium.org/2932703003/diff/80001/android_webview/browser/aw_safe_browsing_ui_manager.cc#newcode26 android_webview/browser/aw_safe_browsing_ui_manager.cc:26: std::string client_name; On 2017/06/12 17:05:40, Jialiu Lin wrote: > ...
3 years, 6 months ago (2017-06-14 17:56:06 UTC) #34
timvolodine
Thanks for the reviews!
3 years, 6 months ago (2017-06-14 18:02:36 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2932703003/100001
3 years, 6 months ago (2017-06-14 18:03:31 UTC) #40
commit-bot: I haz the power
3 years, 6 months ago (2017-06-14 19:41:45 UTC) #43
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/2ef5d746246fd46bccc0035070da...

Powered by Google App Engine
This is Rietveld 408576698