Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1417953002:
PlzNavigate: Add tests to check WebUI navigations. (Closed)
|
Created:
5 years, 2 months ago by carlosk Modified:
5 years, 1 month ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: Add tests to check WebUI navigations.
(was "PlzNavigate: never discard and recreate the same speculative WebUI")
Adds 3 tests to verify the correct handling of WebUI state when navigating between different WebUI URLs.
This change also:
- Improves the PlzNavigate version of RenderFrameHostManagerTest tests by making it better simulate the navigation and fixes a test that was broken because of that.
- Allows control over returned WebUIType values from the mock factory.
This is based off and depends on the landing of https://codereview.chromium.org/1418853003.
BUG=508850
Patch Set 1 : Minor update. #
Total comments: 8
Patch Set 2 : Fixing change/branch dependency. #Patch Set 3 : Changes from clamy@ comments. #
Total comments: 8
Patch Set 4 : Code comment fixes. #Patch Set 5 : Broken test into 3 separate ones. Other minor changes. #
Total comments: 26
Patch Set 6 : Addressing CR comments (still needs a rebase). #
Total comments: 2
Patch Set 7 : Rebase to a different patch (http://crrev.com/1418853003) and comment update. #Messages
Total messages: 20 (9 generated)
Description was changed from ========== PlzNavigate: never discard and recreate the same speculative WebUI. This fixes an issue with PlzNavigate only after the WebUI refactor where a pending WebUI set on the current RenderFrameHost would not be properly reused at navigation commit time when it should be. This change adds also adds a test that checks for this situation among others involving navigations between different WebUI pages. This is based off of http://crrev.com/1352813006. BUG=508850 ========== to ========== PlzNavigate: never discard and recreate the same speculative WebUI. This fixes an issue with PlzNavigate only after the WebUI refactor where a pending WebUI set on the current RenderFrameHost would not be properly reused at navigation commit time when it should be. This change also: - Improves the PlzNavigate version of RenderFrameHostManagerTest tests by making it better simulate the navigation and fixes a test that was broken because of that. - Allows control over returned WebUIType values from the mock factory. - Adds a test that checks for the main fix by testing navigating between different WebUI URLs. This is based off and depends on the landing of http://crrev.com/1352813006. BUG=508850 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
carlosk@chromium.org changed reviewers: + clamy@chromium.org, nasko@chromium.org
nasko@, clamy@: PTAL. This is the fix for the CleanUpNavigation problem I had a TODO for in http://crrev.com/1352813006 (and depends on it landing first). Feel free to take a look now or wait for that one to land. Note: I updated the description so the initial email description will be outdated.
Thanks! A few preliminary comments. I haven't reviewed the new test yet, will do it tomorrow. https://codereview.chromium.org/1417953002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1417953002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1011: render_frame_host_->UpdatePendingWebUI(request.common_params().url, This means that we will call UpdatePendingWebUI twice right? https://codereview.chromium.org/1417953002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1417953002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:90: return reinterpret_cast<void*>(type_); If you want to cast back and from a pointer, you should use uintptr_t instead of int. Also this looks a bit ugly. Would it be possible to have helper functions in WebUI::TypeID instead? https://codereview.chromium.org/1417953002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:424: // Simulates request creation that triggers the 1st internal call to nit: add a line before the comment. https://codereview.chromium.org/1417953002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:427: // And also simulates the 2nd and final call to GetFrameHostForNavigation nit: same here.
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Thanks. https://codereview.chromium.org/1417953002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1417953002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1011: render_frame_host_->UpdatePendingWebUI(request.common_params().url, On 2015/10/21 16:54:22, clamy wrote: > This means that we will call UpdatePendingWebUI twice right? I'm not sure I understood what you mean but we will indeed call it twice if this is considered a same-site navigation at both request and commit time. https://codereview.chromium.org/1417953002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1417953002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:90: return reinterpret_cast<void*>(type_); On 2015/10/21 16:54:22, clamy wrote: > If you want to cast back and from a pointer, you should use uintptr_t instead of > int. Also this looks a bit ugly. Would it be possible to have helper functions > in WebUI::TypeID instead? Done for using uintptr_t. But, as I tried to explain in the comment, WebUI::TypeID is a typedef to void* so we can't add functionality to it. https://codereview.chromium.org/1417953002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:424: // Simulates request creation that triggers the 1st internal call to On 2015/10/21 16:54:22, clamy wrote: > nit: add a line before the comment. Done. https://codereview.chromium.org/1417953002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:427: // And also simulates the 2nd and final call to GetFrameHostForNavigation On 2015/10/21 16:54:22, clamy wrote: > nit: same here. Done.
Thanks! https://codereview.chromium.org/1417953002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1417953002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1004: if (speculative_render_frame_host_) I think you could add a comment here to explain that we don't want to delete a previously created pending WebUI in the RFH if we don't need to. And I guess that the eventual replacement will be done in render_frame_host_->UpdatePendingWebUI right? (If so, mention it as well). https://codereview.chromium.org/1417953002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1417953002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:59: CHECK_NE(reinterpret_cast<void*>(type_), WebUI::kNoWebUI); Since WebUI::TypeID is actually a void*, could we cast to a WebUI::TypeID instead of a void*. I think it will be more readable. Same below. https://codereview.chromium.org/1417953002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:67: void set_webui_type(uintptr_t type) { nit: add a comment explaining that this allows to create WebUIs of different types for testing? https://codereview.chromium.org/1417953002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2523: TEST_F(RenderFrameHostManagerTestWithBrowserSideNavigation, nit: add a comment describing what the test does. Also there seem to be 3 main part in the test, would it make sense to break it into 3 independent tests?
Thanks. https://codereview.chromium.org/1417953002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1417953002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1004: if (speculative_render_frame_host_) On 2015/10/22 15:37:10, clamy wrote: > I think you could add a comment here to explain that we don't want to delete a > previously created pending WebUI in the RFH if we don't need to. And I guess > that the eventual replacement will be done in > render_frame_host_->UpdatePendingWebUI right? (If so, mention it as well). Done. https://codereview.chromium.org/1417953002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1417953002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:59: CHECK_NE(reinterpret_cast<void*>(type_), WebUI::kNoWebUI); On 2015/10/22 15:37:10, clamy wrote: > Since WebUI::TypeID is actually a void*, could we cast to a WebUI::TypeID > instead of a void*. I think it will be more readable. Same below. Done. https://codereview.chromium.org/1417953002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:67: void set_webui_type(uintptr_t type) { On 2015/10/22 15:37:10, clamy wrote: > nit: add a comment explaining that this allows to create WebUIs of different > types for testing? Done. I then also moved the comment below to here. https://codereview.chromium.org/1417953002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2523: TEST_F(RenderFrameHostManagerTestWithBrowserSideNavigation, On 2015/10/22 15:37:10, clamy wrote: > nit: add a comment describing what the test does. > > Also there seem to be 3 main part in the test, would it make sense to break it > into 3 independent tests? Done. And indeed this looks much better now that it's broken down to 3 tests.
Thanks! A few more comments. Other than that I'm fine with the patch. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1006: // maintained for the next call to UpdatePendingWebUI. So instead of calling nit: RenderFrameHostImpl::UpdatePendingWebUI. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1007: // CleanUpNavigation, only the speculative RenderFrameHost is discarded. nit: s/the speculative RenderFrameHost is discarded/discard the speculative RenderFrameHost https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:68: // instance types would be created. This value will be returned in nit: either "a different WebUI instance type" or "different WebUI instance types" https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2528: // are reached when navigating from a dead renderer to a WebUI URL. nit: s/a dead renderer/a renderer that is not live https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2554: // As the initial renderer was not be live, the new RenderFrameHost should nit: remove be https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2567: // Prepare to commit, updates the navigating RenderFrameHost. nit: update https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2569: host->set_pending_commit(true); Now that we have the NavigationHandle, I think we can get rid of this boolean (not in this patch though). We should test for a NavigatioNhandle instead of the boolean. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2621: // Current WebUI should still be in place and a new one should be pending nit: The current.... and there should be a pending WebUI in the current RenderFrameHost. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2631: // Prepare to commit, updates the navigating RenderFrameHost. nit: update https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2633: host->set_pending_commit(true); Again, let's avoid using this boolean. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2683: // Current WebUI should still be in place and a new one should be pending nit: see the comment about this comment above. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2695: // Prepare to commit, updates the navigating RenderFrameHost. Nit: see comment above. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2702: EXPECT_NE(next_web_ui, manager->current_frame_host()->web_ui()); I think we should also test that the current RFH does not have a pending WebUI.
Thanks. This change will need a rebase due to changes still ongoing with the refactor. But I wanted to put this up anyway. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1006: // maintained for the next call to UpdatePendingWebUI. So instead of calling On 2015/10/22 16:51:39, clamy wrote: > nit: RenderFrameHostImpl::UpdatePendingWebUI. Done. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1007: // CleanUpNavigation, only the speculative RenderFrameHost is discarded. On 2015/10/22 16:51:39, clamy wrote: > nit: s/the speculative RenderFrameHost is discarded/discard the speculative > RenderFrameHost Done. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:68: // instance types would be created. This value will be returned in On 2015/10/22 16:51:40, clamy wrote: > nit: either "a different WebUI instance type" or "different WebUI instance > types" Done. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2528: // are reached when navigating from a dead renderer to a WebUI URL. On 2015/10/22 16:51:40, clamy wrote: > nit: s/a dead renderer/a renderer that is not live Done. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2554: // As the initial renderer was not be live, the new RenderFrameHost should On 2015/10/22 16:51:39, clamy wrote: > nit: remove be Done. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2567: // Prepare to commit, updates the navigating RenderFrameHost. On 2015/10/22 16:51:40, clamy wrote: > nit: update Done. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2569: host->set_pending_commit(true); On 2015/10/22 16:51:39, clamy wrote: > Now that we have the NavigationHandle, I think we can get rid of this boolean > (not in this patch though). We should test for a NavigatioNhandle instead of the > boolean. Acknowledged. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2621: // Current WebUI should still be in place and a new one should be pending On 2015/10/22 16:51:39, clamy wrote: > nit: The current.... and there should be a pending WebUI in the current > RenderFrameHost. Done. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2631: // Prepare to commit, updates the navigating RenderFrameHost. On 2015/10/22 16:51:39, clamy wrote: > nit: update Done. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2633: host->set_pending_commit(true); On 2015/10/22 16:51:39, clamy wrote: > Again, let's avoid using this boolean. Acknowledged. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2683: // Current WebUI should still be in place and a new one should be pending On 2015/10/22 16:51:39, clamy wrote: > nit: see the comment about this comment above. I adapted this to follow the same format but they are not the same. It was good you brought my attention to this one as there was an error in it. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2695: // Prepare to commit, updates the navigating RenderFrameHost. On 2015/10/22 16:51:39, clamy wrote: > Nit: see comment above. Done. https://codereview.chromium.org/1417953002/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2702: EXPECT_NE(next_web_ui, manager->current_frame_host()->web_ui()); On 2015/10/22 16:51:39, clamy wrote: > I think we should also test that the current RFH does not have a pending WebUI. Done.
Thanks! Lgtm. https://codereview.chromium.org/1417953002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1417953002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:67: // This method allows simulating the expectation that different WebUI instance nit: s/allows simulating/simulates
I would hold off on this one until the main CL is updated with the latest behavior.
Patchset #7 (id:200001) has been deleted
Description was changed from ========== PlzNavigate: never discard and recreate the same speculative WebUI. This fixes an issue with PlzNavigate only after the WebUI refactor where a pending WebUI set on the current RenderFrameHost would not be properly reused at navigation commit time when it should be. This change also: - Improves the PlzNavigate version of RenderFrameHostManagerTest tests by making it better simulate the navigation and fixes a test that was broken because of that. - Allows control over returned WebUIType values from the mock factory. - Adds a test that checks for the main fix by testing navigating between different WebUI URLs. This is based off and depends on the landing of http://crrev.com/1352813006. BUG=508850 ========== to ========== PlzNavigate: never discard and recreate the same speculative WebUI. This fixes an issue with PlzNavigate only after the WebUI refactor where a pending WebUI set on the current RenderFrameHost would not be properly reused at navigation commit time when it should be. This change also: - Improves the PlzNavigate version of RenderFrameHostManagerTest tests by making it better simulate the navigation and fixes a test that was broken because of that. - Allows control over returned WebUIType values from the mock factory. - Adds a test that checks for the main fix by testing navigating between different WebUI URLs. This is based off and depends on the landing of https://codereview.chromium.org/1418853003. BUG=508850 ==========
Description was changed from ========== PlzNavigate: never discard and recreate the same speculative WebUI. This fixes an issue with PlzNavigate only after the WebUI refactor where a pending WebUI set on the current RenderFrameHost would not be properly reused at navigation commit time when it should be. This change also: - Improves the PlzNavigate version of RenderFrameHostManagerTest tests by making it better simulate the navigation and fixes a test that was broken because of that. - Allows control over returned WebUIType values from the mock factory. - Adds a test that checks for the main fix by testing navigating between different WebUI URLs. This is based off and depends on the landing of https://codereview.chromium.org/1418853003. BUG=508850 ========== to ========== PlzNavigate: Add tests to check WebUI navigations. (was "PlzNavigate: never discard and recreate the same speculative WebUI") Adds 3 tests to verify the correct handling of WebUI state when navigating between different WebUI URLs. This change also: - Improves the PlzNavigate version of RenderFrameHostManagerTest tests by making it better simulate the navigation and fixes a test that was broken because of that. - Allows control over returned WebUIType values from the mock factory. This is based off and depends on the landing of https://codereview.chromium.org/1418853003. BUG=508850 ==========
As finally I'm in need on the original WebUI change of the ability to set the type of the to-be-created WebUI in RenderFrameHostManagerTest I'm also merging this change into that one. https://codereview.chromium.org/1417953002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1417953002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:67: // This method allows simulating the expectation that different WebUI instance On 2015/10/27 11:54:44, clamy wrote: > nit: s/allows simulating/simulates Done. (it will show up on the other change I merged this into)
Merged into https://codereview.chromium.org/1426403006/ |
