Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2060313002:
Navigation throttle for the Safe Browsing Subresource Filter. (Closed)
|
Created:
4 years, 6 months ago by melandory Modified:
4 years, 5 months ago CC:
nasko, Mike West, battre, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@pre-tab-activation Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNavigation throttle for the Safe Browsing Subresource Filter.
Adds the SubresourceFilterNavigationThrottle which is responsible for
tracking the redirects, which is happening after Safe Browsing detects
that the page which is being load contains deceptive embedded content
and appends such redirects to the per-tab list. After navigation is
finished, SubresourceFilterNavigationThrottle passes information if
deceptive embedded content should be filtered.
BUG=609747
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/fd0013bc02112d3c70bdce2bac249f31685042dc
Cr-Commit-Position: refs/heads/master@{#406840}
Patch Set 1 : Navigation throttle for the Safe Browsing Subresource Filter. #
Total comments: 1
Patch Set 2 : Comment to SubresourceFilterNavigationThrotle explaining its purpose. #
Total comments: 40
Patch Set 3 : comments #Patch Set 4 : comments #
Total comments: 10
Patch Set 5 : rebase && comments #
Total comments: 4
Patch Set 6 : clamy@ comments #
Total comments: 40
Patch Set 7 : some battre@ comments #
Total comments: 26
Patch Set 8 : adressing engedy@ comments #Messages
Total messages: 99 (64 generated)
Description was changed from ========== Navigation throttle for the Safe Browsing Subresource Filter. Adds the SubresourceFilterNavigationThrottle which is responsible for tracking the redirects, which is happening after Safe Browsing detects that the page which is being load contains deceptive embedded content and ads such redirects to the per-tab list. After navigation is finished, SubresourceFilterNavigationThrottle passes information if deceptive embedded content should be filtered. BUG=609747 ========== to ========== Navigation throttle for the Safe Browsing Subresource Filter. Adds the SubresourceFilterNavigationThrottle which is responsible for tracking the redirects, which is happening after Safe Browsing detects that the page which is being load contains deceptive embedded content and ads such redirects to the per-tab list. After navigation is finished, SubresourceFilterNavigationThrottle passes information if deceptive embedded content should be filtered. BUG=609747 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
melandory@chromium.org changed reviewers: + clamy@chromium.org, engedy@chromium.org, nasko@chromium.org
clamy@chromium.org and nasko@chromium.org for navigation throttle parts, engedy@ for subresource filter parts.
clamy@chromium.org and nasko@chromium.org for navigation throttle parts, engedy@ for subresource filter parts.
Description was changed from ========== Navigation throttle for the Safe Browsing Subresource Filter. Adds the SubresourceFilterNavigationThrottle which is responsible for tracking the redirects, which is happening after Safe Browsing detects that the page which is being load contains deceptive embedded content and ads such redirects to the per-tab list. After navigation is finished, SubresourceFilterNavigationThrottle passes information if deceptive embedded content should be filtered. BUG=609747 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Navigation throttle for the Safe Browsing Subresource Filter. Adds the SubresourceFilterNavigationThrottle which is responsible for tracking the redirects, which is happening after Safe Browsing detects that the page which is being load contains deceptive embedded content and appends such redirects to the per-tab list. After navigation is finished, SubresourceFilterNavigationThrottle passes information if deceptive embedded content should be filtered. BUG=609747 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
friendly ping
Please take a look, thanks!
Thanks! https://codereview.chromium.org/2060313002/diff/60001/content/public/browser/... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2060313002/diff/60001/content/public/browser/... content/public/browser/navigation_handle.h:126: // called before ReadyToCommitNavigation is dispatched. This is not true in PlzNavigate, and in the current architecture we don't currently dispatch ReadyToCommitNavigation. @Nasko: do we have plans to dispatch ReadyToCommitNavigation after we make the choice to transfer the navigation or not? I think it would be possible. In that case, since the patch doesn't need to cancel/pause navigations, it could be rewritten as a WebContentsObserver that only listens to DidRedirectNavigation and ReadyToCommitNavigation.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) 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/patch-status/2060313002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) 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/patch-status/2060313002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:180001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:200001) has been deleted
Patchset #2 (id:220001) has been deleted
Patchset #2 (id:240001) has been deleted
On 2016/06/23 13:29:18, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) PTAL, thanks!
Looking good, there is one thing I would like to understand and test more, which is how |activation_set| is used for subsequent navigations. Other than that, just some minor comments and nits. Thank you! https://codereview.chromium.org/2060313002/diff/260001/components/subresource... File components/subresource_filter/content/browser/BUILD.gn (right): https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/BUILD.gn:34: "subresource_filter_navigation_throttle_unittests.cc", Don't forget to add this to components_tests.gyp, as well as the dependencies below. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/BUILD.gn:41: "//components/subresource_filter/core/browser:browser", nit: You may just write: "//components/subresource_filter/core/browser", https://codereview.chromium.org/2060313002/diff/260001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:67: for (const auto& url : redirect_urls) { nit: No {} needed. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:84: frame_drivers_.insert(std::make_pair(render_frame_host, std::move(driver))); Note that this is somewhat fragile in the sense that if a driver was already there, it will not be overwritten, instead the passed in |driver| is just destroyed. Could you please either add a comment to give warning of this to the header, or use the approach from lines 51--56? https://codereview.chromium.org/2060313002/diff/260001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:52: const OriginSet& activation_set() { return activate_on_origins_; } Can this and the one below be made const? https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:54: void AddToSocEngList(const GURL& url); s//AddOriginOfURLToActivationSet/ for consistency. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:66: void TestingSetDriverForFrame( nit: I think a more traditional naming convention is SetDriverForFrameHostForTesting (also note the `Host`). https://codereview.chromium.org/2060313002/diff/260001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc (right): https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:35: if (driver_factory->ShouldActivateForURL(initial_url_)) { What happens if the second (or later) URL in a redirect chains matches something on the |activation_set|? (Where that `something` was added by a previous navigation hitting the SB blacklist.) https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:50: if (driver_factory->ShouldActivateForURL(initial_url_)) { Is there an guaranteed ordering in which the throttles are consulted? In other words, suppose there is a navigation that has no redirects. Is it ensured that the SBNavigationThrottle will have run at this point, even if it finishes asynchronously? https://codereview.chromium.org/2060313002/diff/260001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h (right): https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:20: /* The class which is responsible for tracking the redirects, which are nit: // https://codereview.chromium.org/2060313002/diff/260001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc (right): https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:20: #include "testing/gmock/include/gmock/gmock.h" nit: Not used. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:85: return handle()->CallWillStartRequestForTesting( nit: Could you please add /* param_name */ after each boolean argument so it's clear what all the trues and falses do here? https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:91: handle()->CallWillStartRequestForTesting(true, content::Referrer(), false, I find it strange that we need to call this here. Is this really needed? https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:109: base::FieldTrialList field_trial_list(nullptr); nit: Consider moving the base::FieldTrialList instance into the test fixture to avoid duplication. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:113: const GURL url("https://example.com"); nit: Given that this literal appears multiple times within a given test, it's running the risk of typos across different occurrences potentially masking test failures. I'd suggest defining a constant for this literal in an unnamed namespace at the top of the file, so that the compiler can check typos. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:182: redirects.push_back(GURL("https://example1.com")); nit: Same goes for this. Please either define const named GURL instances at the top of this test, or const char[]s at the top of this file. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:202: } // namespace navigation_interception Could you please add two more tests that have a subsequent navigation? One where the first URL matches the activation_set(), and a second with A, B, C, with B matching the activation_set()?
clamy@, can you also take a look?
https://codereview.chromium.org/2060313002/diff/260001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2060313002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2903: if (handle->IsInMainFrame() && handle->GetURL().SchemeIsHTTPOrHTTPS()) { Put a TODO indicating that this should move to a WebContentsObserver, once we call ReadyToCommitNavigation properly? Note: we have a bug tracking that at crbug.com/621856. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc (right): https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:50: if (driver_factory->ShouldActivateForURL(initial_url_)) { On 2016/06/23 22:34:18, engedy wrote: > Is there an guaranteed ordering in which the throttles are consulted? In other > words, suppose there is a navigation that has no redirects. Is it ensured that > the SBNavigationThrottle will have run at this point, even if it finishes > asynchronously? There is no SBNavigationThrottle, but a SBResourceThrottle at this point. Currently, the ordering is guaranteed to be the following: *NavigationThrottle::WillRedirectRequest SafeBrowsingResourceThrottle::WillRedirectRequest *NavigationThrottle::WillProcessResponse SafeBrowsingResourceThrottle::WillProcessResponse AFAIK on desktop the SafeBrowsing check is performed at navigation start, however it is asynchronous on Android. https://codereview.chromium.org/2060313002/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2060313002/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:186: CHECK(state_ >= READY_TO_COMMIT || state_ == WILL_PROCESS_RESPONSE) This should be state_ >= WILL_PROCESS_RESPONSE. https://codereview.chromium.org/2060313002/diff/260001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2060313002/diff/260001/content/public/browser... content/public/browser/navigation_handle.h:129: // The RenderFrameHost returned back might not be the final one, if method is So 2 things here: With PlzNavigate this is the final one. Without PlzNavigate, the navigation may transfer. In that case, you won't get the final one before DidFinishNavigation, since ReadyToCommitNavigation will not be called. The comment should be rewritten to take this into account.
mkwst@chromium.org changed reviewers: + mkwst@chromium.org
Quick drive-by: https://codereview.chromium.org/2060313002/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2060313002/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:186: CHECK(state_ >= READY_TO_COMMIT || state_ == WILL_PROCESS_RESPONSE) On 2016/06/24 at 12:13:38, clamy wrote: > This should be state_ >= WILL_PROCESS_RESPONSE. Note that https://codereview.chromium.org/2025683003 and https://codereview.chromium.org/1617043002/ are also changing this line. You might need to rebase depending on when these land. :)
https://codereview.chromium.org/2060313002/diff/260001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2060313002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2903: if (handle->IsInMainFrame() && handle->GetURL().SchemeIsHTTPOrHTTPS()) { On 2016/06/24 12:13:38, clamy wrote: > Put a TODO indicating that this should move to a WebContentsObserver, once we > call ReadyToCommitNavigation properly? Note: we have a bug tracking that at > crbug.com/621856. Done. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... File components/subresource_filter/content/browser/BUILD.gn (right): https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/BUILD.gn:34: "subresource_filter_navigation_throttle_unittests.cc", On 2016/06/23 22:34:17, engedy wrote: > Don't forget to add this to components_tests.gyp, as well as the dependencies > below. Done. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/BUILD.gn:41: "//components/subresource_filter/core/browser:browser", On 2016/06/23 22:34:17, engedy wrote: > nit: You may just write: > > "//components/subresource_filter/core/browser", Done. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:67: for (const auto& url : redirect_urls) { On 2016/06/23 22:34:17, engedy wrote: > nit: No {} needed. Done. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:84: frame_drivers_.insert(std::make_pair(render_frame_host, std::move(driver))); On 2016/06/23 22:34:17, engedy wrote: > Note that this is somewhat fragile in the sense that if a driver was already > there, it will not be overwritten, instead the passed in |driver| is just > destroyed. > > Could you please either add a comment to give warning of this to the header, or > use the approach from lines 51--56? Done. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:52: const OriginSet& activation_set() { return activate_on_origins_; } On 2016/06/23 22:34:17, engedy wrote: > Can this and the one below be made const? Done. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:54: void AddToSocEngList(const GURL& url); On 2016/06/23 22:34:17, engedy wrote: > s//AddOriginOfURLToActivationSet/ for consistency. Done. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:66: void TestingSetDriverForFrame( On 2016/06/23 22:34:17, engedy wrote: > nit: I think a more traditional naming convention is > SetDriverForFrameHostForTesting (also note the `Host`). Done. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc (right): https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:35: if (driver_factory->ShouldActivateForURL(initial_url_)) { On 2016/06/23 22:34:18, engedy wrote: > What happens if the second (or later) URL in a redirect chains matches something > on the |activation_set|? (Where that `something` was added by a previous > navigation hitting the SB blacklist.) Redirects which has happened before SB match are recorded by SBResourceThrottle. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h (right): https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:20: /* The class which is responsible for tracking the redirects, which are On 2016/06/23 22:34:18, engedy wrote: > nit: // Done. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc (right): https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:20: #include "testing/gmock/include/gmock/gmock.h" On 2016/06/23 22:34:18, engedy wrote: > nit: Not used. Done. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:85: return handle()->CallWillStartRequestForTesting( On 2016/06/23 22:34:18, engedy wrote: > nit: Could you please add /* param_name */ after each boolean argument so it's > clear what all the trues and falses do here? Done. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:91: handle()->CallWillStartRequestForTesting(true, content::Referrer(), false, On 2016/06/23 22:34:18, engedy wrote: > I find it strange that we need to call this here. Is this really needed? Done. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:113: const GURL url("https://example.com"); On 2016/06/23 22:34:18, engedy wrote: > nit: Given that this literal appears multiple times within a given test, it's > running the risk of typos across different occurrences potentially masking test > failures. I'd suggest defining a constant for this literal in an unnamed > namespace at the top of the file, so that the compiler can check typos. Done. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:182: redirects.push_back(GURL("https://example1.com")); On 2016/06/23 22:34:18, engedy wrote: > nit: Same goes for this. Please either define const named GURL instances at the > top of this test, or const char[]s at the top of this file. Done. https://codereview.chromium.org/2060313002/diff/260001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:202: } // namespace navigation_interception On 2016/06/23 22:34:18, engedy wrote: > Could you please add two more tests that have a subsequent navigation? > One where the first URL matches the activation_set() Isn't RequestWithoutRedirectsNoActivation is what you described? > and a second with A, B, C, with B matching the activation_set()? Done. https://codereview.chromium.org/2060313002/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2060313002/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:186: CHECK(state_ >= READY_TO_COMMIT || state_ == WILL_PROCESS_RESPONSE) On 2016/06/24 12:13:38, clamy wrote: > This should be state_ >= WILL_PROCESS_RESPONSE. Done https://codereview.chromium.org/2060313002/diff/260001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2060313002/diff/260001/content/public/browser... content/public/browser/navigation_handle.h:129: // The RenderFrameHost returned back might not be the final one, if method is On 2016/06/24 12:13:38, clamy wrote: > So 2 things here: > With PlzNavigate this is the final one. > Without PlzNavigate, the navigation may transfer. In that case, you won't get > the final one before DidFinishNavigation, since ReadyToCommitNavigation will not > be called. > > The comment should be rewritten to take this into account. Done.
clamy@, PTAL. Thanks!
@melandory: I'm waiting for an agreement on https://codereview.chromium.org/2097083002/ and its landing since your CL will need to be rebased on top of it.
On 2016/06/27 13:24:59, clamy wrote: > @melandory: I'm waiting for an agreement on > https://codereview.chromium.org/2097083002/ and its landing since your CL will > need to be rebased on top of it. Thanks for heads up.
On 2016/06/27 at 13:29:40, melandory wrote: > On 2016/06/27 13:24:59, clamy wrote: > > @melandory: I'm waiting for an agreement on > > https://codereview.chromium.org/2097083002/ and its landing since your CL will > > need to be rebased on top of it. > > Thanks for heads up. That CL landed this morning. It should be a pretty clean rebase.
https://codereview.chromium.org/2060313002/diff/300001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2060313002/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2907: // pre-PlzNavigate world (tracking bug: crbug.com/621856). nit: https://crbug... https://codereview.chromium.org/2060313002/diff/300001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc (right): https://codereview.chromium.org/2060313002/diff/300001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:25: initial_url_ = navigation_handle()->GetURL(); Why not put this in the initializer list? https://codereview.chromium.org/2060313002/diff/300001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:32: subresource_filter::ContentSubresourceFilterDriverFactory* driver_factory = You are already in the subresource_filter namespace, no need to prefix classes with it. It just makes it less readable. https://codereview.chromium.org/2060313002/diff/300001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h (right): https://codereview.chromium.org/2060313002/diff/300001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:20: // The class which is responsible for tracking the redirects, which are "This class is responsible ...". Also, is it only monitoring for redirects? Based on overriding WillProcessResponse, it is tracking the responses too, isn't it? https://codereview.chromium.org/2060313002/diff/300001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:21: // happening after Safe Browsing detects that the page which is being load nit: "being loaded"
Patchset #5 (id:320001) has been deleted
PTAL, thanks https://codereview.chromium.org/2060313002/diff/300001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2060313002/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2907: // pre-PlzNavigate world (tracking bug: crbug.com/621856). On 2016/06/28 22:12:36, nasko wrote: > nit: https://crbug... Done. https://codereview.chromium.org/2060313002/diff/300001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc (right): https://codereview.chromium.org/2060313002/diff/300001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:25: initial_url_ = navigation_handle()->GetURL(); On 2016/06/28 22:12:36, nasko wrote: > Why not put this in the initializer list? No good reason at all. https://codereview.chromium.org/2060313002/diff/300001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:32: subresource_filter::ContentSubresourceFilterDriverFactory* driver_factory = On 2016/06/28 22:12:36, nasko wrote: > You are already in the subresource_filter namespace, no need to prefix classes > with it. It just makes it less readable. Done. https://codereview.chromium.org/2060313002/diff/300001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h (right): https://codereview.chromium.org/2060313002/diff/300001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:20: // The class which is responsible for tracking the redirects, which are On 2016/06/28 22:12:36, nasko wrote: > "This class is responsible ...". Also, is it only monitoring for redirects? > Based on overriding WillProcessResponse, it is tracking the responses too, isn't > it? Done. https://codereview.chromium.org/2060313002/diff/300001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:21: // happening after Safe Browsing detects that the page which is being load On 2016/06/28 22:12:36, nasko wrote: > nit: "being loaded" Done.
Thanks! Lgtm with nits. https://codereview.chromium.org/2060313002/diff/340001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h (right): https://codereview.chromium.org/2060313002/diff/340001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:27: // doesn't nit: line break is wrong. https://codereview.chromium.org/2060313002/diff/340001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2060313002/diff/340001/content/public/browser... content/public/browser/navigation_handle.h:195: // Simulates that throttle is about to process the response. nit:s/that throttle is about to process the response/the reception of the network response
melandory@chromium.org changed reviewers: + battre@chromium.org
Dominic, can you take a look (unfortunately, engedy@ did already one pass, but since I'm OOO for 2 weeks, I don't want to leave the CL lying around for this time). Thanks! https://codereview.chromium.org/2060313002/diff/340001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h (right): https://codereview.chromium.org/2060313002/diff/340001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:27: // doesn't On 2016/07/01 11:48:51, clamy wrote: > nit: line break is wrong. Done. https://codereview.chromium.org/2060313002/diff/340001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2060313002/diff/340001/content/public/browser... content/public/browser/navigation_handle.h:195: // Simulates that throttle is about to process the response. On 2016/07/01 11:48:51, clamy wrote: > nit:s/that throttle is about to process the response/the reception of the > network response Done.
Hi. Sorry, I have a ton of nits and some suggested refactorings that would make some of these obsolete. Best regards, Dominic https://codereview.chromium.org/2060313002/diff/360001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:35: // WebContents and manufactures the per-frame ContentSubresourceFilterDrivers. Can you please add a ton of comments to this class? It makes no sense to me. I wonder whether you should extract the activation set out of this class. // An activation set represents a set of hostnames for which the Subresource Filter should be enabled. A hostname gets into the set class ActivationSet { public: // Adds the host of the current URL and the redirect chain to the // activation set. This is called by the SafeBrowsing service. void OnMainResourceMatchedSafeBrowsingBlacklist(...); // Takes care that once a host of a redirect chain is blacklisted // also the following hosts are blacklisted. Otherwise, the activation // status could be removed by introducing redirects. void OnRedirect(const GURL& initial_url, const GURL& new_url); bool ContainsHost(const std::string& host) const; void AddHost(const std::string& host); private: using OriginSet = std::set<std::string>; OriginSet origin_set_; } https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:51: safe_browsing::ThreatPatternType threat_type); When is this called? What does it do? https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:52: const OriginSet& activation_set() const { return activate_on_origins_; } What is an activation set? Do you want to expose this? The Test can access it via the testing anyway, right? https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:53: bool ShouldActivateForURL(const GURL& url) const; Who determines this? Should this be called "InActivationSet"? "ShouldActivate" grammatically lacks an object. Should activate what? https://codereview.chromium.org/2060313002/diff/360001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc (right): https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:30: SubresourceFilterNavigationThrottle::WillRedirectRequest() { I wonder whether the NavigationThrottle should just delegate these functions. Currently you have two places that determine what should go into the activation set: SubresourceFilterNavigationThrottle::WillRedirectRequest() and ContentSubresourceFilterDriverFactory::OnMainResourceMatchedSafeBrowsingBlacklist(). WillRedirectRequest() would become ContentSubresourceFilterDriverFactory::FromWebContents( navigation_handle()->GetWebContents())->activation_set()->Redirect(initial_url_, navigation_handle()->GetURL()); And WillProcessResponse() would become ContentSubresourceFilterDriverFactory::FromWebContents( navigation_handle()->GetWebContents())->OnProcessResponse(); OnProcessResponse would decide whether to create a ContentSubresourceFilterDriver and do it if appropriate. If you do this, my comments in the headerfile would become obsolete. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:33: navigation_handle()->GetWebContents()); // Ensure that the activation state of the subresource filter is persisted beyond redirects. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h (right): https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:23: // appends such redirects to the per-tab list. What is "the per-tab list"? * it tracks redirects, which happen after Safe Browsing detected that the page being loaded contains deceptive embedded content. If such a redirect happens and lead to new domains, these are also put on the activation list of the tab. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:25: // should be filtered. * it creates a ContentSubresourceFilterDriver for the tab when the final site of a (potentially empty) redirect chain is reached and any URL of the redirect cain was on the activation list. ? https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:28: // necessary. The ", but..." part could be removed in my opinion with the changes above. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc (right): https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:127: safe_browsing::ThreatPatternType::SOCIAL_ENGINEERING_ADS); Why does this happen before SimulateWillStart()? https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:139: RequestWithoutRedirectsNoActivation) { No activation of the Field Trial? https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:153: EXPECT_FALSE(factory()->ShouldActivateForURL(GURL(kTestURL))); move GURL(kTestURL) up: const GURL url_with_activation(kExampleURL); const GURL url_without_activation(kTestURL); ? https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:158: AddRedirectFromNavThrottleToServiceEnptyInitRedirects) { typo: Enpty https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:158: AddRedirectFromNavThrottleToServiceEnptyInitRedirects) { Can you add comments to what these tests test? The testnames are pretty hard to parse. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:172: SimulateRedirects(redirect); No WillStart before Redirects? https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:188: const GURL nav_throttle_redirect(kTestURL); What is the meaning of this variable? https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:191: std::vector<GURL> redirects; you can initialize vectors better now: std::vector<GURL> redirects = { GURL(kRedirectURLFirst), ... }; https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:200: SimulateRedirects(nav_throttle_redirect); again: Redirects before WillStart()? https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:221: std::vector<GURL> redirects; redirects = {redirect_with_match}; https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:228: SetUpNavigationHandleForURL(init_url); WillStart?
Patchset #7 (id:380001) 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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Patchset #7 (id:400001) has been deleted
Patchset #7 (id:420001) has been deleted
The CQ bit was unchecked by melandory@chromium.org
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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
https://codereview.chromium.org/2060313002/diff/360001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:35: // WebContents and manufactures the per-frame ContentSubresourceFilterDrivers. On 2016/07/01 15:51:35, battre wrote: > Can you please add a ton of comments to this class? It makes no sense to me. > > I wonder whether you should extract the activation set out of this class. > > // An activation set represents a set of hostnames for which the Subresource > Filter should be enabled. A hostname gets into the set > class ActivationSet { > public: > // Adds the host of the current URL and the redirect chain to the > // activation set. This is called by the SafeBrowsing service. > void OnMainResourceMatchedSafeBrowsingBlacklist(...); > // Takes care that once a host of a redirect chain is blacklisted > // also the following hosts are blacklisted. Otherwise, the activation > // status could be removed by introducing redirects. > void OnRedirect(const GURL& initial_url, const GURL& new_url); > bool ContainsHost(const std::string& host) const; > void AddHost(const std::string& host); > > private: > using OriginSet = std::set<std::string>; > OriginSet origin_set_; > } I think both comments and extraction better to do in a different CL. As bonus I can sent it to the English readability =) https://codereview.chromium.org/2060313002/diff/360001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc (right): https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:30: SubresourceFilterNavigationThrottle::WillRedirectRequest() { On 2016/07/01 15:51:36, battre wrote: > I wonder whether the NavigationThrottle should just delegate these functions. > Currently you have two places that determine what should go into the activation > set: > SubresourceFilterNavigationThrottle::WillRedirectRequest() and > ContentSubresourceFilterDriverFactory::OnMainResourceMatchedSafeBrowsingBlacklist(). > > WillRedirectRequest() would become > > ContentSubresourceFilterDriverFactory::FromWebContents( > > navigation_handle()->GetWebContents())->activation_set()->Redirect(initial_url_, > navigation_handle()->GetURL()); > > And WillProcessResponse() would become > > ContentSubresourceFilterDriverFactory::FromWebContents( > navigation_handle()->GetWebContents())->OnProcessResponse(); > > OnProcessResponse would decide whether to create a > ContentSubresourceFilterDriver and do it if appropriate. > > If you do this, my comments in the headerfile would become obsolete. I like the suggestion, although I would do this refactoring as follow up if you don't mind. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:33: navigation_handle()->GetWebContents()); On 2016/07/01 15:51:36, battre wrote: > // Ensure that the activation state of the subresource filter is persisted > beyond redirects. Done. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h (right): https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:23: // appends such redirects to the per-tab list. On 2016/07/01 15:51:36, battre wrote: > What is "the per-tab list"? > > * it tracks redirects, which happen after Safe Browsing detected that the page > being loaded contains deceptive embedded content. If such a redirect happens and > lead to new domains, these are also put on the activation list of the tab. Done. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:25: // should be filtered. On 2016/07/01 15:51:36, battre wrote: > * it creates a ContentSubresourceFilterDriver for the tab when the final site of > a (potentially empty) redirect chain is reached and any URL of the redirect cain > was on the activation list. > > ? Done. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:28: // necessary. On 2016/07/01 15:51:36, battre wrote: > The ", but..." part could be removed in my opinion with the changes above. Done. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc (right): https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:127: safe_browsing::ThreatPatternType::SOCIAL_ENGINEERING_ADS); On 2016/07/01 15:51:36, battre wrote: > Why does this happen before SimulateWillStart()? Moved SimulateWillStart. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:139: RequestWithoutRedirectsNoActivation) { On 2016/07/01 15:51:36, battre wrote: > No activation of the Field Trial? Done. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:153: EXPECT_FALSE(factory()->ShouldActivateForURL(GURL(kTestURL))); On 2016/07/01 15:51:36, battre wrote: > move GURL(kTestURL) up: > const GURL url_with_activation(kExampleURL); > const GURL url_without_activation(kTestURL); > ? Done. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:158: AddRedirectFromNavThrottleToServiceEnptyInitRedirects) { On 2016/07/01 15:51:36, battre wrote: > typo: Enpty Done. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:158: AddRedirectFromNavThrottleToServiceEnptyInitRedirects) { On 2016/07/01 15:51:36, battre wrote: > Can you add comments to what these tests test? The testnames are pretty hard to > parse. Done. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:172: SimulateRedirects(redirect); On 2016/07/01 15:51:36, battre wrote: > No WillStart before Redirects? Done. But it's not really nessary. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:188: const GURL nav_throttle_redirect(kTestURL); On 2016/07/01 15:51:36, battre wrote: > What is the meaning of this variable? Renamed. It's the redirect which has happened after Safe Browsing hit. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:191: std::vector<GURL> redirects; On 2016/07/01 15:51:36, battre wrote: > you can initialize vectors better now: > std::vector<GURL> redirects = { > GURL(kRedirectURLFirst), ... }; Done. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:200: SimulateRedirects(nav_throttle_redirect); On 2016/07/01 15:51:36, battre wrote: > again: Redirects before WillStart()? Done. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:221: std::vector<GURL> redirects; On 2016/07/01 15:51:36, battre wrote: > redirects = {redirect_with_match}; Done. https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:228: SetUpNavigationHandleForURL(init_url); On 2016/07/01 15:51:36, battre wrote: > WillStart? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
engedy@, can you please take a look?
LGTM on components/subresource_filter/ % comments. https://codereview.chromium.org/2060313002/diff/360001/components/components_... File components/components_tests.gyp (right): https://codereview.chromium.org/2060313002/diff/360001/components/components_... components/components_tests.gyp:811: 'subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc', nit: s/unittests/unittest/ https://codereview.chromium.org/2060313002/diff/360001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:86: if (iterator_and_inserted.second) { nit: Wouldn't it be better to have overwrite semantics here and drop the conditional? https://codereview.chromium.org/2060313002/diff/360001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc (right): https://codereview.chromium.org/2060313002/diff/360001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:30: SubresourceFilterNavigationThrottle::WillRedirectRequest() { On 2016/07/18 15:16:47, melandory wrote: > On 2016/07/01 15:51:36, battre wrote: > > I wonder whether the NavigationThrottle should just delegate these functions. > > Currently you have two places that determine what should go into the > activation > > set: > > SubresourceFilterNavigationThrottle::WillRedirectRequest() and > > > ContentSubresourceFilterDriverFactory::OnMainResourceMatchedSafeBrowsingBlacklist(). > > > > WillRedirectRequest() would become > > > > ContentSubresourceFilterDriverFactory::FromWebContents( > > > > > navigation_handle()->GetWebContents())->activation_set()->Redirect(initial_url_, > > navigation_handle()->GetURL()); > > > > And WillProcessResponse() would become > > > > ContentSubresourceFilterDriverFactory::FromWebContents( > > navigation_handle()->GetWebContents())->OnProcessResponse(); > > > > OnProcessResponse would decide whether to create a > > ContentSubresourceFilterDriver and do it if appropriate. > > > > If you do this, my comments in the headerfile would become obsolete. > > I like the suggestion, although I would do this refactoring as follow up if you > don't mind. I really like this approach as well. Fine to do in a follow-up CL. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:60: OnMainResourceMatchedSafeBrowsingBlacklist( Could you please refresh my memory: did we conclude that the cross-thread call to this method from the SafeBrowsingResourceThrottle will arrive in time? If so, could you please document in a comment why? https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:78: activate_on_origins_.insert(url.host()); As a safeguard, I think we should either check here that url.host() is non-empty (it will be for URLs with special schemes), or check the same at all call sites plus add a DCHECK here. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:108: void ContentSubresourceFilterDriverFactory::DidStartProvisionalLoadForFrame( Okay to do in a follow-up CL: I am somewhat concerned that we only have very artificial tests for this class, e.g., the activation call in this method does not result in any tests failing. Could you please think about how we could have closer-to-life unittests (or a browsertest) that would organically trigger notifications such as DidStartProvisionalLoadForFrame, thus would be sensitive to this call here? https://codereview.chromium.org/2060313002/diff/440001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc (right): https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:46: if (!navigation_handle()->GetURL().SchemeIsHTTPOrHTTPS()) Do we need the same check above? https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:53: ContentSubresourceFilterDriver* driver = Here we will have to take care of activating not only for the main frame, but for all subframes (including those created dynamically later). I think this should be the responsibility of the DriverFactory. We can do this in a follow up-CL, but please add a TODO. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h (right): https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:16: } nit: } // namespace content https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:21: // * it tracks the redirects, which happen after Safe Browsing detects that the nit: it tracks redirects that happen ... https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:23: // happens and lead to new domains, these are also put on the activation list typo: leads https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:29: // doesn't act on subresources. nit: does not act on subframe navigations or subresource loads. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc (right): https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:17: #include "content/public/browser/navigation_throttle.h" nit: No need to include this, it is necessarily included by the class under test. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:55: DISALLOW_COPY_AND_ASSIGN(TestContentSubresourceFilterDriver); nit: blank line before, and #include "base/macros.h" https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:112: TestContentSubresourceFilterDriver* driver_; nit: Mention that this is owned by the driver factory. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:118: TEST_F(SubresourceFilterNavigationThrottleTest, RequestWithoutRedirects) { Could you please add tests to verify no activation when: -- the URL match SB blacklist, but the feature is disabled, -- in case of non HTTP/HTTPS URLs (e.g. chrome:// or javascript:// URIs). https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:135: EXPECT_EQ(GetMaximumActivationState(), driver()->activation_state()); nit: Make this explicitly ActivationState::ENABLED, same below.
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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
https://codereview.chromium.org/2060313002/diff/440001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:60: OnMainResourceMatchedSafeBrowsingBlacklist( On 2016/07/19 11:27:02, engedy wrote: > Could you please refresh my memory: did we conclude that the cross-thread call > to this method from the SafeBrowsingResourceThrottle will arrive in time? If so, > could you please document in a comment why? Comment added to SubresourceFilterNavigationThrottle::WillRedirectRequest, which is affected by this. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:78: activate_on_origins_.insert(url.host()); On 2016/07/19 11:27:02, engedy wrote: > As a safeguard, I think we should either check here that url.host() is non-empty > (it will be for URLs with special schemes), or check the same at all call sites > plus add a DCHECK here. Done. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc (right): https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:46: if (!navigation_handle()->GetURL().SchemeIsHTTPOrHTTPS()) On 2016/07/19 11:27:02, engedy wrote: > Do we need the same check above? Will not harm. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:53: ContentSubresourceFilterDriver* driver = On 2016/07/19 11:27:02, engedy wrote: > Here we will have to take care of activating not only for the main frame, but > for all subframes (including those created dynamically later). I think this > should be the responsibility of the DriverFactory. > > We can do this in a follow up-CL, but please add a TODO. Done. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h (right): https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:16: } On 2016/07/19 11:27:03, engedy wrote: > nit: } // namespace content Done. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:21: // * it tracks the redirects, which happen after Safe Browsing detects that the On 2016/07/19 11:27:02, engedy wrote: > nit: it tracks redirects that happen ... Done. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:23: // happens and lead to new domains, these are also put on the activation list On 2016/07/19 11:27:03, engedy wrote: > typo: leads Done. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h:29: // doesn't act on subresources. On 2016/07/19 11:27:03, engedy wrote: > nit: does not act on subframe navigations or subresource loads. Done. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc (right): https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:17: #include "content/public/browser/navigation_throttle.h" On 2016/07/19 11:27:03, engedy wrote: > nit: No need to include this, it is necessarily included by the class under > test. Done. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:55: DISALLOW_COPY_AND_ASSIGN(TestContentSubresourceFilterDriver); On 2016/07/19 11:27:03, engedy wrote: > nit: blank line before, and #include "base/macros.h" Done. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:112: TestContentSubresourceFilterDriver* driver_; On 2016/07/19 11:27:03, engedy wrote: > nit: Mention that this is owned by the driver factory. Done. https://codereview.chromium.org/2060313002/diff/440001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:135: EXPECT_EQ(GetMaximumActivationState(), driver()->activation_state()); On 2016/07/19 11:27:03, engedy wrote: > nit: Make this explicitly ActivationState::ENABLED, same below. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
melandory@chromium.org changed reviewers: - battre@chromium.org, mkwst@chromium.org, nasko@chromium.org
Description was changed from ========== Navigation throttle for the Safe Browsing Subresource Filter. Adds the SubresourceFilterNavigationThrottle which is responsible for tracking the redirects, which is happening after Safe Browsing detects that the page which is being load contains deceptive embedded content and appends such redirects to the per-tab list. After navigation is finished, SubresourceFilterNavigationThrottle passes information if deceptive embedded content should be filtered. BUG=609747 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Navigation throttle for the Safe Browsing Subresource Filter. Adds the SubresourceFilterNavigationThrottle which is responsible for tracking the redirects, which is happening after Safe Browsing detects that the page which is being load contains deceptive embedded content and appends such redirects to the per-tab list. After navigation is finished, SubresourceFilterNavigationThrottle passes information if deceptive embedded content should be filtered. BUG=609747 ==========
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_...)
Description was changed from ========== Navigation throttle for the Safe Browsing Subresource Filter. Adds the SubresourceFilterNavigationThrottle which is responsible for tracking the redirects, which is happening after Safe Browsing detects that the page which is being load contains deceptive embedded content and appends such redirects to the per-tab list. After navigation is finished, SubresourceFilterNavigationThrottle passes information if deceptive embedded content should be filtered. BUG=609747 ========== to ========== Navigation throttle for the Safe Browsing Subresource Filter. Adds the SubresourceFilterNavigationThrottle which is responsible for tracking the redirects, which is happening after Safe Browsing detects that the page which is being load contains deceptive embedded content and appends such redirects to the per-tab list. After navigation is finished, SubresourceFilterNavigationThrottle passes information if deceptive embedded content should be filtered. BUG=609747 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #8 (id:460001) has been deleted
Patchset #10 (id:520001) has been deleted
Patchset #8 (id:480001) 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:500001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2060313002/#ps540001 (title: "adressing engedy@ comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:540001)
Message was sent while issue was closed.
Description was changed from ========== Navigation throttle for the Safe Browsing Subresource Filter. Adds the SubresourceFilterNavigationThrottle which is responsible for tracking the redirects, which is happening after Safe Browsing detects that the page which is being load contains deceptive embedded content and appends such redirects to the per-tab list. After navigation is finished, SubresourceFilterNavigationThrottle passes information if deceptive embedded content should be filtered. BUG=609747 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Navigation throttle for the Safe Browsing Subresource Filter. Adds the SubresourceFilterNavigationThrottle which is responsible for tracking the redirects, which is happening after Safe Browsing detects that the page which is being load contains deceptive embedded content and appends such redirects to the per-tab list. After navigation is finished, SubresourceFilterNavigationThrottle passes information if deceptive embedded content should be filtered. BUG=609747 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/fd0013bc02112d3c70bdce2bac249f31685042dc Cr-Commit-Position: refs/heads/master@{#406840} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/fd0013bc02112d3c70bdce2bac249f31685042dc Cr-Commit-Position: refs/heads/master@{#406840} |
