Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2889913002: [subresource_filter] Remove Forwarding NavigationThrottles (Closed)

Created:
6 months ago by Charlie Harrison
Modified:
6 months ago
CC:
chromium-reviews, darin-cc_chromium.org, jam, subresource-filter-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[subresource_filter] Remove Forwarding NavigationThrottles Now that all activation goes through the safe browsing throttle, we can just hook up everything there without doing any sort of persistence in the driver factory. BUG=708181 Review-Url: https://codereview.chromium.org/2889913002 Cr-Commit-Position: refs/heads/master@{#473082} Committed: https://chromium.googlesource.com/chromium/src/+/991638fa4c75139e0ce663e27c56299e9849faae

Patch Set 1 #

Patch Set 2 : Remove silly forwarding throttles #

Total comments: 6

Patch Set 3 : creis review #

Patch Set 4 : shivanisha review #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+104 lines, -199 lines) Patch
M components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h View 1 2 3 7 chunks +13 lines, -27 lines 0 comments Download
M components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc View 1 2 3 8 chunks +58 lines, -87 lines 0 comments Download
M components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc View 1 2 3 1 chunk +9 lines, -11 lines 0 comments Download
M components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h View 1 1 chunk +0 lines, -4 lines 0 comments Download
M components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc View 1 2 chunks +0 lines, -35 lines 0 comments Download
M components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc View 1 2 3 2 chunks +13 lines, -8 lines 1 comment Download
M components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc View 1 2 2 chunks +0 lines, -27 lines 0 comments Download
M content/public/test/navigation_simulator.h View 1 2 2 chunks +6 lines, -0 lines 0 comments Download
M content/public/test/navigation_simulator.cc View 1 2 1 chunk +5 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 30 (17 generated)
Charlie Harrison
shivanisha, more refactoring :D PTAL? Let me know if you're getting overloaded and I can ...
6 months ago (2017-05-17 02:13:41 UTC) #5
Charlie Reis
content/ LGTM with nits. https://codereview.chromium.org/2889913002/diff/20001/content/public/test/navigation_simulator.cc File content/public/test/navigation_simulator.cc (right): https://codereview.chromium.org/2889913002/diff/20001/content/public/test/navigation_simulator.cc#newcode487 content/public/test/navigation_simulator.cc:487: CHECK_EQ(state_, STARTED); nit: Reverse order, ...
6 months ago (2017-05-17 22:39:24 UTC) #8
Charlie Harrison
Thanks Charlie https://codereview.chromium.org/2889913002/diff/20001/content/public/test/navigation_simulator.cc File content/public/test/navigation_simulator.cc (right): https://codereview.chromium.org/2889913002/diff/20001/content/public/test/navigation_simulator.cc#newcode487 content/public/test/navigation_simulator.cc:487: CHECK_EQ(state_, STARTED); On 2017/05/17 22:39:24, Charlie Reis ...
6 months ago (2017-05-18 19:14:05 UTC) #10
shivanisha
lgtm % nit https://codereview.chromium.org/2889913002/diff/20001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2889913002/diff/20001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h#newcode90 components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:90: // and |redirects| are saved. Need ...
6 months ago (2017-05-18 19:54:21 UTC) #12
Charlie Harrison
https://codereview.chromium.org/2889913002/diff/20001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2889913002/diff/20001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h#newcode90 components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:90: // and |redirects| are saved. On 2017/05/18 19:54:21, shivanisha ...
6 months ago (2017-05-18 20:22:41 UTC) #14
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/2889913002/60001
6 months ago (2017-05-18 20:24:51 UTC) #19
commit-bot: I haz the power
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_ng/builds/449004)
6 months ago (2017-05-19 00:36:36 UTC) #21
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/2889913002/60001
6 months ago (2017-05-19 00:55:49 UTC) #23
commit-bot: I haz the power
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/991638fa4c75139e0ce663e27c56299e9849faae
6 months ago (2017-05-19 04:13:45 UTC) #26
Marc Treib
On 2017/05/19 04:13:45, commit-bot: I haz the power wrote: > Committed patchset #4 (id:60001) as ...
6 months ago (2017-05-19 10:10:43 UTC) #27
Marc Treib
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2897613002/ by treib@chromium.org. ...
6 months ago (2017-05-19 10:11:33 UTC) #28
Charlie Harrison
https://codereview.chromium.org/2889913002/diff/60001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2889913002/diff/60001/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc#newcode137 components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:137: check_results_.back(); Here's the issue, we call back() on an ...
6 months ago (2017-05-19 11:15:41 UTC) #29
findit-for-me
6 months ago (2017-05-19 12:47:27 UTC) #30
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 473082 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

Powered by Google App Engine
This is Rietveld efc10ee0f