Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2368183004:
Move redirect_chain from NavigationEntry to FrameNavigationEntry. (Closed)
|
Created:
4 years, 2 months ago by arthursonzogni Modified:
4 years, 2 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, clamy, nasko Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove redirect_chain from NavigationEntry to FrameNavigationEntry.
Now that we keep track of subframe navigations in FrameNavigationEntry,
this CL move the redirect_chain field from NavigationEntry to
FrameNavigationEntry.
BUG=649879
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/e6b67c20b5159f5fa10108b8ebccb9cf95c18479
Cr-Commit-Position: refs/heads/master@{#422737}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Addressed comments. #
Total comments: 2
Patch Set 3 : Rebase #Patch Set 4 : Add tests + bugfix. #
Total comments: 4
Patch Set 5 : Nit #Messages
Total messages: 36 (25 generated)
Description was changed from ========== WIP: move redirect_chain from NavigationEntry to FrameNavigationEntry. BUG=649879 ========== to ========== WIP: move redirect_chain from NavigationEntry to FrameNavigationEntry. BUG=649879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by [email protected]
The CQ bit was unchecked by [email protected]
The CQ bit was checked by [email protected] 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 [email protected]
Dry run: This issue passed the CQ dry run.
Description was changed from ========== WIP: move redirect_chain from NavigationEntry to FrameNavigationEntry. BUG=649879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move redirect_chain from NavigationEntry to FrameNavigationEntry. Now that we keep track of subframe navigations in FrameNavigationEntry, we should move the redirect_chain field from NavigationEntry to FrameNavigationEntry. BUG=649879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Move redirect_chain from NavigationEntry to FrameNavigationEntry. Now that we keep track of subframe navigations in FrameNavigationEntry, we should move the redirect_chain field from NavigationEntry to FrameNavigationEntry. BUG=649879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move redirect_chain from NavigationEntry to FrameNavigationEntry. Now that we keep track of subframe navigations in FrameNavigationEntry, this CL move the redirect_chain field from NavigationEntry to FrameNavigationEntry. BUG=649879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Hi Charlie, Can you take a look at this CL? Since this is the first time I work on this part of Chromium, I have some interrogations. I put some comments. I keep and rename NavigationEntry::(Set/Get)RedirectChain to (Set/Get)MainFrameRedirectChain because it better reflects the new behavior. But we can keep the old name. Though, we can check that every lines that used theses two functions are applied to the right frame, since they appear on the diff. Thank you. https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_navigation_entry.h:158: std::vector<GURL> redirect_chain_; I am not sure of the comment message. Which sentences are still necessary and correct? https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:721: entry->SetMainFrameRedirectChain(params.redirect_chain); I don't know the reasons why the main frame redirection chain is optionally updated. Do we have to do the same thing a few lines above for subframes too? https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:1217: params.method, params.post_id); I want to warn that previously, params.redirects was not used to update the NavigationEntry. I would like to check that it is not wrong to update the FrameNavigationEntry. The same thing happens also in the next two modifications of this file.
[email protected] changed reviewers: + [email protected]
Hi Charlie, Can you take a look at this CL? Since this is the first time I work on this part of Chromium, I have some interrogations. I put some comments. I keep and rename NavigationEntry::(Set/Get)RedirectChain to (Set/Get)MainFrameRedirectChain because it better reflects the new behavior. But we can keep the old name. Though, we can check that every lines that used theses two functions are applied to the right frame, since they appear on the diff. Thank you.
Thanks! This is very nice, and it looks like it fixes an extra bug as well. Maybe we should include a browser test showing that subframe redirects are preserved on commit, and that they don't leak into NavigationEntry::GetRedirectChain? If you agree with the suggested changes, I'm happy to CQ an updated patch for you tomorrow. https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_navigation_entry.h:34: FrameNavigationEntry(const std::string& frame_unique_name, I'm debating whether we should include it as a parameter here as well. This is mostly the same as UpdateEntry, apart from PageState. I suppose it's ok to omit it unless we find a need for it, though. https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_navigation_entry.h:54: const std::vector<GURL>& redirect_chain, Throughout this file, we should list redirect_chain lower among the other parameters/members, perhaps between referrer and page_state. https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_navigation_entry.h:158: std::vector<GURL> redirect_chain_; On 2016/09/27 12:56:20, arthursonzogni wrote: > I am not sure of the comment message. Which sentences are still necessary and > correct? The first sentence is still true (see NavigatorImpl::RequestTransferURL for transfers, and NavigationControllerImpl::LoadURLWithParams for OpenURL). The second sentence is only true for main frames, so let's clarify that. (See ContentSerializedNavigationBuilder::FromNavigationEntry.) The third sentence is still true. nit: As above, let's move this down between referrer_ and page_state_. https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:721: entry->SetMainFrameRedirectChain(params.redirect_chain); On 2016/09/27 12:56:20, arthursonzogni wrote: > I don't know the reasons why the main frame redirection chain is optionally > updated. Do we have to do the same thing a few lines above for subframes too? Let's remove the size check. Originally, calling SetRedirectChain with a zero size chain would have been no-op, but that's less clear as this method has grown to support subframes. https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:1217: params.method, params.post_id); On 2016/09/27 12:56:20, arthursonzogni wrote: > I want to warn that previously, params.redirects was not used to update the > NavigationEntry. I would like to check that it is not wrong to update the > FrameNavigationEntry. > > The same thing happens also in the next two modifications of this file. This case and the SamePage one were covered by the active_entry->SetRedirectChain(params.redirects) call in RendererDidNavigate (around 881). Looks like we'll assign it redundantly, but maybe that's ok. The AutoSubframe case didn't used to track redirects for subframes (since it returned early), so it seems fine to add it now. I think you discovered a bug in the NewSubframe case. That does go through the SetRedirectChain call in RendererDidNavigate, meaning that subframe redirects used to end up on the main frame. (I confirmed in a debugger.) Glad we're fixing it! Repro steps, if you're curious: 0) Start testserver.py. 1) Visit http://csreis.github.io/tests/cross-site-iframe.html. 2) In the DevTools console, enter: navFrame("http://127.0.0.1:8080/server-redirect?https://csreis.github.io"); At this point, NavigationEntry::GetRedirectChain will incorrectly show the redirect chain for the subframe. Your change should fix that. https://codereview.chromium.org/2368183004/diff/1/content/public/browser/navi... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/2368183004/diff/1/content/public/browser/navi... content/public/browser/navigation_entry.h:223: virtual void SetMainFrameRedirectChain( I appreciate getting to easily see all the call sites that are affected due to the rename, but I think we should put it back to SetRedirectChain. We generally haven't been updating other method names (e.g., GetURL, etc), and the subframe tracking is mostly an internal implementation detail.
The CQ bit was checked by [email protected] 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...
Hi Charlie, Thank you very much for this review. I think I fixed every addressed comments. Yes, we should make some tests that check the redirect_chains are the good one for each FrameNavigationEntry. I can do it if you want. But I am on a offsite event the next two days. I come back Monday. Do you think I can put the tests in site_per_process_browsertest ? Do you want to include these tests in this CL or I can I make another one for tests ? Thanks, Arthur https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_navigation_entry.h:34: FrameNavigationEntry(const std::string& frame_unique_name, On 2016/09/28 04:13:17, Charlie Reis wrote: > I'm debating whether we should include it as a parameter here as well. This is > mostly the same as UpdateEntry, apart from PageState. I suppose it's ok to omit > it unless we find a need for it, though. I don't know. I think I didn't include it here because it wasn't previously included in NavigationEntry neither. Let's say it will be added once it become useful. https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_navigation_entry.h:54: const std::vector<GURL>& redirect_chain, On 2016/09/28 04:13:17, Charlie Reis wrote: > Throughout this file, we should list redirect_chain lower among the other > parameters/members, perhaps between referrer and page_state. Done. https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_navigation_entry.h:158: std::vector<GURL> redirect_chain_; On 2016/09/28 04:13:17, Charlie Reis wrote: > On 2016/09/27 12:56:20, arthursonzogni wrote: > > I am not sure of the comment message. Which sentences are still necessary and > > correct? > > The first sentence is still true (see NavigatorImpl::RequestTransferURL for > transfers, and NavigationControllerImpl::LoadURLWithParams for OpenURL). > > The second sentence is only true for main frames, so let's clarify that. (See > ContentSerializedNavigationBuilder::FromNavigationEntry.) > > The third sentence is still true. > > nit: As above, let's move this down between referrer_ and page_state_. Thanks! https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:721: entry->SetMainFrameRedirectChain(params.redirect_chain); On 2016/09/28 04:13:17, Charlie Reis wrote: > On 2016/09/27 12:56:20, arthursonzogni wrote: > > I don't know the reasons why the main frame redirection chain is optionally > > updated. Do we have to do the same thing a few lines above for subframes too? > > Let's remove the size check. Originally, calling SetRedirectChain with a zero > size chain would have been no-op, but that's less clear as this method has grown > to support subframes. Done. https://codereview.chromium.org/2368183004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:1217: params.method, params.post_id); On 2016/09/28 04:13:17, Charlie Reis wrote: > On 2016/09/27 12:56:20, arthursonzogni wrote: > > I want to warn that previously, params.redirects was not used to update the > > NavigationEntry. I would like to check that it is not wrong to update the > > FrameNavigationEntry. > > > > The same thing happens also in the next two modifications of this file. > > This case and the SamePage one were covered by the > active_entry->SetRedirectChain(params.redirects) call in RendererDidNavigate > (around 881). Looks like we'll assign it redundantly, but maybe that's ok. > > The AutoSubframe case didn't used to track redirects for subframes (since it > returned early), so it seems fine to add it now. > > I think you discovered a bug in the NewSubframe case. That does go through the > SetRedirectChain call in RendererDidNavigate, meaning that subframe redirects > used to end up on the main frame. (I confirmed in a debugger.) Glad we're > fixing it! > > Repro steps, if you're curious: > 0) Start testserver.py. > 1) Visit http://csreis.github.io/tests/cross-site-iframe.html. > 2) In the DevTools console, enter: > navFrame("http://127.0.0.1:8080/server-redirect?https://csreis.github.io"); > > At this point, NavigationEntry::GetRedirectChain will incorrectly show the > redirect chain for the subframe. Your change should fix that. I'm glad that it fix a new bug! Thanks for the repro steps. https://codereview.chromium.org/2368183004/diff/1/content/public/browser/navi... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/2368183004/diff/1/content/public/browser/navi... content/public/browser/navigation_entry.h:223: virtual void SetMainFrameRedirectChain( On 2016/09/28 04:13:17, Charlie Reis wrote: > I appreciate getting to easily see all the call sites that are affected due to > the rename, but I think we should put it back to SetRedirectChain. We generally > haven't been updating other method names (e.g., GetURL, etc), and the subframe > tracking is mostly an internal implementation detail. Yes, that's what I thought. I will make it come back to the original names.
Great, thanks. On 2016/09/28 16:36:14, arthursonzogni wrote: > Hi Charlie, > > Thank you very much for this review. > I think I fixed every addressed comments. > > Yes, we should make some tests that check the redirect_chains are the good one > for each FrameNavigationEntry. > I can do it if you want. But I am on a offsite event the next two days. I come > back Monday. > Do you think I can put the tests in site_per_process_browsertest ? > Do you want to include these tests in this CL or I can I make another one for > tests ? We should generally include the test in the same CL as the fix, for a few reasons: it helps document the effect of the change, it shows the change behaves as expected, and it should be reverted together with the fix if we later find out something was wrong. navigation_controller_impl_browsertest.cc would be the right place for this test, probably in the FrameNavigationEntry_ section of tests. Note how many of those check subframe FNEs behind a UseSubframeNavigationEntries check-- we should continue that pattern for the time being. (That mode is on by default now, but we won't be ripping it out until M56.) Doing it when you get back from the offsite sounds fine. Thanks! https://codereview.chromium.org/2368183004/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2368183004/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:1154: pending_entry->GetRedirectChain()); nit: Can fit one one line.
The CQ bit was unchecked by [email protected]
Dry run: This issue passed the CQ dry run.
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #4 (id:100001) has been deleted
Thank you very much for all of these information. It is nice. I made new tests. It helps me to discover and fix on bug with my previous patches. Please tell me what do you think of it. https://codereview.chromium.org/2368183004/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2368183004/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:1154: pending_entry->GetRedirectChain()); On 2016/09/28 17:35:14, Charlie Reis wrote: > nit: Can fit one one line. Done.
The CQ bit was checked by [email protected] 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 [email protected]
Dry run: This issue passed the CQ dry run.
Nice! Glad the test caught a bug, too. LGTM with nits. https://codereview.chromium.org/2368183004/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2368183004/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:4211: NavigateToURL(shell(), redirecting_url); nit: EXPECT_TRUE (same in the other NavigateToURL calls below). (Sorry, this file has a bunch of old calls that could use updating, to avoid copy/paste problems on this pattern. I'll put together a CL.) https://codereview.chromium.org/2368183004/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:4267: // 1. Navigate to a page with an iframe nit: End with period.
Thanks for your comments! https://codereview.chromium.org/2368183004/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2368183004/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:4211: NavigateToURL(shell(), redirecting_url); On 2016/10/03 18:37:22, Charlie Reis wrote: > nit: EXPECT_TRUE (same in the other NavigateToURL calls below). > > (Sorry, this file has a bunch of old calls that could use updating, to avoid > copy/paste problems on this pattern. I'll put together a CL.) Done. Nevertheless, NavigateToURL returns false for redirecting URL because there is a mismatch with the final_url. I used NavigateToURLBlockUntilNavigationsComplete and others assertions instead. https://codereview.chromium.org/2368183004/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:4267: // 1. Navigate to a page with an iframe On 2016/10/03 18:37:22, Charlie Reis wrote: > nit: End with period. Done.
The CQ bit was checked by [email protected]
The patchset sent to the CQ was uploaded after l-g-t-m from [email protected] Link to the patchset: https://codereview.chromium.org/2368183004/#ps140001 (title: "Nit")
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 ========== Move redirect_chain from NavigationEntry to FrameNavigationEntry. Now that we keep track of subframe navigations in FrameNavigationEntry, this CL move the redirect_chain field from NavigationEntry to FrameNavigationEntry. BUG=649879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move redirect_chain from NavigationEntry to FrameNavigationEntry. Now that we keep track of subframe navigations in FrameNavigationEntry, this CL move the redirect_chain field from NavigationEntry to FrameNavigationEntry. BUG=649879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Move redirect_chain from NavigationEntry to FrameNavigationEntry. Now that we keep track of subframe navigations in FrameNavigationEntry, this CL move the redirect_chain field from NavigationEntry to FrameNavigationEntry. BUG=649879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move redirect_chain from NavigationEntry to FrameNavigationEntry. Now that we keep track of subframe navigations in FrameNavigationEntry, this CL move the redirect_chain field from NavigationEntry to FrameNavigationEntry. BUG=649879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/e6b67c20b5159f5fa10108b8ebccb9cf95c18479 Cr-Commit-Position: refs/heads/master@{#422737} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e6b67c20b5159f5fa10108b8ebccb9cf95c18479 Cr-Commit-Position: refs/heads/master@{#422737} |
