Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(150)

Issue 1906213003: OOPIF: Fix subframe back/forward after recreating FTNs (try #2). (Closed)

Created:
4 years, 8 months ago by Charlie Reis
Modified:
4 years, 7 months ago
Reviewers:
alexmos
CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

OOPIF: Fix subframe back/forward after recreating FTNs (try #2). In some cases, we would navigate the main frame when going back/forward in a subframe. This was because the FTN ID had changed from what was stored in the FrameNavigationEntry. We now use the frame's unique name for subframes and the position in the tree for main frames, as is done in default Chrome (in HistoryController). This also relaxes the restriction that a FrameNavigationEntry's item and document sequence numbers don't change, at least until https://crbug.com/596707 is fixed. This is a second attempt, after r386756 caused issue 603245. BUG=586324, 568768 TEST=See bug for repro steps. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/22a7b4c06a8968ac455072e002af7abdc911c2ad Cr-Commit-Position: refs/heads/master@{#390317}

Patch Set 1 : Try #1, rebased #

Patch Set 2 : Remove FTN ID from FNE #

Patch Set 3 : Fix tests #

Patch Set 4 : Clean up and add tests #

Total comments: 16

Patch Set 5 : Rebase #

Patch Set 6 : Fix nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+258 lines, -125 lines) Patch
M content/browser/frame_host/frame_navigation_entry.h View 1 2 3 4 5 4 chunks +4 lines, -16 lines 0 comments Download
M content/browser/frame_host/frame_navigation_entry.cc View 1 4 chunks +7 lines, -15 lines 0 comments Download
M content/browser/frame_host/frame_tree_node.h View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigation_controller_impl.cc View 1 2 3 5 chunks +22 lines, -17 lines 0 comments Download
M content/browser/frame_host/navigation_controller_impl_browsertest.cc View 1 2 3 4 5 5 chunks +158 lines, -2 lines 0 comments Download
M content/browser/frame_host/navigation_controller_impl_unittest.cc View 1 2 19 chunks +26 lines, -7 lines 0 comments Download
M content/browser/frame_host/navigation_entry_impl.h View 1 2 3 3 chunks +10 lines, -13 lines 0 comments Download
M content/browser/frame_host/navigation_entry_impl.cc View 1 2 3 4 5 10 chunks +23 lines, -45 lines 0 comments Download
M content/browser/frame_host/navigator_impl.cc View 1 2 3 4 5 1 chunk +4 lines, -7 lines 0 comments Download
M testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M testing/buildbot/filters/isolate-extensions.content_browsertests.filter View 1 chunk +0 lines, -1 line 0 comments Download
M testing/buildbot/filters/site-per-process.content_browsertests.filter View 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 13 (6 generated)
Charlie Reis
Alex, can you take a look? This is a revised version of https://codereview.chromium.org/1816763002/, which matches ...
4 years, 7 months ago (2016-04-27 19:12:57 UTC) #3
Charlie Reis
FWIW, the layout test in the linux_site_isolation try jobs (http/tests/fetch/referrer/serviceworker-echo-referrer-from-default-document.html) looks like it's a flake ...
4 years, 7 months ago (2016-04-27 22:43:07 UTC) #4
alexmos
Great, LGTM with nits. Glad that we won't need to worry about things like stale ...
4 years, 7 months ago (2016-04-28 01:05:04 UTC) #5
Charlie Reis
Thanks! https://codereview.chromium.org/1906213003/diff/60001/content/browser/frame_host/frame_navigation_entry.h File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/1906213003/diff/60001/content/browser/frame_host/frame_navigation_entry.h#newcode32 content/browser/frame_host/frame_navigation_entry.h:32: explicit FrameNavigationEntry(); On 2016/04/28 01:05:03, alexmos wrote: > ...
4 years, 7 months ago (2016-04-28 05:37:42 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1906213003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1906213003/100001
4 years, 7 months ago (2016-04-28 05:37:56 UTC) #9
commit-bot: I haz the power
Committed patchset #6 (id:100001)
4 years, 7 months ago (2016-04-28 07:20:43 UTC) #11
commit-bot: I haz the power
4 years, 7 months ago (2016-04-30 17:16:42 UTC) #12
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/22a7b4c06a8968ac455072e002af7abdc911c2ad
Cr-Commit-Position: refs/heads/master@{#390317}

Powered by Google App Engine
This is Rietveld 408576698