Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2762403002:
Wire up the ThrottleManager using the existing page activation logic (Closed)
|
Created:
3 years, 9 months ago by Charlie Harrison Modified:
3 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, subresource-filter-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWire up the ThrottleManager using the existing page activation logic
BUG=637415
Review-Url: https://codereview.chromium.org/2762403002
Cr-Commit-Position: refs/heads/master@{#461754}
Committed: https://chromium.googlesource.com/chromium/src/+/e985ebc9f76dd65b59efaa730f018263b64c6c24
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : get rid of bad DCHECK #Patch Set 4 : mock activation throttlex #Patch Set 5 : rebase #Patch Set 6 : fix asan #Patch Set 7 : re-rebase #Patch Set 8 : add subframe filtering browser test #Patch Set 9 : rebase #Patch Set 10 : small tweaks #
Total comments: 19
Patch Set 11 : pkalinnikov review #
Total comments: 27
Patch Set 12 : engedy review #Messages
Total messages: 74 (53 generated)
The CQ bit was checked by csharrison@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 ========== Wire up the ThrottleManager using the existing PageActivation logic BUG=637415 ========== to ========== Wire up the ThrottleManager using the existing page activation logic BUG=637415 ==========
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 csharrison@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 csharrison@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_...)
The CQ bit was checked by csharrison@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...) 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 csharrison@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_asan_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 csharrison@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by csharrison@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 csharrison@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...
csharrison@chromium.org changed reviewers: + melandory@chromium.org
melandory: This is the my WIP CL for wiring up the various navigation throttles to browser side activation / filtering logic. I know you are working on the SB activation throttle work, but I was trying to come up with a design that would work in the meantime, so to unblock the OOPIF re-architecture. My solution was to add a "ForwardingNavigationThrottle", which just notifies the driver factory when WillProcessResponse comes. At this point, we do most of the logic in what is currently the driver factory's ReadyToCommitNavigation hook. My question: Do you think this design is OK in general? I am less familiar with SB code, but from taking a look it seems like we will always get the activation decision before NavigationThrottle::WillProcessResponse will be called. I also took a look at your SB throttle, but my understanding is there is still some work to be done even when that lands, because the driver factory still gets notified by the resource throttle (in addition to the navigation throttle).
On 2017/03/24 14:32:24, Charlie Harrison wrote: > melandory: This is the my WIP CL for wiring up the various navigation throttles > to browser side activation / filtering logic. > > I know you are working on the SB activation throttle work, but I was trying to > come up with a design that would work in the meantime, so to unblock the OOPIF > re-architecture. > > My solution was to add a "ForwardingNavigationThrottle", which just notifies the > driver factory when WillProcessResponse comes. At this point, we do most of the > logic in what is currently the driver factory's ReadyToCommitNavigation hook. > > My question: Do you think this design is OK in general? I am less familiar with > SB code, but from taking a look it seems like we will always get the activation > decision before NavigationThrottle::WillProcessResponse will be called. > > I also took a look at your SB throttle, but my understanding is there is still > some work to be done even when that lands, because the driver factory still gets > notified by the resource throttle (in addition to the navigation throttle). Yep once https://codereview.chromium.org/2645283007/ Cl is ready (and it's almost ready to land), we still need to move logic for Phishing in this navigation throttle.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Do you think this approach works ok in the meantime or should I just block on your CLs? Once this CL is in place I have one more to forward the correct ActivationState to the renderer, which should almost complete the browser side filtering refactor.
On 2017/03/24 14:32:24, Charlie Harrison wrote: > melandory: This is the my WIP CL for wiring up the various navigation throttles > to browser side activation / filtering logic. > > I know you are working on the SB activation throttle work, but I was trying to > come up with a design that would work in the meantime, so to unblock the OOPIF > re-architecture. > > My solution was to add a "ForwardingNavigationThrottle", which just notifies the > driver factory when WillProcessResponse comes. At this point, we do most of the > logic in what is currently the driver factory's ReadyToCommitNavigation hook. > > My question: Do you think this design is OK in general? I am less familiar with > SB code, but from taking a look it seems like we will always get the activation > decision before NavigationThrottle::WillProcessResponse will be called. It's true in the current version. The final implementation will initiate last check in NavigationThrottle::WillProcessResponse, because we care only about last URL in the redirect chain. > > I also took a look at your SB throttle, but my understanding is there is still > some work to be done even when that lands, because the driver factory still gets > notified by the resource throttle (in addition to the navigation throttle).
On 2017/03/24 14:32:24, Charlie Harrison wrote: > melandory: This is the my WIP CL for wiring up the various navigation throttles > to browser side activation / filtering logic. > > I know you are working on the SB activation throttle work, but I was trying to > come up with a design that would work in the meantime, so to unblock the OOPIF > re-architecture. > > My solution was to add a "ForwardingNavigationThrottle", which just notifies the > driver factory when WillProcessResponse comes. At this point, we do most of the > logic in what is currently the driver factory's ReadyToCommitNavigation hook. > > My question: Do you think this design is OK in general? I am less familiar with > SB code, but from taking a look it seems like we will always get the activation > decision before NavigationThrottle::WillProcessResponse will be called. It's true in the current version. The final implementation will initiate last check in NavigationThrottle::WillProcessResponse, because we care only about last URL in the redirect chain. > > I also took a look at your SB throttle, but my understanding is there is still > some work to be done even when that lands, because the driver factory still gets > notified by the resource throttle (in addition to the navigation throttle).
The CQ bit was checked by csharrison@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 csharrison@chromium.org to run a CQ dry run
Ok, I've rebased this and it should be ready for review now. PTAL?
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.
csharrison@chromium.org changed reviewers: + pkalinnikov@chromium.org
Hey Pavel, would you mind reviewing this one too? It is a bit subtle so it could use another set of eyes.
pkalinnikov@chromium.org changed reviewers: + engedy@chromium.org
LGTM % nits. +engedy@ to sanity check. https://codereview.chromium.org/2762403002/diff/170001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2762403002/diff/170001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3458: // These throttles must come after the SubresourceFilterNavigationThrottle. nit: Should this be SubresourceFilterSafeBrowsingActivationThrottle? I think we have quite misleading naming around it. The variable above (line 3450) and its type are named after "activation" but the function that creates it has a quite generic name. https://codereview.chromium.org/2762403002/diff/170001/chrome/browser/subreso... File chrome/browser/subresource_filter/chrome_subresource_filter_client.h (right): https://codereview.chromium.org/2762403002/diff/170001/chrome/browser/subreso... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:69: subresource_filter::VerifiedRulesetDealer::Handle* GetRulesetDealer() nit: #include the Handle's header. https://codereview.chromium.org/2762403002/diff/170001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2762403002/diff/170001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:210: GURL url = navigation_handle->GetURL(); nit: Seems like copying the |url| is not necessary here. Could you make it a const GURL& instead? https://codereview.chromium.org/2762403002/diff/170001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:233: state.measure_performance = measure_performance_; nit: Can you inline 230-231 to this assignment? https://codereview.chromium.org/2762403002/diff/170001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2762403002/diff/170001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:167: std::unique_ptr<ContentSubresourceFilterThrottleManager> throttle_manager_; nit: Please #include <memory> https://codereview.chromium.org/2762403002/diff/170001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2762403002/diff/170001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:49: const char kDisallowUrl[] = "https://example.com/disallowed.html"; nig: kDisallowedUrl https://codereview.chromium.org/2762403002/diff/170001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:353: void NavigateSubframeAndExpectFilter(const GURL& url, bool expect_filtered) { naming nits: - void NavigateSubframeAndExpectCheckResult - bool expect_cancelled https://codereview.chromium.org/2762403002/diff/170001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2762403002/diff/170001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:25: class ForwardingNavigationThrottle : public content::NavigationThrottle { nit: #include "content/public/browser/navigation_throttle.h" https://codereview.chromium.org/2762403002/diff/170001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h (right): https://codereview.chromium.org/2762403002/diff/170001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:101: bool OnMessageReceived(const IPC::Message& message, nit: Please forward declare IPC::Message explicitly.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Thanks Pavel. I've addressed all but one of your concerns. https://codereview.chromium.org/2762403002/diff/170001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2762403002/diff/170001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3458: // These throttles must come after the SubresourceFilterNavigationThrottle. On 2017/04/03 13:40:07, pkalinnikov wrote: > nit: Should this be SubresourceFilterSafeBrowsingActivationThrottle? I think we > have quite misleading naming around it. The variable above (line 3450) and its > type are named after "activation" but the function that creates it has a quite > generic name. Yes, done. Let me fix the naming of the function in a followup. I agree it is pretty generic. https://codereview.chromium.org/2762403002/diff/170001/chrome/browser/subreso... File chrome/browser/subresource_filter/chrome_subresource_filter_client.h (right): https://codereview.chromium.org/2762403002/diff/170001/chrome/browser/subreso... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:69: subresource_filter::VerifiedRulesetDealer::Handle* GetRulesetDealer() On 2017/04/03 13:40:07, pkalinnikov wrote: > nit: #include the Handle's header. Done. https://codereview.chromium.org/2762403002/diff/170001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2762403002/diff/170001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:210: GURL url = navigation_handle->GetURL(); On 2017/04/03 13:40:07, pkalinnikov wrote: > nit: Seems like copying the |url| is not necessary here. Could you make it a > const GURL& instead? Good catch done. Result of copy-paste :P https://codereview.chromium.org/2762403002/diff/170001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:233: state.measure_performance = measure_performance_; On 2017/04/03 13:40:07, pkalinnikov wrote: > nit: Can you inline 230-231 to this assignment? Do you mean do something like: measure_performance_ = state.measure_performance = activation_level != ActivationLevel::Disabled && ShouldMeasurePerformanceForPageLoad(); Or do you just mean swapping the definitions of measure_performance_ and state.measure_performance? https://codereview.chromium.org/2762403002/diff/170001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2762403002/diff/170001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:167: std::unique_ptr<ContentSubresourceFilterThrottleManager> throttle_manager_; On 2017/04/03 13:40:07, pkalinnikov wrote: > nit: Please #include <memory> Done. https://codereview.chromium.org/2762403002/diff/170001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2762403002/diff/170001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:49: const char kDisallowUrl[] = "https://example.com/disallowed.html"; On 2017/04/03 13:40:08, pkalinnikov wrote: > nig: kDisallowedUrl Done. https://codereview.chromium.org/2762403002/diff/170001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:353: void NavigateSubframeAndExpectFilter(const GURL& url, bool expect_filtered) { On 2017/04/03 13:40:08, pkalinnikov wrote: > naming nits: > - void NavigateSubframeAndExpectCheckResult > - bool expect_cancelled Done. https://codereview.chromium.org/2762403002/diff/170001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2762403002/diff/170001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:25: class ForwardingNavigationThrottle : public content::NavigationThrottle { On 2017/04/03 13:40:08, pkalinnikov wrote: > nit: #include "content/public/browser/navigation_throttle.h" Done. https://codereview.chromium.org/2762403002/diff/170001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h (right): https://codereview.chromium.org/2762403002/diff/170001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:101: bool OnMessageReceived(const IPC::Message& message, On 2017/04/03 13:40:08, pkalinnikov wrote: > nit: Please forward declare IPC::Message explicitly. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! Please ignore that comment. https://codereview.chromium.org/2762403002/diff/170001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2762403002/diff/170001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:233: state.measure_performance = measure_performance_; On 2017/04/03 15:33:31, Charlie Harrison wrote: > On 2017/04/03 13:40:07, pkalinnikov wrote: > > nit: Can you inline 230-231 to this assignment? > > > Do you mean do something like: > > measure_performance_ = state.measure_performance = > activation_level != ActivationLevel::Disabled && > ShouldMeasurePerformanceForPageLoad(); > > Or do you just mean swapping the definitions of measure_performance_ and > state.measure_performance? Please ignore this, I overlooked that |measure_performance_| is a member (thought it's just a local variable). Sorry for the noise.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % comments. It's really great to see this come together, huge kudos to everyone! Do I understand correctly that the remaining steps are something along the lines of: 1.) Extend the activation IPC, and refactor the SubresourceFilterAgent so frame-level activation is no longer computed on the renderer side. 2.) Distribute remaining logic in the factory: a.) Move computing and recording performance metrics to the ThrottleManager (?) b.) Move redirect chain pattern metrics to the SBActivationThrottle (?) c.) Move maintaining the whitelist to somewhere else (?) 3.) Remove the driver factory (?) https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3472: if (auto* factory = nit: Can this ever be nullptr? https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... File chrome/browser/subresource_filter/chrome_subresource_filter_client.cc (right): https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:20: #include "components/subresource_filter/content/browser/verified_ruleset_dealer.h" nit: I think we don't need the complete type here. https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:551: IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest, SubFrameFiltering) { nit: Let's name this SubframeDocumentLoadFiltering for clarity! https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:556: ASSERT_NO_FATAL_FAILURE(SetRulesetToDisallowURLsWithPathSuffix(".html")); nit: WDYT about making this "included_script.html", so this test could be made stronger to verify that only `one` and `three` are blocked, and the rest still load just fine? https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:1037: int num_subresource_checks = 6 + 5; nit: 5 + 5 + 1 https://codereview.chromium.org/2762403002/diff/190001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2762403002/diff/190001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:247: return IsWhitelisted(navigation_handle->GetURL()); Sanity-check: For main-frames, this should always be `false` if we get here, right? For everything else, WDYT about not checking the whitelist? (Originally that was only meant for main-frame document URLs.) https://codereview.chromium.org/2762403002/diff/190001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2762403002/diff/190001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:353: void NavigateSubframeAndExpectCheckResult(const GURL& url, nit: This is never called with |expect_cancelled| being false. Let's add such a test too! https://codereview.chromium.org/2762403002/diff/190001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:451: if (!factory || navigation_handle->IsSameDocument()) nit: Can factory be nullptr here? https://codereview.chromium.org/2762403002/diff/190001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2762403002/diff/190001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:24: // Used to forward calls to WillProcessResonse to the driver. Placeholder until nit: Could you please file a tracking bug and mark all locations that will need to be removed with TODO(https://crbug.com/...)? https://codereview.chromium.org/2762403002/diff/190001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:163: if (!dealer_handle_) WDYT about moving this after the injection of the forwarding throttle, so that WillProcessResponse is consistently called in the driver factory? I don't think it makes much of a difference, apart from edge cases like whether redirect chain patterns are recorded when there is no RulesetService (which is only when the feature is disabled).
A nit to a nit. https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... File chrome/browser/subresource_filter/chrome_subresource_filter_client.cc (right): https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:20: #include "components/subresource_filter/content/browser/verified_ruleset_dealer.h" On 2017/04/04 11:36:52, engedy wrote: > nit: I think we don't need the complete type here. We do need the header, because the Handle is an inner class. I was thinking once to move it to outside, to make it forward-declarable. But anyway, this would be a separate CL.
https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... File chrome/browser/subresource_filter/chrome_subresource_filter_client.cc (right): https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:20: #include "components/subresource_filter/content/browser/verified_ruleset_dealer.h" On 2017/04/04 11:43:40, pkalinnikov wrote: > On 2017/04/04 11:36:52, engedy wrote: > > nit: I think we don't need the complete type here. > > We do need the header, because the Handle is an inner class. I was thinking once > to move it to outside, to make it forward-declarable. But anyway, this would be > a separate CL. My point is that we are not using anything more here than what the header for the SubresourceFilterClient interface necessarily needs to include anyway.
https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... File chrome/browser/subresource_filter/chrome_subresource_filter_client.cc (right): https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:20: #include "components/subresource_filter/content/browser/verified_ruleset_dealer.h" On 2017/04/04 12:03:23, engedy wrote: > On 2017/04/04 11:43:40, pkalinnikov wrote: > > On 2017/04/04 11:36:52, engedy wrote: > > > nit: I think we don't need the complete type here. > > > > We do need the header, because the Handle is an inner class. I was thinking > once > > to move it to outside, to make it forward-declarable. But anyway, this would > be > > a separate CL. > > My point is that we are not using anything more here than what the header for > the SubresourceFilterClient interface necessarily needs to include anyway. Ah, got it. Agree with this point.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Thanks folks https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3472: if (auto* factory = On 2017/04/04 11:36:52, engedy wrote: > nit: Can this ever be nullptr? I'm pretty sure it can. Embedders of WebContents are not obligated to add tab helpers. Some WebContents users like reader mode will actually add tab helpers in the middle of a navigation in some cases. https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... File chrome/browser/subresource_filter/chrome_subresource_filter_client.cc (right): https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:20: #include "components/subresource_filter/content/browser/verified_ruleset_dealer.h" On 2017/04/04 11:36:52, engedy wrote: > nit: I think we don't need the complete type here. Done. https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:551: IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest, SubFrameFiltering) { On 2017/04/04 11:36:52, engedy wrote: > nit: Let's name this SubframeDocumentLoadFiltering for clarity! Done. https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:556: ASSERT_NO_FATAL_FAILURE(SetRulesetToDisallowURLsWithPathSuffix(".html")); On 2017/04/04 11:36:52, engedy wrote: > nit: WDYT about making this "included_script.html", so this test could be made > stronger to verify that only `one` and `three` are blocked, and the rest still > load just fine? SGTM, done. https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:1037: int num_subresource_checks = 6 + 5; On 2017/04/04 11:36:52, engedy wrote: > nit: 5 + 5 + 1 Done. https://codereview.chromium.org/2762403002/diff/190001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2762403002/diff/190001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:247: return IsWhitelisted(navigation_handle->GetURL()); On 2017/04/04 11:36:52, engedy wrote: > Sanity-check: For main-frames, this should always be `false` if we get here, > right? For everything else, WDYT about not checking the whitelist? > (Originally that was only meant for main-frame document URLs.) Do you mean for non-main frames this should be false? I've added a condition to only check the whitelist on main frames, and allow everything else. I'll add a test for this once subframe activation is fixed in the followup. https://codereview.chromium.org/2762403002/diff/190001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2762403002/diff/190001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:353: void NavigateSubframeAndExpectCheckResult(const GURL& url, On 2017/04/04 11:36:53, engedy wrote: > nit: This is never called with |expect_cancelled| being false. Let's add such a > test too! Done. https://codereview.chromium.org/2762403002/diff/190001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:451: if (!factory || navigation_handle->IsSameDocument()) On 2017/04/04 11:36:53, engedy wrote: > nit: Can factory be nullptr here? Nope, done. https://codereview.chromium.org/2762403002/diff/190001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2762403002/diff/190001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:24: // Used to forward calls to WillProcessResonse to the driver. Placeholder until On 2017/04/04 11:36:53, engedy wrote: > nit: Could you please file a tracking bug and mark all locations that will need > to be removed with TODO(https://crbug.com/...)? Done. I only marked this location because basically everything follows from removing this. https://codereview.chromium.org/2762403002/diff/190001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:163: if (!dealer_handle_) On 2017/04/04 11:36:53, engedy wrote: > WDYT about moving this after the injection of the forwarding throttle, so that > WillProcessResponse is consistently called in the driver factory? > > I don't think it makes much of a difference, apart from edge cases like whether > redirect chain patterns are recorded when there is no RulesetService (which is > only when the feature is disabled). Sure, done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your understanding is correct afaict: Some comments: 1.) Extend the activation IPC, and refactor the SubresourceFilterAgent so frame-level activation is no longer computed on the renderer side. Yep, this is implemented in the dependent PS. 2.) Distribute remaining logic in the factory: a.) Move computing and recording performance metrics to the ThrottleManager Sounds fine to me, or a helper class. b.) Move redirect chain pattern metrics to the SBActivationThrottle (?) Yes this was my understanding too. c.) Move maintaining the whitelist to somewhere else (?) The whitelist set will be moved to the throttle manager, and more general whitelist will be maintained by the ChromeClient. 3.) Remove the driver factory (?) Yes, or move its functionality to some helper classes hanging off throttle manager.
OK I think I understand your previous comment now, since I believe there is nothing outstanding I will go ahead and land to CQ now. https://codereview.chromium.org/2762403002/diff/190001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2762403002/diff/190001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:247: return IsWhitelisted(navigation_handle->GetURL()); On 2017/04/04 15:54:00, Charlie Harrison wrote: > On 2017/04/04 11:36:52, engedy wrote: > > Sanity-check: For main-frames, this should always be `false` if we get here, > > right? For everything else, WDYT about not checking the whitelist? > > (Originally that was only meant for main-frame document URLs.) > > Do you mean for non-main frames this should be false? I've added a condition to > only check the whitelist on main frames, and allow everything else. > > I'll add a test for this once subframe activation is fixed in the followup. Sorry just re-read the comment. Yes currently this will always be false, due to the way activation state is tracked in the driver. As such, this is kinda a no-op but it will be used once the refactor is complete.
The CQ bit was unchecked by csharrison@chromium.org
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkalinnikov@chromium.org, engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2762403002/#ps210001 (title: "engedy review")
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": 210001, "attempt_start_ts": 1491325095818260,
"parent_rev": "53b0eeafd14a0a5699ff8850df41ddef696caddc", "commit_rev":
"e985ebc9f76dd65b59efaa730f018263b64c6c24"}
Message was sent while issue was closed.
Description was changed from ========== Wire up the ThrottleManager using the existing page activation logic BUG=637415 ========== to ========== Wire up the ThrottleManager using the existing page activation logic BUG=637415 Review-Url: https://codereview.chromium.org/2762403002 Cr-Commit-Position: refs/heads/master@{#461754} Committed: https://chromium.googlesource.com/chromium/src/+/e985ebc9f76dd65b59efaa730f01... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:210001) as https://chromium.googlesource.com/chromium/src/+/e985ebc9f76dd65b59efaa730f01...
Message was sent while issue was closed.
https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2762403002/diff/190001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3472: if (auto* factory = On 2017/04/04 15:54:00, Charlie Harrison wrote: > On 2017/04/04 11:36:52, engedy wrote: > > nit: Can this ever be nullptr? > > I'm pretty sure it can. Embedders of WebContents are not obligated to add tab > helpers. Some WebContents users like reader mode will actually add tab helpers > in the middle of a navigation in some cases. Ah, yes, you are right. https://codereview.chromium.org/2762403002/diff/190001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2762403002/diff/190001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:247: return IsWhitelisted(navigation_handle->GetURL()); On 2017/04/04 16:57:35, Charlie Harrison wrote: > On 2017/04/04 15:54:00, Charlie Harrison wrote: > > On 2017/04/04 11:36:52, engedy wrote: > > > Sanity-check: For main-frames, this should always be `false` if we get here, > > > right? For everything else, WDYT about not checking the whitelist? > > > (Originally that was only meant for main-frame document URLs.) > > > > Do you mean for non-main frames this should be false? I've added a condition > to > > only check the whitelist on main frames, and allow everything else. > > > > I'll add a test for this once subframe activation is fixed in the followup. > > Sorry just re-read the comment. Yes currently this will always be false, due to > the way activation state is tracked in the driver. As such, this is kinda a > no-op but it will be used once the refactor is complete. Got it. https://codereview.chromium.org/2762403002/diff/190001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2762403002/diff/190001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:24: // Used to forward calls to WillProcessResonse to the driver. Placeholder until On 2017/04/04 15:54:01, Charlie Harrison wrote: > On 2017/04/04 11:36:53, engedy wrote: > > nit: Could you please file a tracking bug and mark all locations that will > need > > to be removed with TODO(https://crbug.com/...)? > > Done. I only marked this location because basically everything follows from > removing this. Makes sense.
Message was sent while issue was closed.
On 2017/04/04 15:56:37, Charlie Harrison wrote: > Your understanding is correct afaict: Some comments: > > 1.) Extend the activation IPC, and refactor the SubresourceFilterAgent so > frame-level activation is no longer computed on the renderer side. > Yep, this is implemented in the dependent PS. > > 2.) Distribute remaining logic in the factory: > a.) Move computing and recording performance metrics to the ThrottleManager > Sounds fine to me, or a helper class. > > b.) Move redirect chain pattern metrics to the SBActivationThrottle (?) > Yes this was my understanding too. > > c.) Move maintaining the whitelist to somewhere else (?) > The whitelist set will be moved to the throttle manager, and more general > whitelist will be maintained by the ChromeClient. > > 3.) Remove the driver factory (?) > Yes, or move its functionality to some helper classes hanging off throttle > manager. Thanks, sounds good. |
