Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1617443002:
When a frame needs to navigate, don't prune its subframes from being considered in navigation. (Closed)
|
Created:
4 years, 11 months ago by Avi (use Gerrit) Modified:
4 years, 11 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWhen a frame needs to navigate, don't prune its subframes from being considered in navigation.
BUG=542299
TEST=as in bug
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel;tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/b3b47ab25d16ae178cd926495681205bc6285a4c
Cr-Commit-Position: refs/heads/master@{#371031}
Patch Set 1 #Patch Set 2 : only same-document #Patch Set 3 : with browser test #Patch Set 4 : pedantic++ #
Total comments: 8
Patch Set 5 : moar test #Patch Set 6 : no blank line #Patch Set 7 : Use TestFrameNavigationObserver for cast_shell_linux #Patch Set 8 : LoadCommittedCapturer #Patch Set 9 : patch set 7 #
Messages
Total messages: 44 (21 generated)
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/patch-status/1617443002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617443002/1
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/patch-status/1617443002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617443002/20001
The CQ bit was unchecked by [email protected]
Dry run: This issue passed the CQ dry run.
Description was changed from ========== When a frame needs to navigate, don't prune its subframes from being considered in navigation. BUG=542299 TEST=as in bug ========== to ========== When a frame needs to navigate, don't prune its subframes from being considered in navigation. BUG=542299 TEST=as in bug CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel ==========
Description was changed from ========== When a frame needs to navigate, don't prune its subframes from being considered in navigation. BUG=542299 TEST=as in bug CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel ========== to ========== When a frame needs to navigate, don't prune its subframes from being considered in navigation. BUG=542299 TEST=as in bug CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel;tryserver.chromium.linux:linux_site_isolation ==========
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/patch-status/1617443002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617443002/40001
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/patch-status/1617443002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617443002/60001
The CQ bit was unchecked by [email protected]
Dry run: This issue passed the CQ dry run.
[email protected] changed reviewers: + [email protected]
There's an interesting discussion going on the standards issue; the problem is whether we can elide the "about:blank" when there's no guarantee that it's actually empty. This is a step forward from where we are, and I think we should go for it.
This change LGTM. I'm not sure why we were skipping the subframes during same document navigations before-- that affects more than just the about:blank case. Nate, are we missing anything? We can also observe the bug in a case like: 1) Visit page1 with iframe page2 (non-blank). 2) Do a fragment navigation to #foo. 3) Go to page3 in the iframe. 4) Go back two entries at once, before the fragment nav occurred. The iframe should now be on page2 (and the top URL shouldn't have #foo), but instead the iframe is still on page3. Can you add that test as well? That will help catch regressions if we later decide to change anything else about the about:blank case (due to https://github.com/whatwg/html/issues/546). https://codereview.chromium.org/1617443002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1617443002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3197: // navigation history of a subframe when it is loaded, and 2) that that initial Note: We also have some pretty explicit tests for 1) in NavigationControllerBrowserTest.FrameNavigationEntry_BlankAutoSubframe. https://codereview.chromium.org/1617443002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3215: nit: Remove blank line. (Same in the blocks below.) https://codereview.chromium.org/1617443002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3233: // Now create a new state, snapshotting the old page that has an empty iframe. It's not clear what you mean by snapshotting here. Maybe you're saying that we create a separate NavigationEntry, such that the first entry keeps about:blank in its iframe? https://codereview.chromium.org/1617443002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3258: // even though we elided it in regards to its navigation entry. nit: even though we replaced it in the second NavigationEntry.
[email protected] changed reviewers: + [email protected]
Oops, +japhet for real this time. Nate, was there a reason for the early return on same document navigations?
On 2016/01/21 22:48:29, Charlie Reis wrote: > Oops, +japhet for real this time. Nate, was there a reason for the early return > on same document navigations? I think that's just the way it's always been, WebKit's comparable logic doesn't appear to check child frames if the parent is navigating same-document. But I agree, it should be possible to navigate a frame same-document and one if its children in the same back/forward navigation.
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/patch-status/1617443002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617443002/80001
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/patch-status/1617443002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617443002/100001
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/patch-status/1617443002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617443002/120001
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/patch-status/1617443002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617443002/140001
Charlie, cast_shell_linux keeps failing on the new test, and I can't figure out why. Any thoughts? https://codereview.chromium.org/1617443002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1617443002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3197: // navigation history of a subframe when it is loaded, and 2) that that initial On 2016/01/21 22:47:43, Charlie Reis wrote: > Note: We also have some pretty explicit tests for 1) in > NavigationControllerBrowserTest.FrameNavigationEntry_BlankAutoSubframe. True, though no harm in calling out what is tested here. https://codereview.chromium.org/1617443002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3215: On 2016/01/21 22:47:43, Charlie Reis wrote: > nit: Remove blank line. (Same in the blocks below.) The reason I do it this way is that this comment covers multiple blocks of code, all the way down to line 3231. If I were to remove the blank line, that would imply that it only covered through, say, line 3221. https://codereview.chromium.org/1617443002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3233: // Now create a new state, snapshotting the old page that has an empty iframe. On 2016/01/21 22:47:43, Charlie Reis wrote: > It's not clear what you mean by snapshotting here. Maybe you're saying that we > create a separate NavigationEntry, such that the first entry keeps about:blank > in its iframe? Done. https://codereview.chromium.org/1617443002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3258: // even though we elided it in regards to its navigation entry. On 2016/01/21 22:47:43, Charlie Reis wrote: > nit: even though we replaced it in the second NavigationEntry. Done.
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patch 7 looks right to me, and it seems pretty green on the bots. I would expect TestFrameNavigationObserver to be the right thing. (Also, it doesn't look like these failures were cast_shell_linux specific.)
The CQ bit was checked by [email protected] to run a CQ dry run
On 2016/01/22 17:30:47, Charlie Reis wrote: > Patch 7 looks right to me, and it seems pretty green on the bots. I would > expect TestFrameNavigationObserver to be the right thing. > > (Also, it doesn't look like these failures were cast_shell_linux specific.) Patch set 9 == 7 up; ptal.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617443002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617443002/160001
The CQ bit was unchecked by [email protected]
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/1617443002/#ps160001 (title: "patch set 7")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617443002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617443002/160001
Message was sent while issue was closed.
Description was changed from ========== When a frame needs to navigate, don't prune its subframes from being considered in navigation. BUG=542299 TEST=as in bug CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel;tryserver.chromium.linux:linux_site_isolation ========== to ========== When a frame needs to navigate, don't prune its subframes from being considered in navigation. BUG=542299 TEST=as in bug CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel;tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== When a frame needs to navigate, don't prune its subframes from being considered in navigation. BUG=542299 TEST=as in bug CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel;tryserver.chromium.linux:linux_site_isolation ========== to ========== When a frame needs to navigate, don't prune its subframes from being considered in navigation. BUG=542299 TEST=as in bug CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel;tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/b3b47ab25d16ae178cd926495681205bc6285a4c Cr-Commit-Position: refs/heads/master@{#371031} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b3b47ab25d16ae178cd926495681205bc6285a4c Cr-Commit-Position: refs/heads/master@{#371031}
Message was sent while issue was closed.
Yep, LGTM. Thanks! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
