Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2696493003:
Introduce SubframeNavigationFilteringThrottle (Closed)
|
Created:
3 years, 10 months ago by Charlie Harrison Modified:
3 years, 9 months ago CC:
chromium-reviews, nasko+codewatch_chromium.org, darin-cc_chromium.org, jam, subresource-filter-reviews_chromium.org, creis+watch_chromium.org, clamy Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce SubframeNavigationFilteringThrottle
As of now, it is never injected into the throttle list. Unit tests
coming in a followup.
BUG=637415
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2696493003
Cr-Commit-Position: refs/heads/master@{#453375}
Committed: https://chromium.googlesource.com/chromium/src/+/33e60b30fadf6d9b3b129a884e8b1b2eeec79988
Patch Set 1 #Patch Set 2 : subframe filtering #Patch Set 3 : fix test names #
Total comments: 18
Patch Set 4 : pkalinnikov review #Patch Set 5 : clean up + add subsubframe test #Patch Set 6 : Augment Call*ForTesting methods #Patch Set 7 : fix use after stack return, make code nicer #
Total comments: 18
Patch Set 8 : engedy review #Patch Set 9 : s/proceed2/allowed2/ (trybots prev) #
Total comments: 3
Patch Set 10 : rebase #Patch Set 11 : slight reworking #Patch Set 12 : rebase #Patch Set 13 : remove tests #
Messages
Total messages: 61 (39 generated)
Description was changed from ========== Introduce SubframeBlockingNavigationThrottle BUG=637415 ========== to ========== Introduce SubframeBlockingNavigationThrottle BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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 ========== Introduce SubframeBlockingNavigationThrottle BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce SubframeFilteringNavigationThrottle BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Introduce SubframeFilteringNavigationThrottle BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce SubframeFilteringNavigationThrottle As of now, it is never injected into the throttle list and is only exercised in unit tests. BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
csharrison@chromium.org changed reviewers: + engedy@chromium.org
Hey engedy@, would you take a look at this at a high level? It differs slightly from the design in some places. I was planning on having factory methods in the Driver that MaybeCreate these throttles, as the Driver will have some more global knowledge (e.g. that the parent filter is active).
engedy@chromium.org changed reviewers: + pkalinnikov@chromium.org
Pavel, could you please take a first look?
Some initial comments. Thanks. https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter.cc (right): https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter.cc:131: if (activation_state.activation_level != ActivationLevel::DISABLED) { You can remove this check, I have modified to dependee CL. https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... File components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.cc (right): https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.cc:15: AsyncDocumentSubresourceFilter* async_filter) nit: Rename to |parent_filter| too (see comment in .h file). https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.cc:50: content::NavigationThrottle::CANCEL); We should also to call parent_filter_->ReportDisallowedLoad() here to trigger the UI. https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... File components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.h (right): https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.h:23: // created for subframe navigations occuring in document's which are already nit: s/document's/documents/ https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.h:24: // activated and have associated DocumentSubresourceFilters. Phrasing suggestion: NavigationThrottle responsible for determining activation levels for direct subframes of a given |parent| frame (either main frame of a subframe itself), and cancelling disallowed loads. This class should only be instantiated for subframe navigations occuring in documents which already have filtering activated and an associated (Async)DocumentSubresourceFilter. https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.h:29: AsyncDocumentSubresourceFilter* async_filter); nit: Please call this consistently with the stored pointer. How about both being |parent_filter|? https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... File components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc:39: test_handle_.reset(); Don't you need to reset the other handles here, e.g., |ruleset_handle_|? If I understand correctly, currently the task to delete their counterparts is posted to the |blocking_task_runner_| on destruction of the test fixture. But the task runner is not RunUntilIdle on destruction, so this leads to a memory leak. I remember some of the "asan" trybots catching this for me, just git cl try it. https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc:55: // posted. nit: until the task is run. https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc:190: TEST_F(SubframeFilteringNavigationThrottleTest, NeverFilterWithInactiveParent) { This test is probably not needed any more because it's not possible to create an inactive ADSF.
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
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review. I'm going to try to fix the //content public interface to allow a better pattern than IsCancellingForTesting, but feel free to review non //content parts for now. https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter.cc (right): https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter.cc:131: if (activation_state.activation_level != ActivationLevel::DISABLED) { On 2017/02/14 12:11:06, pkalinnikov wrote: > You can remove this check, I have modified to dependee CL. Done. https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... File components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.cc (right): https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.cc:15: AsyncDocumentSubresourceFilter* async_filter) On 2017/02/14 12:11:07, pkalinnikov wrote: > nit: Rename to |parent_filter| too (see comment in .h file). Done. https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.cc:50: content::NavigationThrottle::CANCEL); On 2017/02/14 12:11:07, pkalinnikov wrote: > We should also to call parent_filter_->ReportDisallowedLoad() here to trigger > the UI. Done. https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... File components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.h (right): https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.h:23: // created for subframe navigations occuring in document's which are already On 2017/02/14 12:11:07, pkalinnikov wrote: > nit: s/document's/documents/ Done. https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.h:24: // activated and have associated DocumentSubresourceFilters. On 2017/02/14 12:11:07, pkalinnikov wrote: > Phrasing suggestion: > > NavigationThrottle responsible for determining activation levels for direct > subframes of a given |parent| frame (either main frame of a subframe itself), > and cancelling disallowed loads. This class should only be instantiated for > subframe navigations occuring in documents which already have filtering > activated and an associated (Async)DocumentSubresourceFilter. Reworded even more, because this class *doesn't* determine activation levels... https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.h:29: AsyncDocumentSubresourceFilter* async_filter); On 2017/02/14 12:11:07, pkalinnikov wrote: > nit: Please call this consistently with the stored pointer. How about both being > |parent_filter|? Done. https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... File components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc:39: test_handle_.reset(); On 2017/02/14 12:11:07, pkalinnikov wrote: > Don't you need to reset the other handles here, e.g., |ruleset_handle_|? > > If I understand correctly, currently the task to delete their counterparts is > posted to the |blocking_task_runner_| on destruction of the test fixture. But > the task runner is not RunUntilIdle on destruction, so this leads to a memory > leak. I remember some of the "asan" trybots catching this for me, just git cl > try it. Yep, my non-cosmetic patch (PS1) did catch this problem. I've reset the other handle's and run the runner :) https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc:55: // posted. On 2017/02/14 12:11:07, pkalinnikov wrote: > nit: until the task is run. Done. https://codereview.chromium.org/2696493003/diff/40001/components/subresource_... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc:190: TEST_F(SubframeFilteringNavigationThrottleTest, NeverFilterWithInactiveParent) { On 2017/02/14 12:11:07, pkalinnikov wrote: > This test is probably not needed any more because it's not possible to create an > inactive ADSF. Done.
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 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: + clamy@chromium.org
+clamy: Here is my initial go at augmenting Call*ForTesting methods on NavigationHandle. LMK what you think. I debated with myself if a caller's complete_callback should only run if the checks complete asynchronously, but decided that consistency is better.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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: This issue passed the CQ dry run.
Thanks a lot, really appreciate the thorough unit tests! subresource_filter parts LGTM % comments. https://codereview.chromium.org/2696493003/diff/120001/components/subresource... File components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.cc (right): https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.cc:19: DCHECK(!handle->IsInMainFrame()); nit: #include "base/logging.h" https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.cc:49: navigation_handle()->CancelDeferredNavigation( Should we swap this with the next line? A comment above CancelDeferredNavigation warns that it might lead to deleting the NavigationHandle, and the destruction of all NavigationThrottles. https://codereview.chromium.org/2696493003/diff/120001/components/subresource... File components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.h (right): https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.h:21: // NavigationThrottle responsible for filtering direct subframe navigations Comment wording ideas: NavigationThrottle responsible for filtering subframe document loads, which are considered subresource loads of their parent frame, hence are subject to subresource filtering using the parent frame's AsyncDocumentSubresourceFilter. The throttle should only be instantiated for navigations occuring in subframes owned by documents which already have filtering activated, and therefore an associated (Async)DocumentSubresourceFilter. https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.h:25: class SubframeFilteringNavigationThrottle : public content::NavigationThrottle { This name is somewhat ambiguous, what do you think about: SubframeNavigationFilteringThrottle, or SubframeDocumentFilteringThrottle? https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.h:29: AsyncDocumentSubresourceFilter* parent_filter); nit: How about calling this more specifically parent_frame_filter? https://codereview.chromium.org/2696493003/diff/120001/components/subresource... File components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc:61: "filter.html", &test_ruleset_pair_)); nit: disallowed.html https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc:82: void ExpectActivation(ActivationState expected, ActivationState state) { nit for future: This was needed in the tests for the ADSF too. In a follow-up CL, it might be good to provide test_support for the Handles with utilities for doing this. https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc:134: SimulateWillProcessResponse() { nit: For consistency, how about making this SimulateWillProcessResponseAndExpectResult()? https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc:167: CreateTestSubframeNavigation(GURL("https://example.test/proceed.html"), nit: allowed.html
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...
Thanks! https://codereview.chromium.org/2696493003/diff/120001/components/subresource... File components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.cc (right): https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.cc:19: DCHECK(!handle->IsInMainFrame()); On 2017/02/14 22:04:33, engedy wrote: > nit: #include "base/logging.h" Done. https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.cc:49: navigation_handle()->CancelDeferredNavigation( On 2017/02/14 22:04:32, engedy wrote: > Should we swap this with the next line? A comment above > CancelDeferredNavigation warns that it might lead to deleting the > NavigationHandle, and the destruction of all NavigationThrottles. Good catch. This would get caught in a browser test, but not these unit tests. I'll make sure to catch this case in a browser test suite in a follow up (possibly, when we wire these all together). https://codereview.chromium.org/2696493003/diff/120001/components/subresource... File components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.h (right): https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.h:21: // NavigationThrottle responsible for filtering direct subframe navigations On 2017/02/14 22:04:33, engedy wrote: > Comment wording ideas: > > NavigationThrottle responsible for filtering subframe document loads, which are > considered subresource loads of their parent frame, hence are subject to > subresource filtering using the parent frame's AsyncDocumentSubresourceFilter. > > The throttle should only be instantiated for navigations occuring in subframes > owned by documents which already have filtering activated, and therefore an > associated (Async)DocumentSubresourceFilter. Nice, done https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.h:25: class SubframeFilteringNavigationThrottle : public content::NavigationThrottle { On 2017/02/14 22:04:33, engedy wrote: > This name is somewhat ambiguous, what do you think about: > > SubframeNavigationFilteringThrottle, or > SubframeDocumentFilteringThrottle? I prefer SubframeNavigationFilteringThrottle, changed it. https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle.h:29: AsyncDocumentSubresourceFilter* parent_filter); On 2017/02/14 22:04:33, engedy wrote: > nit: How about calling this more specifically parent_frame_filter? Done. https://codereview.chromium.org/2696493003/diff/120001/components/subresource... File components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc:61: "filter.html", &test_ruleset_pair_)); On 2017/02/14 22:04:33, engedy wrote: > nit: disallowed.html Done. https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc:82: void ExpectActivation(ActivationState expected, ActivationState state) { On 2017/02/14 22:04:33, engedy wrote: > nit for future: This was needed in the tests for the ADSF too. In a follow-up > CL, it might be good to provide test_support for the Handles with utilities for > doing this. Good idea. I will do this for the activation computing throttle CL, which also wants to do this :D https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc:134: SimulateWillProcessResponse() { On 2017/02/14 22:04:33, engedy wrote: > nit: For consistency, how about making this > SimulateWillProcessResponseAndExpectResult()? Done. https://codereview.chromium.org/2696493003/diff/120001/components/subresource... components/subresource_filter/content/browser/subframe_filtering_navigation_throttle_unittest.cc:167: CreateTestSubframeNavigation(GURL("https://example.test/proceed.html"), On 2017/02/14 22:04:33, engedy wrote: > nit: allowed.html Done.
LGTM for subresource_filter.
Please don't forget to update the CL title and description with the new name.
Description was changed from ========== Introduce SubframeFilteringNavigationThrottle As of now, it is never injected into the throttle list and is only exercised in unit tests. BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce SubframeNavigationFilteringThrottle As of now, it is never injected into the throttle list and is only exercised in unit tests. This patch also augments NavigationHandle::Call*ForTesting methods, which now take an optionally-null callback that will always be called with the result of the navigation throttle checks, even if they are calculated asynchronously. BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
On 2017/02/15 09:31:28, engedy wrote: > Please don't forget to update the CL title and description with the new name. Done.
Thanks! A few comments below. https://codereview.chromium.org/2696493003/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2696493003/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:233: NavigationThrottle::ThrottleCheckResult WillStartRequest( I'm don't really like having the result be returned in two different ways, 1) directly and 2) via the callback being run. It looks confusing to me.
https://codereview.chromium.org/2696493003/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2696493003/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:233: NavigationThrottle::ThrottleCheckResult WillStartRequest( On 2017/02/15 16:37:22, clamy wrote: > I'm don't really like having the result be returned in two different ways, 1) > directly and 2) via the callback being run. It looks confusing to me. What would you prefer? One idea is to have the callback run only in async cases, but keep everything else as-is. I.e. we know the callback will be run later if the method returns DEFER.
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_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2696493003/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2696493003/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:233: NavigationThrottle::ThrottleCheckResult WillStartRequest( On 2017/02/15 16:42:12, Charlie Harrison wrote: > On 2017/02/15 16:37:22, clamy wrote: > > I'm don't really like having the result be returned in two different ways, 1) > > directly and 2) via the callback being run. It looks confusing to me. > > What would you prefer? One idea is to have the callback run only in async cases, > but keep everything else as-is. I.e. we know the callback will be run later if > the method returns DEFER. I really don't want to be modifying the semantic of the callback right now. We will do it in the future though, where either NavigationHandle will be merged in the NavigationRequest or NavigationRequest methods will be called directly by NavigationHandle. This will result in the callback disappearing. So thinking about this future direction, I see the following possible solutions: 1) pass a pointer (possibly weak ptr) to a ThrottleCheckResult that will be updated when we are done with the checks. We can internally bind the callback to update this pointer in tests. 2) add new functions ProcessAll*Checks that will only return when all checks are done. It need not be on the handler (can be a RFH function or something), or be added to the NavigationSimulator I'm adding in that patch: https://codereview.chromium.org/2682313002/.
I've removed the edits to WillStartRequest, etc. and made this CL only touch the testing method. Let me know if you think this is an ok short-term workaround. I took a look at augmenting the NavigationSimulator to do this task, keeping in mind that the callback in NavigationHandle might go away and lead to direct calls into the NavigationRequest. The problem with going "callback"-less is that we have no real means of introspecting the throttle check result (including CANCEL/BLOCK variants). There are some workarounds you can do if you constantly post tasks asking about some internal value, but even that doesn't give us anything more specific than NavigationHandleImpl::state_for_testing. It also is very hacky and I would like to avoid it if possible. My impression is that once we have direct calls into NavigationRequest we'll have to just add a callback interface there to support this use case. Note that we already have one of these: "set_on_start_checks_complete_closure_for_testing" For now though, if we were to support both PlzNavigate and non-PlzNavigate flows, it seems like we'd have to have both callback interfaces (with the way things are set up in the Simulator). This is probably undesirable. WDYT?
On 2017/02/16 19:00:47, Charlie Harrison wrote: > I've removed the edits to WillStartRequest, etc. and made this CL only touch the > testing method. Let me know if you think this is an ok short-term workaround. > > I took a look at augmenting the NavigationSimulator to do this task, keeping in > mind that the callback in NavigationHandle might go away and lead to direct > calls into the NavigationRequest. The problem with going "callback"-less is that > we have no real means of introspecting the throttle check result (including > CANCEL/BLOCK variants). There are some workarounds you can do if you constantly > post tasks asking about some internal value, but even that doesn't give us > anything more specific than NavigationHandleImpl::state_for_testing. It also is > very hacky and I would like to avoid it if possible. > > My impression is that once we have direct calls into NavigationRequest we'll > have to just add a callback interface there to support this use case. Note that > we already have one of these: "set_on_start_checks_complete_closure_for_testing" > > > For now though, if we were to support both PlzNavigate and non-PlzNavigate > flows, it seems like we'd have to have both callback interfaces (with the way > things are set up in the Simulator). This is probably undesirable. > > WDYT? Stepping back, the minimum that I need is an interface on NavigationSimulator that tells me if the navigation was cancelled, via asynchronous navigation throttle calls. This is something we can implement using the current design of the NavigatorSimulator, with the following additions: - Optional: Support registering test throttles. The test harness could do that but then all test harnesses that want this behavior would have to be WebContentsObservers and inject the throttles at DidStartNavigation - Support a "non auto-advance mode" for throttle checks. TBH I think this should be the default, with a method on the NavigationSimulator to SkipAsynchronousThrottleChecks(). - Make sure that a throttle CANCELing a navigation actually cancels it in the simulator, and we can retrieve that status after the fact (via the simulator or DidFinishNavigation). I can't tell if the current implementation supports this.
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: This issue passed the CQ dry run.
engedy, pkalinnikov, I am going to break this CL apart and land the tests separately to unblock myself from a long chain of dependent CLs. Does that sound okay to you? Currently the tests are waiting on the NavigationSimulator infrastructure.
On 2017/02/27 21:03:48, Charlie Harrison wrote: > engedy, pkalinnikov, I am going to break this CL apart and land the tests > separately to unblock myself from a long chain of dependent CLs. > > Does that sound okay to you? Currently the tests are waiting on the > NavigationSimulator infrastructure. SGTM.
csharrison@chromium.org changed reviewers: - clamy@chromium.org
Thanks! Note that I am also planning on doing this for the activation throttle if that's ok. Once they all land, the test CLs will all have a single parent dependency (the test infra), rather than a linear chain like it is now. clamy: moving you to cc as this doesn't touch content anymore. I'll upload a new PS for the simulator shortly, as I have confirmed it works for these tests with a few adjustments.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org, pkalinnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2696493003/#ps240001 (title: "remove tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/27 21:12:03, Charlie Harrison wrote: > Thanks! Note that I am also planning on doing this for the activation throttle > if that's ok. Once they all land, the test CLs will all have a single parent > dependency (the test infra), rather than a linear chain like it is now. > > clamy: moving you to cc as this doesn't touch content anymore. I'll upload a new > PS for the simulator shortly, as I have confirmed it works for these tests with > a few adjustments. Yes, that makes a lot of sense to me, given that the code is not used in production yet.
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1488229929614850,
"parent_rev": "6be7f531cadb6afd9d953ffae9a0572de0ee766f", "commit_rev":
"33e60b30fadf6d9b3b129a884e8b1b2eeec79988"}
Message was sent while issue was closed.
Description was changed from ========== Introduce SubframeNavigationFilteringThrottle As of now, it is never injected into the throttle list and is only exercised in unit tests. This patch also augments NavigationHandle::Call*ForTesting methods, which now take an optionally-null callback that will always be called with the result of the navigation throttle checks, even if they are calculated asynchronously. BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce SubframeNavigationFilteringThrottle As of now, it is never injected into the throttle list and is only exercised in unit tests. This patch also augments NavigationHandle::Call*ForTesting methods, which now take an optionally-null callback that will always be called with the result of the navigation throttle checks, even if they are calculated asynchronously. BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2696493003 Cr-Commit-Position: refs/heads/master@{#453375} Committed: https://chromium.googlesource.com/chromium/src/+/33e60b30fadf6d9b3b129a884e8b... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/33e60b30fadf6d9b3b129a884e8b...
Message was sent while issue was closed.
Description was changed from ========== Introduce SubframeNavigationFilteringThrottle As of now, it is never injected into the throttle list and is only exercised in unit tests. This patch also augments NavigationHandle::Call*ForTesting methods, which now take an optionally-null callback that will always be called with the result of the navigation throttle checks, even if they are calculated asynchronously. BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2696493003 Cr-Commit-Position: refs/heads/master@{#453375} Committed: https://chromium.googlesource.com/chromium/src/+/33e60b30fadf6d9b3b129a884e8b... ========== to ========== Introduce SubframeNavigationFilteringThrottle As of now, it is never injected into the throttle list. Unit tests coming in a followup. BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2696493003 Cr-Commit-Position: refs/heads/master@{#453375} Committed: https://chromium.googlesource.com/chromium/src/+/33e60b30fadf6d9b3b129a884e8b... ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
