Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2148313002:
Fix setHistoryItemStateForCommit for back/forward in subframes. (Closed)
|
Created:
4 years, 5 months ago by Charlie Reis Modified:
4 years, 5 months ago CC:
chromium-reviews, tyoshino+watch_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, loading-reviews_chromium.org, darin-cc_chromium.org, gavinp+loader_chromium.org, blink-reviews, Nate Chapin, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix setHistoryItemStateForCommit for back/forward in subframes.
We were not preserving m_provisionalItem on an initial history
commit of a newly created subframe, meaning that we lost things
like the scroll position and document sequence number.
BUG=628286
TEST=See bug for repro steps
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/4e3f962bc6418327ee700d11eadee4501fa5b12f
Cr-Commit-Position: refs/heads/master@{#405844}
Patch Set 1 : Initial commit #
Total comments: 4
Messages
Total messages: 21 (10 generated)
Description was changed from ========== Fix setHistoryItemStateForCommit for back/forward in subframes. We were not preserving m_provisionalItem on an initial history commit of a newly created subframe, meaning that we lost things like the scroll position and document sequence number. BUG=628286 TEST=See bug for repro steps ========== to ========== Fix setHistoryItemStateForCommit for back/forward in subframes. We were not preserving m_provisionalItem on an initial history commit of a newly created subframe, meaning that we lost things like the scroll position and document sequence number. BUG=628286 TEST=See bug for repro steps CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by creis@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by creis@chromium.org
creis@chromium.org changed reviewers: + alexmos@chromium.org, dcheng@chromium.org
Alex, can you review (especially the test)? Daniel, can you review FrameLoader? Not sure if we want to handle it a different way. https://codereview.chromium.org/2148313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2148313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:369: if (isBackForwardLoadType(loadType) && m_provisionalItem) It's non-trivial to write a isBackForwardCommitType, because HistoryCommitType conflates FrameLoadTypeInitialHistoryLoad and FrameLoadTypeInitialInChildFrame (the latter of which is not a history load). I suppose I could split apart HistoryCommitType's InitialCommitInChildFrame and try to update all the uses, if that would be better. Using the FrameLoadType here gives us what we need, anyway.
Description was changed from ========== Fix setHistoryItemStateForCommit for back/forward in subframes. We were not preserving m_provisionalItem on an initial history commit of a newly created subframe, meaning that we lost things like the scroll position and document sequence number. BUG=628286 TEST=See bug for repro steps CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix setHistoryItemStateForCommit for back/forward in subframes. We were not preserving m_provisionalItem on an initial history commit of a newly created subframe, meaning that we lost things like the scroll position and document sequence number. BUG=628286 TEST=See bug for repro steps CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
LGTM https://codereview.chromium.org/2148313002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2148313002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:2740: ->frame_entry->document_sequence_number()); Makes sense - without the fix, these DSNs were freshly generated by HistoryItem::create() rather than restored, right? https://codereview.chromium.org/2148313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2148313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:369: if (isBackForwardLoadType(loadType) && m_provisionalItem) On 2016/07/14 20:45:47, Charlie Reis wrote: > It's non-trivial to write a isBackForwardCommitType, because HistoryCommitType > conflates FrameLoadTypeInitialHistoryLoad and FrameLoadTypeInitialInChildFrame > (the latter of which is not a history load). > > I suppose I could split apart HistoryCommitType's InitialCommitInChildFrame and > try to update all the uses, if that would be better. Using the FrameLoadType > here gives us what we need, anyway. Acknowledged, either approach seems fine to me. There are only a couple of places that use InitialCommitInChildFrame if you wanted to split it apart, but your current approach seems ok as well. If you split them, you might still want something like isInitialCommitInChildFrame(HistoryCommitType), which would be handy in places like the mixed content check in FrameLoader::receivedFirstData.
Thanks. Daniel, your thoughts? https://codereview.chromium.org/2148313002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2148313002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:2740: ->frame_entry->document_sequence_number()); On 2016/07/14 23:03:27, alexmos wrote: > Makes sense - without the fix, these DSNs were freshly generated by > HistoryItem::create() rather than restored, right? Exactly.
This approach LGTM. Do you have a sense of if the other approach (splitting apart InitialCommitInChildFrame) would lead to cleanups elsewhere? If not, this seems good enough.
Thanks! On 2016/07/15 06:41:09, dcheng wrote: > This approach LGTM. > > Do you have a sense of if the other approach (splitting apart > InitialCommitInChildFrame) would lead to cleanups elsewhere? If not, this seems > good enough. Right now, I think there's only two uses: 1) HistoryController handles both cases together in UpdateForCommit. That's a bit ugly, but that code is going away once I get the navigation layout tests fixed. 2) There's the mixed content check in FrameLoader::receivedFirstData that Alex mentioned. I don't understand that one, but Alex made it sound like it cares about both the history and non history cases, so splitting wouldn't be helpful. So I think it's worth proceeding with this version.
The CQ bit was checked by creis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Message was sent while issue was closed.
Description was changed from ========== Fix setHistoryItemStateForCommit for back/forward in subframes. We were not preserving m_provisionalItem on an initial history commit of a newly created subframe, meaning that we lost things like the scroll position and document sequence number. BUG=628286 TEST=See bug for repro steps CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix setHistoryItemStateForCommit for back/forward in subframes. We were not preserving m_provisionalItem on an initial history commit of a newly created subframe, meaning that we lost things like the scroll position and document sequence number. BUG=628286 TEST=See bug for repro steps CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #1 (id:20001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Fix setHistoryItemStateForCommit for back/forward in subframes. We were not preserving m_provisionalItem on an initial history commit of a newly created subframe, meaning that we lost things like the scroll position and document sequence number. BUG=628286 TEST=See bug for repro steps CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix setHistoryItemStateForCommit for back/forward in subframes. We were not preserving m_provisionalItem on an initial history commit of a newly created subframe, meaning that we lost things like the scroll position and document sequence number. BUG=628286 TEST=See bug for repro steps CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/4e3f962bc6418327ee700d11eadee4501fa5b12f Cr-Commit-Position: refs/heads/master@{#405844} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/4e3f962bc6418327ee700d11eadee4501fa5b12f Cr-Commit-Position: refs/heads/master@{#405844} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
