Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697703002:
Introduce ActivationStateComputingNavigationThrottle (Closed)
|
Created:
3 years, 10 months ago by Charlie Harrison Modified:
3 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, subresource-filter-reviews_chromium.org, pkalinnikov Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce ActivationStateComputingNavigationThrottle
This throttle is designed to be after the
SubframeNavigationFilteringThrottle, and will calculate activation state
for the navigating frame on WillProcessResponse.
BUG=637415
Review-Url: https://codereview.chromium.org/2697703002
Cr-Commit-Position: refs/heads/master@{#453391}
Committed: https://chromium.googlesource.com/chromium/src/+/b30a80aa9cbf4a1881912da54484f948afdadf0c
Patch Set 1 #Patch Set 2 : Introduce ActivationStateComputingNavigationThrottle #Patch Set 3 : unit test refactor #
Total comments: 10
Patch Set 4 : rebase + respond to comments #
Total comments: 48
Patch Set 5 : respond to comments #Patch Set 6 : rebase #Patch Set 7 : remove unit tests #Patch Set 8 : remove unit tests #
Messages
Total messages: 49 (36 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
csharrison@chromium.org changed reviewers: + engedy@chromium.org, pkalinnikov@chromium.org
engedy: could you PTAL at this throttle? This patch also refactors unit tests expecting ADSF activation out to a separate ADSF test utils file. +pkalinnikov: I ended up not using the initialization param constructors you wrote because: - I didn't want the params as a class member, bloating the object size for all main frame navigations. - Main frame navigations get their activation state plumbed in asynchronously, so we must persist some of it. Persisting as an ActivationState seemed cleaner than an additional bool + ActivationLevel.
Description was changed from ========== Introduce ActivationStateComputingNavigationThrottle BUG=637415 ========== to ========== Introduce ActivationStateComputingNavigationThrottle This throttle is designed to be after the SubframeNavigationFilteringThrottle, and will calculate activation state for the navigating frame on WillProcessResponse. This patch also refactors some unit tests which use a similar pattern for expecting activation from an AsyncDocumentSubresourceFilter. BUG=637415 ==========
Note: patch errors expected, it's due to the long chain of dependent CLs. Tests are passing locally.
Some initial comments. Balazs, please make a round as well. https://codereview.chromium.org/2697703002/diff/40001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc (right): https://codereview.chromium.org/2697703002/diff/40001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc:21: return base::WrapUnique(new ActivationStateComputingNavigationThrottle( nit: Prefer MakeUnique to WrapUnique (here and below). https://codereview.chromium.org/2697703002/diff/40001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc:94: base::OnceClosure()); Please leave TODO to replace the empty OnceClosure with a UI-triggering callback. https://codereview.chromium.org/2697703002/diff/40001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h (right): https://codereview.chromium.org/2697703002/diff/40001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:26: // given frame (including main frames and subframes). The navigation is paused nit: s/paused/deferred/ https://codereview.chromium.org/2697703002/diff/40001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:65: std::unique_ptr<AsyncDocumentSubresourceFilter> TakeFilter(); nit: Semantically speaking, I would call this ReleaseFilter(), and modify the comment in a way that it clearly states that the ADSF is not owned by the throttle any more. https://codereview.chromium.org/2697703002/diff/40001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h (right): https://codereview.chromium.org/2697703002/diff/40001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h:29: ActivationState activation_state_; nit: last_activation_state_ You probably should rebase your CL chain, because the ADSF CL changed significantly (including this member name).
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...
Thank you! https://codereview.chromium.org/2697703002/diff/40001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc (right): https://codereview.chromium.org/2697703002/diff/40001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc:21: return base::WrapUnique(new ActivationStateComputingNavigationThrottle( On 2017/02/15 20:21:42, pkalinnikov wrote: > nit: Prefer MakeUnique to WrapUnique (here and below). Sadly, you can't use MakeUnique with a private constructor, see the chromium-dev thread: https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/iQgMedVA8-k... https://codereview.chromium.org/2697703002/diff/40001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc:94: base::OnceClosure()); On 2017/02/15 20:21:42, pkalinnikov wrote: > Please leave TODO to replace the empty OnceClosure with a UI-triggering > callback. Done. https://codereview.chromium.org/2697703002/diff/40001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h (right): https://codereview.chromium.org/2697703002/diff/40001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:26: // given frame (including main frames and subframes). The navigation is paused On 2017/02/15 20:21:42, pkalinnikov wrote: > nit: s/paused/deferred/ Done. https://codereview.chromium.org/2697703002/diff/40001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:65: std::unique_ptr<AsyncDocumentSubresourceFilter> TakeFilter(); On 2017/02/15 20:21:42, pkalinnikov wrote: > nit: Semantically speaking, I would call this ReleaseFilter(), and modify the > comment in a way that it clearly states that the ADSF is not owned by the > throttle any more. Done. https://codereview.chromium.org/2697703002/diff/40001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h (right): https://codereview.chromium.org/2697703002/diff/40001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h:29: ActivationState activation_state_; On 2017/02/15 20:21:42, pkalinnikov wrote: > nit: last_activation_state_ > > You probably should rebase your CL chain, because the ADSF CL changed > significantly (including this member name). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Looks great! First batch of comments, mostly nits and phrasing suggestions. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h (right): https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:17: namespace content { nit: I think this should not be needed, we are only using the NavigationHandle in places where the base class uses it, so it necessarily has to be included or forward-declared. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:25: // NavigationThrottle responsible for determining the activation state for a Phrasing suggestion: ... the activation state of subresource filtering for a given navigation (either in the main frame or in a subframe); and for deferring that navigation at WillProcessResponse until the activation state computation on the blocking pool thread is complete. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:33: // For main frames, a verified ruleset handle is usually not available. Since nit: ... not readily available at construction time. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:34: // it is expensive to "warm up" the ruleset, this object will have it injected Phrasing suggestion: s/this object will have it/the ruleset handle will be/ https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:35: // in NotifyPageActivationWithRuleset when it is decided that activation nit: s/when it is decided/once it has been established/ https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:52: // called, or it is called with a DISABLED state, this object remains a simple nits: -- This object is always a pass-through throttle, but one that potentially delays the navigation, let's emphasize that. -- Let's emphasize in this comment that this method must be called before WillProcessResponse is invoked on *this* throttle. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:56: const ActivationState& page_activation_state); @Pavel, do you prefer taking ActivationState here, rather than an ActivationLevel + bool (measure_performance)? https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:62: // After the navigation is finished, the subresource filter can have its Phrasing suggestion: After the navigation is finished, the client may optionally choose to continue using the DocumentSubresourceFilter that was used to compute the activation state for this frame. The ... https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:69: // which will most likely happen in ReadyToCommitNavigation. which, for example, will have happened at ReadyToCommitNavigation. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:88: // null until NotifyPageActivationWithRuleset is called. nit: nullptr https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h (right): https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h:12: #include "testing/gtest/include/gtest/gtest.h" nit: This is only used in the .cc file. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h:14: namespace subresource_filter { nit: For consistency with other test support, let's put this into subresource_filter::testing. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h:24: void ExpectReceivedOnce(ActivationState expected_state) const; nit: Can this be a const&?
https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h (right): https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:56: const ActivationState& page_activation_state); On 2017/02/16 10:47:39, engedy wrote: > @Pavel, do you prefer taking ActivationState here, rather than an > ActivationLevel + bool (measure_performance)? I think it's fine like this. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:64: // calculateload policy for subframe navigations occuring in this frame. nit: ... calculate load ...
Thanks a lot! LGTM % comments and sign-off from PlzNavigate folks on how we are testing the throttles. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc (right): https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc:6: nit: #include <utility> for std::move https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc:8: #include "base/callback_forward.h" nit: Hmm, I would have expected that we need the complete definition here? https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc:85: ->GetRenderFrameHost() Could you please check with Camille if this will continue to work with PlzNavigate in the future? While currently it indeed looks like that we will always have a RFH by this point, there is an ominous comment in the implementation proposing to CHECK that this is called in ReadyToCommitNavigation at the earliest. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:7: #include <memory> #include <utility> for std::move https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:94: void CreateTestNavigationForSubframe( style: blank line before https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:171: InitializeDocumentSubresourceFilter(GURL("http://example.test")); Hang on, we shouldn't need a DSF for these tests. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:192: CreateTestNavigationForMainFrame(GURL("http://example.test/activate.html")); nit: inactivate.html? Same below. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:231: // Not really a child frame, so whitelist should not apply. I'm not sure I fully understand this test -- could you please clarify? https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:283: } // namespace subresource_filter Let's have two more tests here: 1.) One that verifies that a DRYRUN level is correctly propagated, 2.) One more that verifies that `false` values for |disabled_for_document| and |generic_blocking_rules_disabled| from the parent are properly propagated even if there are no rules to explicitly whitelist the child.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
csharrison@chromium.org changed reviewers: + clamy@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
+clamy, there is one question for you regarding accessing a NavigationHandle's RFH at WillProcessResponse time. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc (right): https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc:6: On 2017/02/16 14:14:30, engedy wrote: > nit: #include <utility> for std::move Done. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc:8: #include "base/callback_forward.h" On 2017/02/16 14:14:30, engedy wrote: > nit: Hmm, I would have expected that we need the complete definition here? You're right we do. I think we get it from the async dsf. Good catch. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc:85: ->GetRenderFrameHost() On 2017/02/16 14:14:30, engedy wrote: > Could you please check with Camille if this will continue to work with > PlzNavigate in the future? > > While currently it indeed looks like that we will always have a RFH by this > point, there is an ominous comment in the implementation proposing to CHECK that > this is called in ReadyToCommitNavigation at the earliest. +clamy to answer this question. I think if we need to we can use the frame tree node id trick here. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h (right): https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:17: namespace content { On 2017/02/16 10:47:39, engedy wrote: > nit: I think this should not be needed, we are only using the NavigationHandle > in places where the base class uses it, so it necessarily has to be included or > forward-declared. Done. I've had reviewers in the past ask for this but I agree with you. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:25: // NavigationThrottle responsible for determining the activation state for a On 2017/02/16 10:47:39, engedy wrote: > Phrasing suggestion: ... the activation state of subresource filtering for a > given navigation (either in the main frame or in a subframe); and for deferring > that navigation at WillProcessResponse until the activation state computation on > the blocking pool thread is complete. Done. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:33: // For main frames, a verified ruleset handle is usually not available. Since On 2017/02/16 10:47:39, engedy wrote: > nit: ... not readily available at construction time. Done. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:34: // it is expensive to "warm up" the ruleset, this object will have it injected On 2017/02/16 10:47:39, engedy wrote: > Phrasing suggestion: s/this object will have it/the ruleset handle will be/ Done. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:35: // in NotifyPageActivationWithRuleset when it is decided that activation On 2017/02/16 10:47:39, engedy wrote: > nit: s/when it is decided/once it has been established/ Done. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:52: // called, or it is called with a DISABLED state, this object remains a simple On 2017/02/16 10:47:39, engedy wrote: > nits: > -- This object is always a pass-through throttle, but one that potentially > delays the navigation, let's emphasize that. > -- Let's emphasize in this comment that this method must be called before > WillProcessResponse is invoked on *this* throttle. Reworded. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:62: // After the navigation is finished, the subresource filter can have its On 2017/02/16 10:47:39, engedy wrote: > Phrasing suggestion: > > After the navigation is finished, the client may optionally choose to continue > using the DocumentSubresourceFilter that was used to compute the activation > state for this frame. The ... Done. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:64: // calculateload policy for subframe navigations occuring in this frame. On 2017/02/16 13:29:04, pkalinnikov wrote: > nit: ... calculate load ... Done. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:69: // which will most likely happen in ReadyToCommitNavigation. On 2017/02/16 10:47:39, engedy wrote: > which, for example, will have happened at ReadyToCommitNavigation. Done. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:88: // null until NotifyPageActivationWithRuleset is called. On 2017/02/16 10:47:39, engedy wrote: > nit: nullptr Done. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:7: #include <memory> On 2017/02/16 14:14:30, engedy wrote: > #include <utility> for std::move Done. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:94: void CreateTestNavigationForSubframe( On 2017/02/16 14:14:30, engedy wrote: > style: blank line before Done. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:171: InitializeDocumentSubresourceFilter(GURL("http://example.test")); On 2017/02/16 14:14:30, engedy wrote: > Hang on, we shouldn't need a DSF for these tests. Done. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:192: CreateTestNavigationForMainFrame(GURL("http://example.test/activate.html")); On 2017/02/16 14:14:30, engedy wrote: > nit: inactivate.html? Same below. Done. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:231: // Not really a child frame, so whitelist should not apply. On 2017/02/16 14:14:30, engedy wrote: > I'm not sure I fully understand this test -- could you please clarify? This test is mainly testing that the parent-child whitelist does not apply for frames that are not parent-child. Happy to remove if you think it isn't relevant to this throttle. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:283: } // namespace subresource_filter On 2017/02/16 14:14:30, engedy wrote: > Let's have two more tests here: > 1.) One that verifies that a DRYRUN level is correctly propagated, > 2.) One more that verifies that `false` values for |disabled_for_document| and > |generic_blocking_rules_disabled| from the parent are properly propagated even > if there are no rules to explicitly whitelist the child. do you mean "true" values? I've added a test for disabled document and disabled generic. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h (right): https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h:12: #include "testing/gtest/include/gtest/gtest.h" On 2017/02/16 10:47:40, engedy wrote: > nit: This is only used in the .cc file. Done. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h:14: namespace subresource_filter { On 2017/02/16 10:47:39, engedy wrote: > nit: For consistency with other test support, let's put this into > subresource_filter::testing. Done. https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h:24: void ExpectReceivedOnce(ActivationState expected_state) const; On 2017/02/16 10:47:40, engedy wrote: > nit: Can this be a const&? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc (right): https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc:85: ->GetRenderFrameHost() On 2017/02/16 16:59:44, Charlie Harrison wrote: > On 2017/02/16 14:14:30, engedy wrote: > > Could you please check with Camille if this will continue to work with > > PlzNavigate in the future? > > > > While currently it indeed looks like that we will always have a RFH by this > > point, there is an ominous comment in the implementation proposing to CHECK > that > > this is called in ReadyToCommitNavigation at the earliest. > > +clamy to answer this question. I think if we need to we can use the frame tree > node id trick here. Actually it's not guaranteed that you have one, and it could change. Two main caveats: - if the navigation will not commit (eg 204) we don't actually set the RFH in PlzNavigate. - if the navigation transfers in the current architecture, you will have the RFH that initiated the navigation and not the one that it will commit in. I think it'd be safer to use the FTN id if you just want to access teh parent origin.
https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc (right): https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc:85: ->GetRenderFrameHost() On 2017/02/21 13:05:11, clamy wrote: > On 2017/02/16 16:59:44, Charlie Harrison wrote: > > On 2017/02/16 14:14:30, engedy wrote: > > > Could you please check with Camille if this will continue to work with > > > PlzNavigate in the future? > > > > > > While currently it indeed looks like that we will always have a RFH by this > > > point, there is an ominous comment in the implementation proposing to CHECK > > that > > > this is called in ReadyToCommitNavigation at the earliest. > > > > +clamy to answer this question. I think if we need to we can use the frame > tree > > node id trick here. > > Actually it's not guaranteed that you have one, and it could change. Two main > caveats: > - if the navigation will not commit (eg 204) we don't actually set the RFH in > PlzNavigate. > - if the navigation transfers in the current architecture, you will have the RFH > that initiated the navigation and not the one that it will commit in. > > I think it'd be safer to use the FTN id if you just want to access teh parent > origin. Thanks for the clarification!
On 2017/02/21 13:48:53, engedy (slow) wrote: > https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... > File > components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc > (right): > > https://codereview.chromium.org/2697703002/diff/60001/components/subresource_... > components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc:85: > ->GetRenderFrameHost() > On 2017/02/21 13:05:11, clamy wrote: > > On 2017/02/16 16:59:44, Charlie Harrison wrote: > > > On 2017/02/16 14:14:30, engedy wrote: > > > > Could you please check with Camille if this will continue to work with > > > > PlzNavigate in the future? > > > > > > > > While currently it indeed looks like that we will always have a RFH by > this > > > > point, there is an ominous comment in the implementation proposing to > CHECK > > > that > > > > this is called in ReadyToCommitNavigation at the earliest. > > > > > > +clamy to answer this question. I think if we need to we can use the frame > > tree > > > node id trick here. > > > > Actually it's not guaranteed that you have one, and it could change. Two main > > caveats: > > - if the navigation will not commit (eg 204) we don't actually set the RFH in > > PlzNavigate. > > - if the navigation transfers in the current architecture, you will have the > RFH > > that initiated the navigation and not the one that it will commit in. > > > > I think it'd be safer to use the FTN id if you just want to access teh parent > > origin. > > Thanks for the clarification! I've updated the code to use the FTN pattern, and I've removed the unit tests for now. Will go ahead and land as discussed on the subframe filtering CL.
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 Link to the patchset: https://codereview.chromium.org/2697703002/#ps130001 (title: "remove unit tests")
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 ActivationStateComputingNavigationThrottle This throttle is designed to be after the SubframeNavigationFilteringThrottle, and will calculate activation state for the navigating frame on WillProcessResponse. This patch also refactors some unit tests which use a similar pattern for expecting activation from an AsyncDocumentSubresourceFilter. BUG=637415 ========== to ========== Introduce ActivationStateComputingNavigationThrottle This throttle is designed to be after the SubframeNavigationFilteringThrottle, and will calculate activation state for the navigating frame on WillProcessResponse. BUG=637415 ==========
CQ is committing da patch.
Bot data: {"patchset_id": 130001, "attempt_start_ts": 1488233959618030,
"parent_rev": "11e15a648963d343125c2bb95369f4bfa178ea69", "commit_rev":
"b30a80aa9cbf4a1881912da54484f948afdadf0c"}
Message was sent while issue was closed.
Description was changed from ========== Introduce ActivationStateComputingNavigationThrottle This throttle is designed to be after the SubframeNavigationFilteringThrottle, and will calculate activation state for the navigating frame on WillProcessResponse. BUG=637415 ========== to ========== Introduce ActivationStateComputingNavigationThrottle This throttle is designed to be after the SubframeNavigationFilteringThrottle, and will calculate activation state for the navigating frame on WillProcessResponse. BUG=637415 Review-Url: https://codereview.chromium.org/2697703002 Cr-Commit-Position: refs/heads/master@{#453391} Committed: https://chromium.googlesource.com/chromium/src/+/b30a80aa9cbf4a1881912da54484... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001) as https://chromium.googlesource.com/chromium/src/+/b30a80aa9cbf4a1881912da54484... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
