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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by Charlie Harrison
Modified:
2 months, 1 week 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
Commit queue not available (can’t edit this change).

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 ...
2 months, 1 week ago (2017-05-17 02:13:41 UTC) #5
Charlie Reis (slow)
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, ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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
2 months, 1 week 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)
2 months, 1 week 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
2 months, 1 week 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
2 months, 1 week 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 ...
2 months, 1 week 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. ...
2 months, 1 week 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 ...
2 months, 1 week ago (2017-05-19 11:15:41 UTC) #29
findit-for-me
2 months, 1 week 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973