|
|
DescriptionSend activation signal to subframes.
BUG=609747, 641035
Committed: https://crrev.com/73880b6945b9cc635396db030426a21c825669f5
Cr-Commit-Position: refs/heads/master@{#415179}
Patch Set 1 #Patch Set 2 : cl format #Patch Set 3 : fix unittests #
Total comments: 3
Patch Set 4 : rebased #Patch Set 5 : . #Patch Set 6 : .. #
Total comments: 4
Patch Set 7 : special case test #
Total comments: 9
Patch Set 8 : embedded tests server #Patch Set 9 : fix test after rebase #Patch Set 10 : . #Patch Set 11 : cleanup #
Messages
Total messages: 64 (46 generated)
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: This issue passed the CQ dry run.
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_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by 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: This issue passed the CQ dry run.
melandory@chromium.org changed reviewers: + engedy@chromium.org
PTAL, thanks!
Please see some initial comments. https://codereview.chromium.org/2243863002/diff/40001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2243863002/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:159: driver->ActivateForProvisionalLoad(GetMaximumActivationState()); Do we need to reset the activation flag on the mainframe? Suppose host A is on the activation list, but B is not. User navigates to A, then to B. Are we going to enable filtering in subframes on B? I wonder if it would be cleaner to store a single flag in this WebContentsObserver, which is reset on the start of every provisional load in the mainframe, then set accordingly, and is then consulted for all frames. https://codereview.chromium.org/2243863002/diff/40001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2243863002/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:336: TEST_F(ContentSubresourceFilterDriverFactoryTest, Can we reduce the amount of boilerplate in these tests, so that it is easier to identify the test scenarios and expectations? For instance, we could have a bunch of methods in the test fixture that implement larger logical testing steps, such as simulating a provisional load, committing that load, or simulating the birth of two subframes. Then, we could write the tests more concisely in terms of executing operations on this `abstract machine`. https://codereview.chromium.org/2243863002/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:358: SetActivationStateForTesting(driver(), ActivationState::ENABLED); We seem to be "micromanaging" the SUT quite a bit. Do you think we could make the tests rely on, and exercise bigger chunks of functionality in the SUT itself? At the extreme end, the approach would really be just to emulate the WebContentsObserver notifications, the SB callbacks, and expect that the right activation signals are sent.
I should be able to finish this before branch point, currently have some problems with tests.
Sounds good, let me know if you want to discuss any issues!
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
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: 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 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...
On 2016/08/24 08:01:05, engedy wrote: > Sounds good, let me know if you want to discuss any issues! PTAL.
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/2243863002/diff/100001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2243863002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:125: void ContentSubresourceFilterDriverFactory::ActivateForFrameHostIfNeeded( To make it easier to argue about code correctness, could we make this method not mutate instance state and remove `control coupling`? Essentially, the only place to turn on |activation_state_| for a page should be in ReadyToCommitMainFrameNavigation(), while DidStartProvisionalLoad for the main frame would reset it. I would recommend taking all this logic here that mutates state and inlining it over there, and make this method just read |activation_state_| and send the IPC to frames accordingly. https://codereview.chromium.org/2243863002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:131: auto parent_factory = Note that |parent_factory == this|, so you could remove a bit of complexity here and remove the set_activation_state() setter entirely. https://codereview.chromium.org/2243863002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:186: ActivateForFrameHostIfNeeded(render_frame_host, validated_url); Let's put this on an 'else' branch once it no longer has side-effects.
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
On 2016/08/25 12:21:29, engedy wrote: > https://codereview.chromium.org/2243863002/diff/100001/components/subresource... > File > components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc > (right): > > https://codereview.chromium.org/2243863002/diff/100001/components/subresource... > components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:125: > void ContentSubresourceFilterDriverFactory::ActivateForFrameHostIfNeeded( > To make it easier to argue about code correctness, could we make this method not > mutate instance state and remove `control coupling`? > > Essentially, the only place to turn on |activation_state_| for a page should be > in ReadyToCommitMainFrameNavigation(), while DidStartProvisionalLoad for the > main frame would reset it. > > I would recommend taking all this logic here that mutates state and inlining it > over there, and make this method just read |activation_state_| and send the IPC > to frames accordingly. > > https://codereview.chromium.org/2243863002/diff/100001/components/subresource... > components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:131: > auto parent_factory = > Note that |parent_factory == this|, so you could remove a bit of complexity here > and remove the set_activation_state() setter entirely. > > https://codereview.chromium.org/2243863002/diff/100001/components/subresource... > components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:186: > ActivateForFrameHostIfNeeded(render_frame_host, validated_url); > Let's put this on an 'else' branch once it no longer has side-effects. ptal.
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 ========== Send activation signal to subframes. BUG=609747 ========== to ========== Send activation signal to subframes. BUG=609747, 609747 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2243863002/diff/100001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2243863002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:186: ActivateForFrameHostIfNeeded(render_frame_host, validated_url); On 2016/08/25 12:21:29, engedy wrote: > Let's put this on an 'else' branch once it no longer has side-effects. Yeah, I am not sure what to do with failing browser tests if we go with this approach. Because there we rely that activation signal is sent even with absence of the navigation throttle.
Description was changed from ========== Send activation signal to subframes. BUG=609747, 609747 ========== to ========== Send activation signal to subframes. BUG=609747, 641035 ==========
LGTM % comments. https://codereview.chromium.org/2243863002/diff/120001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2243863002/diff/120001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:121: if (GetCurrentActivationScope() == ActivationScope::ALL_SITES) nit: GetCurrentActivationScope() is actually quite expensive to call, we'd better save the result of this in a local variable. https://codereview.chromium.org/2243863002/diff/120001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:203: client_->ToggleNotificationVisibility(activation_state() == Note that this call site will call ToggleNotificationVisibility() possible multiple times if there are in-page navigations (e.g. to #refs). We should make sure that we do not overcount prompt impressions in histograms. https://codereview.chromium.org/2243863002/diff/120001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2243863002/diff/120001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:140: // Checks base on the value of |urr| and current activation scope if Phrasing nit: Checks based on the current activation scope whether filtering should be activated for the page load with the given main-frame |url|. https://codereview.chromium.org/2243863002/diff/120001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2243863002/diff/120001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:142: int pending_id = nit: According to the documentation, we should finish this with RenderFrameHostTester::SimulateNavigationCommit(). It might also be simpler. Same below. Maybe we could call it twice? https://codereview.chromium.org/2243863002/diff/120001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:155: /* nit: // comment style and formatting if you want to keep this. https://codereview.chromium.org/2243863002/diff/120001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:167: EXPECT_CALL(*driver(), ActivateForProvisionalLoad(::testing::_)) Can we call the above method instead of lines 167--178, or do we need to get at the |pending_id|? I'd try out if the WebContentsObserver gets the right calls if we call RenderFrameHostTester::SimulateNavigationCommit() twice, or if we actually just simulate a full navigation to the example.com#ref (the tester seem to exercise a lot of the logic, it might already handle that). https://codereview.chromium.org/2243863002/diff/120001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:181: EXPECT_CALL(*driver(), ActivateForProvisionalLoad(::testing::_)).Times(0); nit: Note that you could relax this to <=1 times (with the correct argument).
Still not sure what to do with failing tests. So I probably can try to create a NavigationThrottle manually (which might not work according to comments that some methods I intent to use are for unit tests only), but even if this suceed, I will have to relax checks in the Throttle itself, where I check for the protocol. WDYT, should I try this route? https://codereview.chromium.org/2243863002/diff/120001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2243863002/diff/120001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:142: int pending_id = On 2016/08/25 20:54:50, engedy wrote: > nit: According to the documentation, we should finish this with > RenderFrameHostTester::SimulateNavigationCommit(). It might also be simpler. > > Same below. Maybe we could call it twice? I'm not sure now, but I think I've tried and SimulateNavigationCommit calls DidStartProvisionalLoad again, which resets the activation state. https://codereview.chromium.org/2243863002/diff/120001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:167: EXPECT_CALL(*driver(), ActivateForProvisionalLoad(::testing::_)) On 2016/08/25 20:54:50, engedy wrote: > Can we call the above method instead of lines 167--178, or do we need to get at > the |pending_id|? > > I'd try out if the WebContentsObserver gets the right calls if we call > RenderFrameHostTester::SimulateNavigationCommit() twice, or if we actually just > simulate a full navigation to the example.com#ref (the tester seem to exercise a > lot of the logic, it might already handle that). I've struggled to find method which allows to put a call between DidStartProvisionalLoad and DidCommitProvisionalLoadForFrame.
On 2016/08/27 07:27:03, melandory wrote: > Still not sure what to do with failing tests. So I probably can try to create a > NavigationThrottle manually (which might not work according to comments that > some methods I intent to use are for unit tests only), but even if this suceed, > I will have to relax checks in the Throttle itself, where I check for the > protocol. WDYT, should I try this route? For the full picture: I thhink alternative is to split activation logic between DidStartProvisionalLoad and ReadyToCommit until we can do ReadyToCommit for both pre-plz-navigate and plz-navigate worlds. > https://codereview.chromium.org/2243863002/diff/120001/components/subresource... > File > components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc > (right): > > https://codereview.chromium.org/2243863002/diff/120001/components/subresource... > components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:142: > int pending_id = > On 2016/08/25 20:54:50, engedy wrote: > > nit: According to the documentation, we should finish this with > > RenderFrameHostTester::SimulateNavigationCommit(). It might also be simpler. > > > > Same below. Maybe we could call it twice? > > I'm not sure now, but I think I've tried and SimulateNavigationCommit calls > DidStartProvisionalLoad again, which resets the activation state. > > https://codereview.chromium.org/2243863002/diff/120001/components/subresource... > components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:167: > EXPECT_CALL(*driver(), ActivateForProvisionalLoad(::testing::_)) > On 2016/08/25 20:54:50, engedy wrote: > > Can we call the above method instead of lines 167--178, or do we need to get > at > > the |pending_id|? > > > > I'd try out if the WebContentsObserver gets the right calls if we call > > RenderFrameHostTester::SimulateNavigationCommit() twice, or if we actually > just > > simulate a full navigation to the example.com#ref (the tester seem to exercise > a > > lot of the logic, it might already handle that). > > I've struggled to find method which allows to put a call between > DidStartProvisionalLoad and DidCommitProvisionalLoadForFrame.
On 2016/08/27 07:27:03, melandory wrote: > Still not sure what to do with failing tests. So I probably can try to create a > NavigationThrottle manually (which might not work according to comments that > some methods I intent to use are for unit tests only), but even if this suceed, > I will have to relax checks in the Throttle itself, where I check for the > protocol. WDYT, should I try this route? For the full picture: I thhink alternative is to split activation logic between DidStartProvisionalLoad and ReadyToCommit until we can do ReadyToCommit for both pre-plz-navigate and plz-navigate worlds. > https://codereview.chromium.org/2243863002/diff/120001/components/subresource... > File > components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc > (right): > > https://codereview.chromium.org/2243863002/diff/120001/components/subresource... > components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:142: > int pending_id = > On 2016/08/25 20:54:50, engedy wrote: > > nit: According to the documentation, we should finish this with > > RenderFrameHostTester::SimulateNavigationCommit(). It might also be simpler. > > > > Same below. Maybe we could call it twice? > > I'm not sure now, but I think I've tried and SimulateNavigationCommit calls > DidStartProvisionalLoad again, which resets the activation state. > > https://codereview.chromium.org/2243863002/diff/120001/components/subresource... > components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:167: > EXPECT_CALL(*driver(), ActivateForProvisionalLoad(::testing::_)) > On 2016/08/25 20:54:50, engedy wrote: > > Can we call the above method instead of lines 167--178, or do we need to get > at > > the |pending_id|? > > > > I'd try out if the WebContentsObserver gets the right calls if we call > > RenderFrameHostTester::SimulateNavigationCommit() twice, or if we actually > just > > simulate a full navigation to the example.com#ref (the tester seem to exercise > a > > lot of the logic, it might already handle that). > > I've struggled to find method which allows to put a call between > DidStartProvisionalLoad and DidCommitProvisionalLoadForFrame.
Looks like we could quite easily use EmbeddedTestServer in the browsertest. An instance is already created by the browser test fixture, you just need to ask it to start serving files from the test directory, and start it. See example in drive_app_converter_browsertest.cc. For the unittest, could you please double-check that SimulateNavigationCommit() starts provisional loads? It should only be committing the pending navigation -- otherwise it's a bug in the test helpers.
And for the unit test, something like this should work: content::WebContentsTester::For(web_contents())->StartNavigation(url); factory()->ReadyToCommitMainFrameNavigation(main_rfh(), url); content::RenderFrameHostTester::For(main_rfh())->SimulateNavigationCommit(url); content::RenderFrameHostTester::For(main_rfh())->SimulateNavigationCommit(url);
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...
On 2016/08/28 16:58:36, engedy wrote: > And for the unit test, something like this should work: > > content::WebContentsTester::For(web_contents())->StartNavigation(url); > factory()->ReadyToCommitMainFrameNavigation(main_rfh(), url); > content::RenderFrameHostTester::For(main_rfh())->SimulateNavigationCommit(url); > content::RenderFrameHostTester::For(main_rfh())->SimulateNavigationCommit(url); Ok, it works. Probably I had different issue before.
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: 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 engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2243863002/#ps200001 (title: "cleanup")
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.
Description was changed from ========== Send activation signal to subframes. BUG=609747, 641035 ========== to ========== Send activation signal to subframes. BUG=609747, 641035 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Send activation signal to subframes. BUG=609747, 641035 ========== to ========== Send activation signal to subframes. BUG=609747, 641035 Committed: https://crrev.com/73880b6945b9cc635396db030426a21c825669f5 Cr-Commit-Position: refs/heads/master@{#415179} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/73880b6945b9cc635396db030426a21c825669f5 Cr-Commit-Position: refs/heads/master@{#415179} |