Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2157153003:
Call WillStartRequestForTesting in TestRenderFrameHost (Closed)
|
Created:
4 years, 5 months ago by Charlie Harrison Modified:
4 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, Bryan McQuade Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCall WillStartRequestForTesting in TestRenderFrameHost
This CL calls WillStartRequest on the current NavigationHandle when a
navigation is starting. This mimics the SimulateRedirect call, and enables
NavigationThrottles which override WillStartRequest to be unit testable
using the content unit test framework.
Note: like SimulateRedirect, there is no current support for the throttles
actually cancelling / deferring the navigation.
BUG=627501
Committed: https://crrev.com/dd06babf17af821c4fce1d6509946188c43fd208
Cr-Commit-Position: refs/heads/master@{#407150}
Patch Set 1 #
Total comments: 1
Patch Set 2 : merge session / remove DCHECK #
Total comments: 2
Patch Set 3 : rebase on #406654 #Patch Set 4 : Thread transition and docuemnt ShouldAttachNavigationThrottle #
Total comments: 1
Patch Set 5 : Just null check nav handle #
Messages
Total messages: 42 (27 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Description was changed from ========== Call WillStartRequestForTesting in TestRenderFrameHost This CL calls WillStartRequest on the current NavigationHandle when a navigation is starting. This mimics the SimulateRedirect call, and enables NavigationThrottles which override WillStartRequest to be unit testable using the content unit test framework. Note: like SimulateRedirect, there is no current support for the throttles actually cancelling / deferring the navigation. BUG= ========== to ========== Call WillStartRequestForTesting in TestRenderFrameHost This CL calls WillStartRequest on the current NavigationHandle when a navigation is starting. This mimics the SimulateRedirect call, and enables NavigationThrottles which override WillStartRequest to be unit testable using the content unit test framework. Note: like SimulateRedirect, there is no current support for the throttles actually cancelling / deferring the navigation. BUG=627501 ==========
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@, WDYT about this? If you think this method looks okay, then it should greatly simplify https://codereview.chromium.org/2132603002/, where the current approach has weird behavior to satisfy unit tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Looks like the MergeSessionNavigationThrottle does NOT like being added in unit test code :) Would you mind if I added a path in MergeSessionNavigationThrottle::Create that returns nullptr if user_manager::UserManager::Get is null?
Thanks! That seems reasonable though I'm not an owner for the MergeSessionThrottle, so you should contact one. https://codereview.chromium.org/2157153003/diff/1/content/test/test_render_fr... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/2157153003/diff/1/content/test/test_render_fr... content/test/test_render_frame_host.cc:453: DCHECK_EQ(NavigationThrottle::PROCEED, result); On Android the result won't be proceed because the interception throttle runs in asynchronous mode (due to it potentially deleting the WebContents). This is what is causing the failure.
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.
csharrison@chromium.org changed reviewers: + oshima@chromium.org
clamy@ I couldn't quite get the resuming / deferring working so I punted on that. +oshima, could you look at the changes to MergeSession?
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.
lgtm https://codereview.chromium.org/2157153003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/signin/merge_session_throttling_utils.h (right): https://codereview.chromium.org/2157153003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/merge_session_throttling_utils.h:28: bool ShouldAttachNavigationThrottle(); please document 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...
Thanks! clamy@, could you take another look? https://codereview.chromium.org/2157153003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/signin/merge_session_throttling_utils.h (right): https://codereview.chromium.org/2157153003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/merge_session_throttling_utils.h:28: bool ShouldAttachNavigationThrottle(); On 2016/07/20 21:53:31, oshima wrote: > please document this Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! One issue with one of the SimulateWillStart calls below. If it is fine with the PlzNavigate bot, I'll stamp it. https://codereview.chromium.org/2157153003/diff/60001/content/test/test_rende... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/2157153003/diff/60001/content/test/test_rende... content/test/test_render_frame_host.cc:295: SimulateWillStartRequest(transition); It seems to me that we could potentially end up simulating the start twice, which seems problematic. The bots are green right now, but I worry this could be an issue with PlzNavigate enabled, hence why I kicked off a run of the PlzNavigate bot.
Looks like you were right to be nervous about PlzNavigate. I'll continue poking at 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.
Failing tests were calling SimulateWillStartRequest without a valid navigation_handle(). Changing the DCHECK to a early return fixes the tests.
side note: Do you have a plan for running this bot on the CQ?
Thanks! Lgtm. We do run content_browsertests with PlzNavigate on CQ, but not the unit tests.
On 2016/07/22 14:10:09, clamy wrote: > Thanks! Lgtm. We do run content_browsertests with PlzNavigate on CQ, but not the > unit tests. Ah okay sounds good! Thanks for the review(s) :)
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2157153003/#ps80001 (title: "Just null check nav handle")
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 ========== Call WillStartRequestForTesting in TestRenderFrameHost This CL calls WillStartRequest on the current NavigationHandle when a navigation is starting. This mimics the SimulateRedirect call, and enables NavigationThrottles which override WillStartRequest to be unit testable using the content unit test framework. Note: like SimulateRedirect, there is no current support for the throttles actually cancelling / deferring the navigation. BUG=627501 ========== to ========== Call WillStartRequestForTesting in TestRenderFrameHost This CL calls WillStartRequest on the current NavigationHandle when a navigation is starting. This mimics the SimulateRedirect call, and enables NavigationThrottles which override WillStartRequest to be unit testable using the content unit test framework. Note: like SimulateRedirect, there is no current support for the throttles actually cancelling / deferring the navigation. BUG=627501 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Call WillStartRequestForTesting in TestRenderFrameHost This CL calls WillStartRequest on the current NavigationHandle when a navigation is starting. This mimics the SimulateRedirect call, and enables NavigationThrottles which override WillStartRequest to be unit testable using the content unit test framework. Note: like SimulateRedirect, there is no current support for the throttles actually cancelling / deferring the navigation. BUG=627501 ========== to ========== Call WillStartRequestForTesting in TestRenderFrameHost This CL calls WillStartRequest on the current NavigationHandle when a navigation is starting. This mimics the SimulateRedirect call, and enables NavigationThrottles which override WillStartRequest to be unit testable using the content unit test framework. Note: like SimulateRedirect, there is no current support for the throttles actually cancelling / deferring the navigation. BUG=627501 Committed: https://crrev.com/dd06babf17af821c4fce1d6509946188c43fd208 Cr-Commit-Position: refs/heads/master@{#407150} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/dd06babf17af821c4fce1d6509946188c43fd208 Cr-Commit-Position: refs/heads/master@{#407150} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
