Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2645283007:
Add the client for accessing Subresource Filter only list. (Closed)
|
Created:
3 years, 11 months ago by melandory Modified:
3 years, 8 months ago CC:
chromium-reviews, vakh+watch_chromium.org, droger+watchlist_chromium.org, grt+watch_chromium.org, sdefresne+watchlist_chromium.org, jam, Randy Smith (Not in Mondays), darin-cc_chromium.org, asvitkine+watch_chromium.org, blundell+watchlist_chromium.org, loading-reviews_chromium.org, mmenke Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement the SubresourceFilterActivationNavigationThrottle for accessing the Subresource Filter list.
In this Cl NavigationThrottle acts only on WillProcessResonce signal.
This is temporal behavior and will changed in followup CLs.
Part 1, https://codereview.chromium.org/2690413002
Part 2, https://codereview.chromium.org/2645283007 (this one)
Part 3, https://codereview.chromium.org/2770153004
BUG=671962
Review-Url: https://codereview.chromium.org/2645283007
Cr-Commit-Position: refs/heads/master@{#460476}
Committed: https://chromium.googlesource.com/chromium/src/+/2cf99830cfbe5d462b161089bb159ac9f29a620c
Patch Set 1 #
Total comments: 8
Patch Set 2 : fix unittest #
Total comments: 6
Patch Set 3 : cleanup #
Total comments: 1
Patch Set 4 : . #
Total comments: 12
Patch Set 5 : guard in unittestst #Patch Set 6 : . #Patch Set 7 : fix HistoryNavTest #
Total comments: 8
Patch Set 8 : . #
Total comments: 9
Patch Set 9 : adressed comments from jwd #
Total comments: 2
Patch Set 10 : adressed comments from jwd #
Total comments: 39
Patch Set 11 : full changes and engedy@ comments #Patch Set 12 : only changes specific to this cl #
Total comments: 43
Patch Set 13 : comments #Patch Set 14 : feature guard #Patch Set 15 : guard creation of nav. throttle with feature #Patch Set 16 : rebased #Patch Set 17 : feature #Messages
Total messages: 193 (161 generated)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Add the client for accessing Subresource Filter only list. BUG=671962 ========== to ========== Implement the SubresourceFilterActivationNavigationThrottle for accessing the Subresource Filter list. BUG=671962 ==========
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
melandory@chromium.org changed reviewers: + vakh@chromium.org
Hi Varun, could you please review the whole CL, but especially navigation throttle and ugly hack I made in order to work with v4 database in the tests. Thanks a lot!
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
https://codereview.chromium.org/2645283007/diff/100001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2645283007/diff/100001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:1244: "subresource_filter/subresource_filter_navigation_trottle_util.cc", nit: since it is already in the subresource_fiter directory, why not call it: navigation_throttle_util also: typo: throttle. https://codereview.chromium.org/2645283007/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/test_safe_browsing_service.cc (right): https://codereview.chromium.org/2645283007/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/test_safe_browsing_service.cc:13: #include "components/safe_browsing_db/v4_feature_list.h" #include not needed? https://codereview.chromium.org/2645283007/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/test_safe_browsing_service.cc:61: return database_manager_; this looks like a hack we should avoid. https://codereview.chromium.org/2645283007/diff/100001/components/subresource... File components/subresource_filter/core/common/activation_list.h (right): https://codereview.chromium.org/2645283007/diff/100001/components/subresource... components/subresource_filter/core/common/activation_list.h:17: LAST = SUBRESOURCE_FILTER_ONLY, qq: I may be misinformed, apologies: didn't we decide to call the list "subresource filtering" and drop the "only"? https://codereview.chromium.org/2645283007/diff/180001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2645283007/diff/180001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:94: const char kMatchesPatternHistogramName[] = Didn't look closely at the changes in this file. You might want to get it reviewed by someone who understands the SubResource filter details better. https://codereview.chromium.org/2645283007/diff/180001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_navigation_trottle_util.cc (right): https://codereview.chromium.org/2645283007/diff/180001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_navigation_trottle_util.cc:19: (safe_browsing::V4FeatureList::IsV4HybridEnabled() || these methods are going away soon so don't be surprised if your build breaks after a rebase. See: https://codereview.chromium.org/2675063002/#ps160001 https://codereview.chromium.org/2645283007/diff/180001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_navigation_trottle_util.h (right): https://codereview.chromium.org/2645283007/diff/180001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_navigation_trottle_util.h:5: #ifndef CHROME_BROWSER_SUBRESOURCE_FILTER_SUBRESOURCE_FILTER_NAVIGATION_TROTTLE_UTIL_H_ typo: throttle https://codereview.chromium.org/2645283007/diff/180001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.cc (right): https://codereview.chromium.org/2645283007/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.cc:102: OnCheckBrowseUrlResult(GURL::EmptyGURL(), safe_browsing::SB_THREAT_TYPE_SAFE, This should be the url for which the request timed out.
Description was changed from ========== Implement the SubresourceFilterActivationNavigationThrottle for accessing the Subresource Filter list. BUG=671962 ========== to ========== Implement the SubresourceFilterActivationNavigationThrottle for accessing the Subresource Filter list. In this Cl NavigationThrottle acts only on WillProcessResonce signal. This is temporal behavior and will changed in followup CLs. BUG=671962 ==========
Patchset #7 (id:200001) has been deleted
https://codereview.chromium.org/2645283007/diff/100001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2645283007/diff/100001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:1244: "subresource_filter/subresource_filter_navigation_trottle_util.cc", On 2017/02/10 03:00:11, vakh (Varun Khaneja) wrote: > nit: since it is already in the subresource_fiter directory, why not call it: > navigation_throttle_util > > also: typo: throttle. Done. https://codereview.chromium.org/2645283007/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/test_safe_browsing_service.cc (right): https://codereview.chromium.org/2645283007/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/test_safe_browsing_service.cc:13: #include "components/safe_browsing_db/v4_feature_list.h" On 2017/02/10 03:00:11, vakh (Varun Khaneja) wrote: > #include not needed? Done. https://codereview.chromium.org/2645283007/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/test_safe_browsing_service.cc:61: return database_manager_; On 2017/02/10 03:00:12, vakh (Varun Khaneja) wrote: > this looks like a hack we should avoid. Yep, I'll piggy back on changes in your CL 2675063002. WDYT, about extracting Fake V4 databases and factories you've introduced in CL 2675063002 in separate file, so it's easy to reuse them? Here I've drafted the CL: https://codereview.chromium.org/2690413002 And in the last patchset of this CL you can see how it will be used. https://codereview.chromium.org/2645283007/diff/100001/components/subresource... File components/subresource_filter/core/common/activation_list.h (right): https://codereview.chromium.org/2645283007/diff/100001/components/subresource... components/subresource_filter/core/common/activation_list.h:17: LAST = SUBRESOURCE_FILTER_ONLY, On 2017/02/10 03:00:12, vakh (Varun Khaneja) wrote: > qq: I may be misinformed, apologies: didn't we decide to call the list > "subresource filtering" and drop the "only"? yep, sorry. Fixed. https://codereview.chromium.org/2645283007/diff/180001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_navigation_trottle_util.cc (right): https://codereview.chromium.org/2645283007/diff/180001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_navigation_trottle_util.cc:19: (safe_browsing::V4FeatureList::IsV4HybridEnabled() || On 2017/02/10 03:00:12, vakh (Varun Khaneja) wrote: > these methods are going away soon so don't be surprised if your build breaks > after a rebase. > See: https://codereview.chromium.org/2675063002/#ps160001 Rebased on top of it. https://codereview.chromium.org/2645283007/diff/180001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.cc (right): https://codereview.chromium.org/2645283007/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.cc:102: OnCheckBrowseUrlResult(GURL::EmptyGURL(), safe_browsing::SB_THREAT_TYPE_SAFE, On 2017/02/10 03:00:12, vakh (Varun Khaneja) wrote: > This should be the url for which the request timed out. Done.
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
Patchset #4 (id:220001) has been deleted
Description was changed from ========== Implement the SubresourceFilterActivationNavigationThrottle for accessing the Subresource Filter list. In this Cl NavigationThrottle acts only on WillProcessResonce signal. This is temporal behavior and will changed in followup CLs. BUG=671962 ========== to ========== Implement the SubresourceFilterActivationNavigationThrottle for accessing the Subresource Filter list. In this Cl NavigationThrottle acts only on WillProcessResonce signal. This is temporal behavior and will changed in followup CLs. Part 1, https://codereview.chromium.org/2690413002 Part 2, https://codereview.chromium.org/2645283007 (this one) BUG=671962 ==========
On 2017/02/14 17:40:39, melandory wrote: Please take another look. Thanks!
Patchset #5 (id:260001) has been deleted
Patchset #5 (id:280001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Patchset #6 (id:320001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
jwd@chromium.org: Please review change in histogram.xml
melandory@chromium.org changed reviewers: + engedy@chromium.org, jwd@chromium.org
engedy@: Please take a look at subresource_filter specific files.
Some nits, naming suggestions, documenting our offline discussions mostly for myself so that I will remember it. https://codereview.chromium.org/2645283007/diff/360001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.cc (right): https://codereview.chromium.org/2645283007/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.cc:60: // SafeBrowsingService::Client implementation, called on the IO thread once nit: I'd just remove this comment (sign) when we already have the DCHECK (cop). https://codereview.chromium.org/2645283007/diff/360001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.h (right): https://codereview.chromium.org/2645283007/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.h:13: #include "base/time/time.h" nit: Unused include. https://codereview.chromium.org/2645283007/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.h:22: class SubresourceFilterActivationNavigationThrottle For disambiguation from other throttles, we should call this either SafeBrowsingSubresourceFilterActivationThrottle or SubresourceFilterSafeBrowsingActivationThrottle. https://codereview.chromium.org/2645283007/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.h:35: content::NavigationThrottle::ThrottleCheckResult WillProcessResponse() nit: Add above this line: // content::NavigaionThrottle: https://codereview.chromium.org/2645283007/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.h:39: void OnCheckBrowseUrlResult( As discussed off-line, we will have to introduce an object to proxy these calls and receive the results from the SBDatabaseManager on the IO thread. The easiest seems to be make this object live on the IO thread with the DeleteSoon idiom, and make it PostTask to this throttle using a WeakPtr to it.
https://codereview.chromium.org/2645283007/diff/360001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2645283007/diff/360001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67155: + <suffix name="SubresourceFilter"/> I don't think this does anything. You need to have a <histogram_suffixes> tag in the histogram_suffixes_list. Documentation for this is at the top of the file.
https://codereview.chromium.org/2645283007/diff/300001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2645283007/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3303: MaybeCreateSubresourceFilterNavigationThrottle( nit: you don't need the temporary std::unique_ptr
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:380001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #8 (id:400001) has been deleted
Patchset #8 (id:420001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:440001) has been deleted
https://codereview.chromium.org/2645283007/diff/360001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.cc (right): https://codereview.chromium.org/2645283007/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.cc:60: // SafeBrowsingService::Client implementation, called on the IO thread once On 2017/02/17 20:50:52, engedy (slow) wrote: > nit: I'd just remove this comment (sign) when we already have the DCHECK (cop). Done. https://codereview.chromium.org/2645283007/diff/360001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.h (right): https://codereview.chromium.org/2645283007/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.h:13: #include "base/time/time.h" On 2017/02/17 20:50:52, engedy (slow) wrote: > nit: Unused include. Done. https://codereview.chromium.org/2645283007/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.h:22: class SubresourceFilterActivationNavigationThrottle On 2017/02/17 20:50:53, engedy (slow) wrote: > For disambiguation from other throttles, we should call this either > SafeBrowsingSubresourceFilterActivationThrottle or > SubresourceFilterSafeBrowsingActivationThrottle. Done. https://codereview.chromium.org/2645283007/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.h:35: content::NavigationThrottle::ThrottleCheckResult WillProcessResponse() On 2017/02/17 20:50:53, engedy (slow) wrote: > nit: Add above this line: > > // content::NavigaionThrottle: Done. https://codereview.chromium.org/2645283007/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_activation_navigation_throttle.h:39: void OnCheckBrowseUrlResult( On 2017/02/17 20:50:53, engedy (slow) wrote: > As discussed off-line, we will have to introduce an object to proxy these calls > and receive the results from the SBDatabaseManager on the IO thread. The easiest > seems to be make this object live on the IO thread with the DeleteSoon idiom, > and make it PostTask to this throttle using a WeakPtr to it. Done. https://codereview.chromium.org/2645283007/diff/360001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2645283007/diff/360001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67155: + <suffix name="SubresourceFilter"/> On 2017/02/17 22:40:32, jwd wrote: > I don't think this does anything. You need to have a <histogram_suffixes> tag in > the histogram_suffixes_list. Documentation for this is at the top of the file. Done.
Patchset #6 (id:340001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #8 (id:470001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Patchset #9 (id:510001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
friendly ping.
Looks good, please see initial comments, I'll do another more detailed pass. https://codereview.chromium.org/2645283007/diff/530001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2645283007/diff/530001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:338: DidURLMatchSubresourceFilterOnlyList(const GURL& url) const { Let's try to avoid duplicating the code above, and make the signature of this: `bool DidURLMatchActivationList(const GURL&, ActivationList)`. Then DidURLMatchCurrentActivationList could call into this with passing GetCurrentActivationList() as the second argument. This would also remove the need for the lambda below. https://codereview.chromium.org/2645283007/diff/530001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:353: void ContentSubresourceFilterDriverFactory::RecordRedirectChainMatchPattern() What do you think of recording two histograms here entirely independently of each other, and independently of the current activation list: SubresourceFilter.PageLoad.*.SocialEngineeringAdsInterstitial SubresourceFilter.PageLoad.*.SubresourceFilter[Only] https://codereview.chromium.org/2645283007/diff/530001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2645283007/diff/530001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:115: task_runner_->DeleteSoon(FROM_HERE, activation_client_.release()); nit: You get this for free with std::unique_ptr<T, base::OnTaskRunnerDeleter> https://codereview.chromium.org/2645283007/diff/530001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h (right): https://codereview.chromium.org/2645283007/diff/530001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h:33: // content::NavigaionThrottle: typo: Navigation
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #10 (id:550001) has been deleted
Patchset #10 (id:570001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:590001) has been deleted
https://codereview.chromium.org/2645283007/diff/530001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2645283007/diff/530001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:338: DidURLMatchSubresourceFilterOnlyList(const GURL& url) const { On 2017/03/02 12:00:42, engedy (slow) wrote: > Let's try to avoid duplicating the code above, and make the signature of this: > `bool DidURLMatchActivationList(const GURL&, ActivationList)`. > > Then DidURLMatchCurrentActivationList could call into this with passing > GetCurrentActivationList() as the second argument. > > This would also remove the need for the lambda below. Done. https://codereview.chromium.org/2645283007/diff/530001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:353: void ContentSubresourceFilterDriverFactory::RecordRedirectChainMatchPattern() On 2017/03/02 12:00:43, engedy (slow) wrote: > What do you think of recording two histograms here entirely independently of > each other, and independently of the current activation list: > > SubresourceFilter.PageLoad.*.SocialEngineeringAdsInterstitial > SubresourceFilter.PageLoad.*.SubresourceFilter[Only] I like the idea. A bit of refactoring will be involved, I think it's better to do separately, I've started WIP cL: https://codereview.chromium.org/2733833002 https://codereview.chromium.org/2645283007/diff/530001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2645283007/diff/530001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:115: task_runner_->DeleteSoon(FROM_HERE, activation_client_.release()); On 2017/03/02 12:00:43, engedy (slow) wrote: > nit: You get this for free with std::unique_ptr<T, base::OnTaskRunnerDeleter> Done. https://codereview.chromium.org/2645283007/diff/530001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h (right): https://codereview.chromium.org/2645283007/diff/530001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h:33: // content::NavigaionThrottle: On 2017/03/02 12:00:43, engedy (slow) wrote: > typo: Navigation Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2645283007/diff/610001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2645283007/diff/610001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67712: <summary>Total length of the server redirects during the navigation.</summary> Can you reword the summary, either to "...server redirect chain during..." or "Total number of server redirects..." https://codereview.chromium.org/2645283007/diff/610001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2645283007/diff/610001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67712: + <suffix name="SubresourceFilterOnly"/> Remove these suffix tags, only the histogram_suffixes is needed. https://codereview.chromium.org/2645283007/diff/610001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:120591: + name="SubresourceFilter.PageLoad.RedirectChainMatchPattern"/> Histogram suffixes have a hierarchical implication. Based on this, I'd assume that SubresourceFilter.PageLoad.RedirectChainLength has the length for all the cases, and SubresourceFilter.PageLoad.RedirectChainLength.SubresourceFilterOnly has the length for a subset. From the code, it looks like this isn't the case. I'd suggest creating a suffix for the non-SubresourceFilterOnly case, and either having the base histograms capture the total values, or not log to the base histogram anymore. If you want to keep it as is please add a comment to both the affected histograms making it clear that they are disjoint from the SubresourceFilterOnly case.
https://codereview.chromium.org/2645283007/diff/610001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2645283007/diff/610001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67712: <summary>Total length of the server redirects during the navigation.</summary> On 2017/03/07 22:31:17, jwd wrote: > Can you reword the summary, either to "...server redirect chain during..." or > "Total number of server redirects..." Done. https://codereview.chromium.org/2645283007/diff/610001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2645283007/diff/610001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67712: + <suffix name="SubresourceFilterOnly"/> On 2017/03/07 22:31:17, jwd wrote: > Remove these suffix tags, only the histogram_suffixes is needed. Done. https://codereview.chromium.org/2645283007/diff/610001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:120591: + name="SubresourceFilter.PageLoad.RedirectChainMatchPattern"/> On 2017/03/07 22:31:17, jwd wrote: > Histogram suffixes have a hierarchical implication. Based on this, I'd assume > that SubresourceFilter.PageLoad.RedirectChainLength has the length for all the > cases, and SubresourceFilter.PageLoad.RedirectChainLength.SubresourceFilterOnly > has the length for a subset. From the code, it looks like this isn't the case. > I'd suggest creating a suffix for the non-SubresourceFilterOnly case, and either > having the base histograms capture the total values, or not log to the base > histogram anymore. > > If you want to keep it as is please add a comment to both the affected > histograms making it clear that they are disjoint from the SubresourceFilterOnly > case. I'm implementing suffixes in followup CL: https://codereview.chromium.org/2733833002/
https://codereview.chromium.org/2645283007/diff/610001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2645283007/diff/610001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:120591: + name="SubresourceFilter.PageLoad.RedirectChainMatchPattern"/> On 2017/03/08 19:37:25, melandory wrote: > On 2017/03/07 22:31:17, jwd wrote: > > Histogram suffixes have a hierarchical implication. Based on this, I'd assume > > that SubresourceFilter.PageLoad.RedirectChainLength has the length for all the > > cases, and > SubresourceFilter.PageLoad.RedirectChainLength.SubresourceFilterOnly > > has the length for a subset. From the code, it looks like this isn't the case. > > I'd suggest creating a suffix for the non-SubresourceFilterOnly case, and > either > > having the base histograms capture the total values, or not log to the base > > histogram anymore. > > > > If you want to keep it as is please add a comment to both the affected > > histograms making it clear that they are disjoint from the > SubresourceFilterOnly > > case. > I'm implementing suffixes in followup CL: > https://codereview.chromium.org/2733833002/ Yes, but that CL appears to have the same problem. My understanding of that code is that you're using the base histograms (SubresourceFilter.PageLoad.RedirectChainLength, SubresourceFilter.PageLoad.RedirectChainMatchPattern) as a fallback for anything that doesn't fit into your other cases. I don't consider this an intuitive use. If you want to have a fallback for the non-specified cases, you should should create a suffix for that purpose. Let me know if I'm interpreting the code wrong though! https://codereview.chromium.org/2645283007/diff/630001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2645283007/diff/630001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67713: + Total number of server redirects during the navigation. This histogram Nit: whitespace. Was it added by our pretty print tool?
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
https://codereview.chromium.org/2645283007/diff/610001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2645283007/diff/610001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:120591: + name="SubresourceFilter.PageLoad.RedirectChainMatchPattern"/> On 2017/03/08 22:59:26, jwd wrote: > On 2017/03/08 19:37:25, melandory wrote: > > On 2017/03/07 22:31:17, jwd wrote: > > > Histogram suffixes have a hierarchical implication. Based on this, I'd > assume > > > that SubresourceFilter.PageLoad.RedirectChainLength has the length for all > the > > > cases, and > > SubresourceFilter.PageLoad.RedirectChainLength.SubresourceFilterOnly > > > has the length for a subset. From the code, it looks like this isn't the > case. > > > I'd suggest creating a suffix for the non-SubresourceFilterOnly case, and > > either > > > having the base histograms capture the total values, or not log to the base > > > histogram anymore. > > > > > > If you want to keep it as is please add a comment to both the affected > > > histograms making it clear that they are disjoint from the > > SubresourceFilterOnly > > > case. > > I'm implementing suffixes in followup CL: > > https://codereview.chromium.org/2733833002/ > > Yes, but that CL appears to have the same problem. My understanding of that code > is that you're using the base histograms > (SubresourceFilter.PageLoad.RedirectChainLength, > SubresourceFilter.PageLoad.RedirectChainMatchPattern) as a fallback for anything > that doesn't fit into your other cases. I don't consider this an intuitive use. > If you want to have a fallback for the non-specified cases, you should should > create a suffix for that purpose. It's true for this CL that 2 types of histogram will be recorded: with suffix and without. But for the followup CL, the histogram without suffix should never be recorded. But I can add a suffix for no suffix case which should not be recorded if you prefer. > > Let me know if I'm interpreting the code wrong though! https://codereview.chromium.org/2645283007/diff/630001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2645283007/diff/630001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67713: + Total number of server redirects during the navigation. This histogram On 2017/03/08 22:59:26, jwd wrote: > Nit: whitespace. Was it added by our pretty print tool? Yep. It was new line before.
vakh@ and engedy@: can you take another look, please.
Could we replace this on top of https://codereview.chromium.org/2733833002? I think that would make everything much cleaner. https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3420: content::NavigationThrottle* subresource_filter_activation_throttle = nit: Let's make this return a unique_ptr in the first place (it is fine to return a null unique_ptr). https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... File chrome/browser/subresource_filter/navigation_throttle_util.h (right): https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... chrome/browser/subresource_filter/navigation_throttle_util.h:12: } style nit: // namespace safe_browsing https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... chrome/browser/subresource_filter/navigation_throttle_util.h:15: content::NavigationHandle* handle, nit: s/handle/navigation_handle/, s/sb/safe_browsing/, also in the .cc file https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:104: // Names of navigation chain patterns diagram. nit: ... histograms. https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:203: std::string(kSafeBrowsingSubresourceFilter.name) + nit: Let's make this nicer by either appending two separate switches (if possible), or using base::JoinString({..}, ','); https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:214: store_factory_ = new safe_browsing::TestV4StoreFactory(); nit: Not used outside of this function. https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:259: void MarkUrlForListIdUnexpired( nit: Add comment for what `unexpired` means, or name this something like MarkUrlAsMatchingListWithId. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:7: #include "base/memory/ptr_util.h" nit: I think this is not used. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:27: class SubresourceFilterSafeBrowsingActivationThrottle::ActivationClient nit: Given this is the client of the SB database, how about calling the class SBDatabaseClient, its instance |database_client|? https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:34: scoped_refptr<base::SingleThreadTaskRunner> task_runner) nit: s//callback_task_runner/g Alternatively: use content::BrowserThread::PostTask(content::BrowserThread::UI, ...) directly, if it does not make testing really hard. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:42: DCHECK(thread_checker_.CalledOnValidThread()); Need to call CancelCheck here as well in case we get destroyed while both the SB check and the the timer is still running. Not sure if this could happen, I can image it could if the WebContents is closed while we are DERER'ing. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:42: DCHECK(thread_checker_.CalledOnValidThread()); Given that this class not only needs to be called from one thread, but that thread has to be the IO thread, let's replace the ThreadChecker with: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:46: DCHECK(thread_checker_.CalledOnValidThread()); nit: Add DCHECK(url_being_checked.is_empty()); to declare no re-entrancy. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:47: if (database_manager_->CheckUrlForSubresourceFilter(url, this)) { The comment above this method is a little unclear, but it seems like we need to handle both true/false return values here? https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:49: task_runner_->PostTask( nit: Could we just call into OnCheckBrowserUrlResult from here? Stopping a non-started timer is defined as a no-op. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:89: // Timer to abort the safe browsing check if it takes too long. nit: blank line above comment https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:120: task_runner_->PostTask( nit: Could post directly to content::BrowserThread::PostTask(content::BrowserThread::IO, ...) if does not make testing hard. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h (right): https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h:14: #include "components/safe_browsing_db/database_manager.h" nit: This include could be avoided if we fwd-declared the DatabaseManager and #include'd "base/memory/ref_counted.h" and "components/safe_browsing_db/util.h" instead. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h:28: const scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>& style nit: According to [1], the new preferred way is now to pass in a scoped_refptr by value, and std::move it into the member variable on the initializer list. 1: https://www.chromium.org/developers/smart-pointer-guidelines
Histograms LGTM. Thanks for the changes! https://codereview.chromium.org/2645283007/diff/610001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2645283007/diff/610001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:120591: + name="SubresourceFilter.PageLoad.RedirectChainMatchPattern"/> On 2017/03/09 15:48:45, melandory wrote: > On 2017/03/08 22:59:26, jwd wrote: > > On 2017/03/08 19:37:25, melandory wrote: > > > On 2017/03/07 22:31:17, jwd wrote: > > > > Histogram suffixes have a hierarchical implication. Based on this, I'd > > assume > > > > that SubresourceFilter.PageLoad.RedirectChainLength has the length for all > > the > > > > cases, and > > > SubresourceFilter.PageLoad.RedirectChainLength.SubresourceFilterOnly > > > > has the length for a subset. From the code, it looks like this isn't the > > case. > > > > I'd suggest creating a suffix for the non-SubresourceFilterOnly case, and > > > either > > > > having the base histograms capture the total values, or not log to the > base > > > > histogram anymore. > > > > > > > > If you want to keep it as is please add a comment to both the affected > > > > histograms making it clear that they are disjoint from the > > > SubresourceFilterOnly > > > > case. > > > I'm implementing suffixes in followup CL: > > > https://codereview.chromium.org/2733833002/ > > > > Yes, but that CL appears to have the same problem. My understanding of that > code > > is that you're using the base histograms > > (SubresourceFilter.PageLoad.RedirectChainLength, > > SubresourceFilter.PageLoad.RedirectChainMatchPattern) as a fallback for > anything > > that doesn't fit into your other cases. I don't consider this an intuitive > use. > > If you want to have a fallback for the non-specified cases, you should should > > create a suffix for that purpose. > It's true for this CL that 2 types of histogram will be recorded: with suffix > and without. > But for the followup CL, the histogram without suffix should never be recorded. > But I can add a suffix for no suffix case which should not be recorded if you > prefer. > > > > Let me know if I'm interpreting the code wrong though! > Ah ok, I didn't see that it wouldn't be recorded in the follow up cl, it's clearer now thanks! It's fine how it is then.
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #13 (id:670001) has been deleted
Patchset #13 (id:690001) has been deleted
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3420: content::NavigationThrottle* subresource_filter_activation_throttle = On 2017/03/10 14:39:32, engedy wrote: > nit: Let's make this return a unique_ptr in the first place (it is fine to > return a null unique_ptr). Look at pachset 5 https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... File chrome/browser/subresource_filter/navigation_throttle_util.h (right): https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... chrome/browser/subresource_filter/navigation_throttle_util.h:12: } On 2017/03/10 14:39:32, engedy wrote: > style nit: // namespace safe_browsing I think the agreement is that when it's one line, then // namespace safe_browsing is optional. And I on the side of the fence which proposes to omit it for 1 line forward declarations. https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... chrome/browser/subresource_filter/navigation_throttle_util.h:15: content::NavigationHandle* handle, On 2017/03/10 14:39:32, engedy wrote: > nit: s/handle/navigation_handle/, s/sb/safe_browsing/, also in the .cc file Done. https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:104: // Names of navigation chain patterns diagram. On 2017/03/10 14:39:32, engedy wrote: > nit: ... histograms. Done. https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:203: std::string(kSafeBrowsingSubresourceFilter.name) + On 2017/03/10 14:39:32, engedy wrote: > nit: Let's make this nicer by either appending two separate switches (if > possible), or using base::JoinString({..}, ','); Done. https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:214: store_factory_ = new safe_browsing::TestV4StoreFactory(); On 2017/03/10 14:39:32, engedy wrote: > nit: Not used outside of this function. Done. https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:259: void MarkUrlForListIdUnexpired( On 2017/03/10 14:39:32, engedy wrote: > nit: Add comment for what `unexpired` means, or name this something like > MarkUrlAsMatchingListWithId. Done. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:7: #include "base/memory/ptr_util.h" On 2017/03/10 14:39:32, engedy wrote: > nit: I think this is not used. Done. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:27: class SubresourceFilterSafeBrowsingActivationThrottle::ActivationClient On 2017/03/10 14:39:32, engedy wrote: > nit: Given this is the client of the SB database, how about calling the class > SBDatabaseClient, its instance |database_client|? Done. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:34: scoped_refptr<base::SingleThreadTaskRunner> task_runner) On 2017/03/10 14:39:32, engedy wrote: > nit: s//callback_task_runner/g > > Alternatively: use content::BrowserThread::PostTask(content::BrowserThread::UI, > ...) directly, if it does not make testing really hard. Done. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:42: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/03/10 14:39:32, engedy wrote: > Given that this class not only needs to be called from one thread, but that > thread has to be the IO thread, let's replace the ThreadChecker with: > > DCHECK_CURRENTLY_ON(content::BrowserThread::IO); Done. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:42: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/03/10 14:39:32, engedy wrote: > Need to call CancelCheck here as well in case we get destroyed while both the SB > check and the the timer is still running. Not sure if this could happen, I can > image it could if the WebContents is closed while we are DERER'ing. Done. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:46: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/03/10 14:39:32, engedy wrote: > nit: Add DCHECK(url_being_checked.is_empty()); to declare no re-entrancy. Done. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:47: if (database_manager_->CheckUrlForSubresourceFilter(url, this)) { On 2017/03/10 14:39:32, engedy wrote: > The comment above this method is a little unclear, but it seems like we need to > handle both true/false return values here? Yep. It actually should be other way around. Unit test are coming (but separate patch and they actually caught that) https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:49: task_runner_->PostTask( On 2017/03/10 14:39:32, engedy wrote: > nit: Could we just call into OnCheckBrowserUrlResult from here? Stopping a > non-started timer is defined as a no-op. Done. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:89: // Timer to abort the safe browsing check if it takes too long. On 2017/03/10 14:39:32, engedy wrote: > nit: blank line above comment Done. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:120: task_runner_->PostTask( On 2017/03/10 14:39:32, engedy wrote: > nit: Could post directly to > content::BrowserThread::PostTask(content::BrowserThread::IO, ...) if does not > make testing hard. I like to keep task_runner if you don't have strong feeling. https://codereview.chromium.org/2645283007/diff/650001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h (right): https://codereview.chromium.org/2645283007/diff/650001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h:14: #include "components/safe_browsing_db/database_manager.h" On 2017/03/10 14:39:33, engedy wrote: > nit: This include could be avoided if we fwd-declared the DatabaseManager and > #include'd "base/memory/ref_counted.h" and "components/safe_browsing_db/util.h" > instead. For some weird reason it doesn't work
lgtm with nits As with another file renamed previously, please consider: components/subresource_filter/content/browser/safe_browsing_activation_throttle.* over components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.* https://codereview.chromium.org/2645283007/diff/730001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:405: REPORT_REDIRECT_PATTERN_FOR_SUFFIX("SubresourceFilterOnly", hits_pattern, s/Only// ? https://codereview.chromium.org/2645283007/diff/730001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:47: DCHECK(!url_being_checked_.is_empty()); This CHECK should probably be at the beginning of the function for url https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:62: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); Optional: I would encourage you to add the check/dcheck: CHECK_EQ (url_being_checked_, url)
LGTM % comments, thanks a lot for rebasing on the refactor, it makes the change very clean! https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3420: content::NavigationThrottle* subresource_filter_activation_throttle = On 2017/03/15 13:41:35, melandory wrote: > On 2017/03/10 14:39:32, engedy wrote: > > nit: Let's make this return a unique_ptr in the first place (it is fine to > > return a null unique_ptr). > > Look at pachset 5 Ping on this. https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... File chrome/browser/subresource_filter/navigation_throttle_util.h (right): https://codereview.chromium.org/2645283007/diff/650001/chrome/browser/subreso... chrome/browser/subresource_filter/navigation_throttle_util.h:12: } On 2017/03/15 13:41:35, melandory wrote: > On 2017/03/10 14:39:32, engedy wrote: > > style nit: // namespace safe_browsing > > I think the agreement is that when it's one line, then // namespace > safe_browsing is optional. And I on the side of the fence which proposes to omit > it for 1 line forward declarations. Acknowledged. https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3422: if (subresource_filter_activation_throttle) nit: {} https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/subreso... File chrome/browser/subresource_filter/navigation_throttle_util.cc (right): https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/subreso... chrome/browser/subresource_filter/navigation_throttle_util.cc:16: safe_browsing_service->database_manager()->IsSupported() && nit: Let's #include "components/safe_browsing_db/database_manager.h" to be clean. https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/subreso... File chrome/browser/subresource_filter/navigation_throttle_util.h (right): https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/subreso... chrome/browser/subresource_filter/navigation_throttle_util.h:14: content::NavigationThrottle* MaybeCreateSubresourceFilterNavigationThrottle( nit: Mention in a comment that |safe_browsing_service| can be nullptr. https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:204: base::JoinString({std::string(kSafeBrowsingSubresourceFilter.name), nit: I think you can drop std::string, should work with literals. https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:1063: EXPECT_THAT(tester.GetAllSamples(std::string(kMatchesPatternHistogramName) + nit: Could you please add a comment to explain that this is not (yet) supported? https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:1073: } // namespace subresource_filter We should have at least one test that verifies end-to-end behavior on SUBRESOURCE_FILTER sites: a) when that list is enabled via variations parameters, verify that subresources end up getting filtered, b) when the list is not enabled, verify that subresources are not getting filtered. Given that the throttle goes live as of CL, it would be good to do it in this CL. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:257: const std::string& histogram_suffix, Note that this is not used. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:300: if (expected_pattern != EMPTY) { nit: If |suffix| is empty, can we make some strong assertions here such as calling HistogramTester::GetTotalCountsForPrefix to ensure that no samples are recorded into this histogram family? https://codereview.chromium.org/2645283007/diff/730001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:16: // Maximum time in milliseconds to wait for the safe browsing service to nit: %s/safe browsing/Safe Browsing/g https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:18: // aborted, and the URL will be treated as if it doesn't belong to the nit: s/doesn't/didn't/ https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:20: const int kCheckUrlTimeoutMs = 5000; nit: Apparently the fashionable way to write this is now: constexpr base::TimeDelta kCheckURLTimeout = base::TimeDelta::FromSeconds(5); https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:88: base::ThreadChecker thread_checker_; nit: No longer used. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:102: callback_task_runner_(content::BrowserThread::GetTaskRunnerForThread( nit: It's a bit confusing to name both task runners "callback". Let's s/callback/io|(database_)client/ this if you really want to keep this. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:138: navigation_handle()->Resume(); Please add TODO and file a bug on adding histogram to measure the delay introduced here. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h (right): https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h:8: #include <string> nit: Unused. nit: #include <memory> https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h:9: #include <vector> nit: Only used in .cc file. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h:20: // Client for accessing list with patterns to be used by the Subresource Filter. Phrasing suggestion: // Navigation throttle responsible for activating subresource filtering on page loads that match the SUBRESOURCE_FILTER Safe Browsing list. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h:28: scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> #include "base/memory/ref_counted.h" https://codereview.chromium.org/2645283007/diff/730001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2645283007/diff/730001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:68723: + and without the prefix for all other redirect chains. nit: Is this last line still up-to-date? I don't see any code paths recording values without a suffix. (Which seems the expected behavior.)
Patchset #4 (id:240001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #13 (id:750001) has been deleted
https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3422: if (subresource_filter_activation_throttle) On 2017/03/21 13:44:16, engedy wrote: > nit: {} Done. https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/subreso... File chrome/browser/subresource_filter/navigation_throttle_util.cc (right): https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/subreso... chrome/browser/subresource_filter/navigation_throttle_util.cc:16: safe_browsing_service->database_manager()->IsSupported() && On 2017/03/21 13:44:16, engedy wrote: > nit: Let's #include "components/safe_browsing_db/database_manager.h" to be > clean. Done. https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/subreso... File chrome/browser/subresource_filter/navigation_throttle_util.h (right): https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/subreso... chrome/browser/subresource_filter/navigation_throttle_util.h:14: content::NavigationThrottle* MaybeCreateSubresourceFilterNavigationThrottle( On 2017/03/21 13:44:16, engedy wrote: > nit: Mention in a comment that |safe_browsing_service| can be nullptr. Done. https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:204: base::JoinString({std::string(kSafeBrowsingSubresourceFilter.name), On 2017/03/21 13:44:16, engedy wrote: > nit: I think you can drop std::string, should work with literals. Done. https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:1063: EXPECT_THAT(tester.GetAllSamples(std::string(kMatchesPatternHistogramName) + On 2017/03/21 13:44:16, engedy wrote: > nit: Could you please add a comment to explain that this is not (yet) supported? You mean that the real list is empty? https://codereview.chromium.org/2645283007/diff/730001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:1073: } // namespace subresource_filter On 2017/03/21 13:44:16, engedy (out of office) wrote: > We should have at least one test that verifies end-to-end behavior on > SUBRESOURCE_FILTER sites: > a) when that list is enabled via variations parameters, verify that > subresources end up getting filtered, > b) when the list is not enabled, verify that subresources are not getting > filtered. > > Given that the throttle goes live as of CL, it would be good to do it in this > CL. Done. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:405: REPORT_REDIRECT_PATTERN_FOR_SUFFIX("SubresourceFilterOnly", hits_pattern, On 2017/03/16 20:51:18, vakh (Varun Khaneja) wrote: > s/Only// ? I like "Only" in suffix, without it full histogram name sounds strange to me https://codereview.chromium.org/2645283007/diff/730001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:257: const std::string& histogram_suffix, On 2017/03/21 13:44:16, engedy wrote: > Note that this is not used. Done. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:300: if (expected_pattern != EMPTY) { On 2017/03/21 13:44:16, engedy wrote: > nit: If |suffix| is empty, can we make some strong assertions here such as > calling HistogramTester::GetTotalCountsForPrefix to ensure that no samples are > recorded into this histogram family? Done. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:16: // Maximum time in milliseconds to wait for the safe browsing service to On 2017/03/21 13:44:16, engedy wrote: > nit: %s/safe browsing/Safe Browsing/g Done. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:18: // aborted, and the URL will be treated as if it doesn't belong to the On 2017/03/21 13:44:16, engedy wrote: > nit: s/doesn't/didn't/ Done. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:20: const int kCheckUrlTimeoutMs = 5000; On 2017/03/21 13:44:16, engedy wrote: > nit: Apparently the fashionable way to write this is now: > > constexpr base::TimeDelta kCheckURLTimeout = base::TimeDelta::FromSeconds(5); Done. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:47: DCHECK(!url_being_checked_.is_empty()); On 2017/03/16 20:51:18, vakh (Varun Khaneja) wrote: > This CHECK should probably be at the beginning of the function for url Done. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:62: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); On 2017/03/16 20:51:18, vakh (Varun Khaneja) wrote: > Optional: I would encourage you to add the check/dcheck: CHECK_EQ > (url_being_checked_, url) Done. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:88: base::ThreadChecker thread_checker_; On 2017/03/21 13:44:16, engedy wrote: > nit: No longer used. Done. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:102: callback_task_runner_(content::BrowserThread::GetTaskRunnerForThread( On 2017/03/21 13:44:16, engedy wrote: > nit: It's a bit confusing to name both task runners "callback". Let's > s/callback/io|(database_)client/ this if you really want to keep this. Done. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:138: navigation_handle()->Resume(); On 2017/03/21 13:44:16, engedy wrote: > Please add TODO and file a bug on adding histogram to measure the delay > introduced here. Done. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h (right): https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h:8: #include <string> On 2017/03/21 13:44:16, engedy wrote: > nit: Unused. > nit: #include <memory> Done. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h:9: #include <vector> On 2017/03/21 13:44:16, engedy wrote: > nit: Only used in .cc file. Done. https://codereview.chromium.org/2645283007/diff/730001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h:28: scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> On 2017/03/21 13:44:16, engedy wrote: > #include "base/memory/ref_counted.h" Done. https://codereview.chromium.org/2645283007/diff/730001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2645283007/diff/730001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:68723: + and without the prefix for all other redirect chains. On 2017/03/21 13:44:16, engedy (out of office) wrote: > nit: Is this last line still up-to-date? I don't see any code paths recording > values without a suffix. (Which seems the expected behavior.) Done.
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #13 (id:770001) has been deleted
Patchset #13 (id:790001) has been deleted
Patchset #13 (id:810001) has been deleted
Patchset #13 (id:830001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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_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_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implement the SubresourceFilterActivationNavigationThrottle for accessing the Subresource Filter list. In this Cl NavigationThrottle acts only on WillProcessResonce signal. This is temporal behavior and will changed in followup CLs. Part 1, https://codereview.chromium.org/2690413002 Part 2, https://codereview.chromium.org/2645283007 (this one) BUG=671962 ========== to ========== Implement the SubresourceFilterActivationNavigationThrottle for accessing the Subresource Filter list. In this Cl NavigationThrottle acts only on WillProcessResonce signal. This is temporal behavior and will changed in followup CLs. Part 1, https://codereview.chromium.org/2690413002 Part 2, https://codereview.chromium.org/2645283007 (this one) Part 3, https://codereview.chromium.org/2770153004 BUG=671962 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Guarding creation of the nav throttle with the feature and landing (this way I will not wait until part 3 is ready)
Guarding creation of the nav throttle with the feature and landing (this way I will not wait until part 3 is ready)
Guarding creation of the nav throttle with the feature and landing (this way I will not wait until part 3 is ready)
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org, vakh@chromium.org, engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2645283007/#ps930001 (title: "feature")
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org, vakh@chromium.org, engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2645283007/#ps930001 (title: "feature")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 930001, "attempt_start_ts": 1490810308483080,
"parent_rev": "624dc15f7cb6b404e607f536aef0c18d325903ee", "commit_rev":
"2cf99830cfbe5d462b161089bb159ac9f29a620c"}
Message was sent while issue was closed.
Description was changed from ========== Implement the SubresourceFilterActivationNavigationThrottle for accessing the Subresource Filter list. In this Cl NavigationThrottle acts only on WillProcessResonce signal. This is temporal behavior and will changed in followup CLs. Part 1, https://codereview.chromium.org/2690413002 Part 2, https://codereview.chromium.org/2645283007 (this one) Part 3, https://codereview.chromium.org/2770153004 BUG=671962 ========== to ========== Implement the SubresourceFilterActivationNavigationThrottle for accessing the Subresource Filter list. In this Cl NavigationThrottle acts only on WillProcessResonce signal. This is temporal behavior and will changed in followup CLs. Part 1, https://codereview.chromium.org/2690413002 Part 2, https://codereview.chromium.org/2645283007 (this one) Part 3, https://codereview.chromium.org/2770153004 BUG=671962 Review-Url: https://codereview.chromium.org/2645283007 Cr-Commit-Position: refs/heads/master@{#460476} Committed: https://chromium.googlesource.com/chromium/src/+/2cf99830cfbe5d462b161089bb15... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:930001) as https://chromium.googlesource.com/chromium/src/+/2cf99830cfbe5d462b161089bb15... |
