Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15294012:
Fix initial erroneous navigation in iframe to not add history entry. (Closed)
|
Created:
7 years, 7 months ago by nasko Modified:
7 years, 2 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix initial erroneous navigation in iframe to not add history entry.
The underlying issue is that AUTO_SUBFRAME transition type was incorrectly being set on subframe navigations. I'm fixing this to be compliant with the HTML5 spec.
Blink side of this patch is https://codereview.chromium.org/25269006/.
BUG=178380
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226669
Patch Set 1 #Patch Set 2 : Adding a test and lots of logging for try jobs. #Patch Set 3 : Removing logs and fixing up the test. #
Total comments: 2
Patch Set 4 : Redo how the test case works. #Patch Set 5 : Another improvement to the test. #
Total comments: 6
Patch Set 6 : Rebased on ToT. #Patch Set 7 : Properly set AUTO_SUBFRAME transition type on subframe navigations #
Total comments: 2
Patch Set 8 : Fix in sync with Blink and modified the test case a bit #
Total comments: 4
Patch Set 9 : Fixing nits. #
Messages
Total messages: 26 (0 generated)
Hey guys, Can you review this change? mmenke@ -> errorpage_browsertest.cc creis@ -> render_view_impl.cc darin@ -> chrome OWNERs Thanks in advance. Nasko
Thanks for fixing this. https://codereview.chromium.org/15294012/diff/16001/chrome/browser/errorpage_... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/15294012/diff/16001/chrome/browser/errorpage_... chrome/browser/errorpage_browsertest.cc:262: "document.title = 'done';"; This failed consistently prior to the fix? My concern here is that the history could theoretically be updated only after the frame commits, which happens asynchronously. Is there some way we could wait until after the iframe has been filled with the error page?
https://codereview.chromium.org/15294012/diff/16001/chrome/browser/errorpage_... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/15294012/diff/16001/chrome/browser/errorpage_... chrome/browser/errorpage_browsertest.cc:262: "document.title = 'done';"; On 2013/05/22 17:25:50, mmenke wrote: > This failed consistently prior to the fix? My concern here is that the history > could theoretically be updated only after the frame commits, which happens > asynchronously. > > Is there some way we could wait until after the iframe has been filled with the > error page? I see your concern. I haven't tried the test on the try bots before the fix, but it failed consistently for me locally. I've been looking for a way to wait for a failed load, but there is no such notification one can monitor. Let me try another approach and see if it is better.
I've changed the test to use explicit DidFailProvisionalLoad observer and ensure that we see the expected failure. I think this is more of what I initially had in mind. Please review.
On 2013/05/23 18:21:17, nasko wrote: > I've changed the test to use explicit DidFailProvisionalLoad observer and ensure > that we see the expected failure. > I think this is more of what I initially had in mind. > > Please review. Quick question: Is this waiting long enough, or should we wait until the error page is committed, after the failure?
On 2013/05/23 18:25:47, mmenke wrote: > On 2013/05/23 18:21:17, nasko wrote: > > I've changed the test to use explicit DidFailProvisionalLoad observer and > ensure > > that we see the expected failure. > > I think this is more of what I initially had in mind. > > > > Please review. > > Quick question: Is this waiting long enough, or should we wait until the error > page is committed, after the failure? Which page is expected to commit? http://mock.failed.request/-105 or the data: URL for the actual error text?
On 2013/05/23 19:03:16, nasko wrote: > On 2013/05/23 18:25:47, mmenke wrote: > > On 2013/05/23 18:21:17, nasko wrote: > > > I've changed the test to use explicit DidFailProvisionalLoad observer and > > ensure > > > that we see the expected failure. > > > I think this is more of what I initially had in mind. > > > > > > Please review. > > > > Quick question: Is this waiting long enough, or should we wait until the > error > > page is committed, after the failure? > > Which page is expected to commit? http://mock.failed.request/-105 or the data: > URL for the actual error text? The bogus URL for the error page (data:text/html,chromewebdata is the URL, I believe). At least for the main frame, order is this: DidStartProvisionalLoad for original URL DidFailProvisionalLoad for original URL DidStartProvisionalLoad for the error page DidCommitProvisionalLoad for the error page I assume we have the same events for subframes as well. I'm not sure if there's an even for when the load of the error page completes, but if there is, ideally we should be waiting for that.
On 2013/05/23 19:29:59, mmenke wrote: > On 2013/05/23 19:03:16, nasko wrote: > > On 2013/05/23 18:25:47, mmenke wrote: > > > On 2013/05/23 18:21:17, nasko wrote: > > > > I've changed the test to use explicit DidFailProvisionalLoad observer and > > > ensure > > > > that we see the expected failure. > > > > I think this is more of what I initially had in mind. > > > > > > > > Please review. > > > > > > Quick question: Is this waiting long enough, or should we wait until the > > error > > > page is committed, after the failure? > > > > Which page is expected to commit? http://mock.failed.request/-105 or the data: > > URL for the actual error text? > > The bogus URL for the error page (data:text/html,chromewebdata is the URL, I > believe). > > At least for the main frame, order is this: > > DidStartProvisionalLoad for original URL > DidFailProvisionalLoad for original URL > DidStartProvisionalLoad for the error page > DidCommitProvisionalLoad for the error page > > I assume we have the same events for subframes as well. I'm not sure if there's > an even for when the load of the error page completes, but if there is, ideally > we should be waiting for that. The events are the same, though the commit comes for the original URL. I've made it a bit simpler - just wait on LOAD_STOP event and record the fail URL to ensure we've failed. New version is ready for review.
On 2013/05/23 20:35:13, nasko wrote: > On 2013/05/23 19:29:59, mmenke wrote: > > On 2013/05/23 19:03:16, nasko wrote: > > > On 2013/05/23 18:25:47, mmenke wrote: > > > > On 2013/05/23 18:21:17, nasko wrote: > > > > > I've changed the test to use explicit DidFailProvisionalLoad observer > and > > > > ensure > > > > > that we see the expected failure. > > > > > I think this is more of what I initially had in mind. > > > > > > > > > > Please review. > > > > > > > > Quick question: Is this waiting long enough, or should we wait until the > > > error > > > > page is committed, after the failure? > > > > > > Which page is expected to commit? http://mock.failed.request/-105 or the > data: > > > URL for the actual error text? > > > > The bogus URL for the error page (data:text/html,chromewebdata is the URL, I > > believe). > > > > At least for the main frame, order is this: > > > > DidStartProvisionalLoad for original URL > > DidFailProvisionalLoad for original URL > > DidStartProvisionalLoad for the error page > > DidCommitProvisionalLoad for the error page > > > > I assume we have the same events for subframes as well. I'm not sure if > there's > > an even for when the load of the error page completes, but if there is, > ideally > > we should be waiting for that. > > The events are the same, though the commit comes for the original URL. I've made > it a bit simpler - just wait on LOAD_STOP event and record the fail URL to > ensure we've failed. New version is ready for review. DidFailProvisionalLoad happens well before LOAD_STOP, actually. LOAD_STOP only happens once the error page has committed. I'm going to apply your patch locally and investigate the ordering of events. Sorry for holding up the CL, but I've learned to be paranoid about event ordering with browser tests.
On 2013/05/23 20:39:05, mmenke wrote: > On 2013/05/23 20:35:13, nasko wrote: > > On 2013/05/23 19:29:59, mmenke wrote: > > > On 2013/05/23 19:03:16, nasko wrote: > > > > On 2013/05/23 18:25:47, mmenke wrote: > > > > > On 2013/05/23 18:21:17, nasko wrote: > > > > > > I've changed the test to use explicit DidFailProvisionalLoad observer > > and > > > > > ensure > > > > > > that we see the expected failure. > > > > > > I think this is more of what I initially had in mind. > > > > > > > > > > > > Please review. > > > > > > > > > > Quick question: Is this waiting long enough, or should we wait until > the > > > > error > > > > > page is committed, after the failure? > > > > > > > > Which page is expected to commit? http://mock.failed.request/-105 or the > > data: > > > > URL for the actual error text? > > > > > > The bogus URL for the error page (data:text/html,chromewebdata is the URL, I > > > believe). > > > > > > At least for the main frame, order is this: > > > > > > DidStartProvisionalLoad for original URL > > > DidFailProvisionalLoad for original URL > > > DidStartProvisionalLoad for the error page > > > DidCommitProvisionalLoad for the error page > > > > > > I assume we have the same events for subframes as well. I'm not sure if > > there's > > > an even for when the load of the error page completes, but if there is, > > ideally > > > we should be waiting for that. > > > > The events are the same, though the commit comes for the original URL. I've > made > > it a bit simpler - just wait on LOAD_STOP event and record the fail URL to > > ensure we've failed. New version is ready for review. > > DidFailProvisionalLoad happens well before LOAD_STOP, actually. LOAD_STOP only > happens once the error page has committed. > > I'm going to apply your patch locally and investigate the ordering of events. > Sorry for holding up the CL, but I've learned to be paranoid about event > ordering with browser tests. No problem on being paranoid, I share the same mentality. The LOAD_STOP does happen after the DidFailProvisionalLoad, you are correct. The idea is to wait as late as possible, so there has been a commit, which in general is the step at which we create a history entry AFAIK. The test for history entry should be done as late as possible, which LOAD_STOP seemed appropriate for.
LGTM! https://codereview.chromium.org/15294012/diff/23002/chrome/browser/errorpage_... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/15294012/diff/23002/chrome/browser/errorpage_... chrome/browser/errorpage_browsertest.cc:304: load_observer.Wait(); Sorry. Just skimmed it last time, and didn't notice the two different observer, which is why I was confused.
Just want to be sure we're on the right track. https://codereview.chromium.org/15294012/diff/23002/chrome/browser/errorpage_... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/15294012/diff/23002/chrome/browser/errorpage_... chrome/browser/errorpage_browsertest.cc:300: content::NOTIFICATION_LOAD_STOP, nit: Wrong indent. https://codereview.chromium.org/15294012/diff/23002/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/15294012/diff/23002/content/renderer/render_v... content/renderer/render_view_impl.cc:3421: document_state->navigation_state()->pending_page_id() == -1) { It's hard to tell whether this fix is correct, since it's not clear how these conditions relate to each other. What's the goal here? Is it that all initial iframe navigations should be considered auto-subframe? Is pending_page_id() == -1 a good indicator of that? If so, do we still need the isLoading clause? Maybe a comment about what cases this is trying to address would help.
I've updated this CL with a new approach to fixing this bug. It depends on Blink change, which is currently up for review at https://codereview.chromium.org/24286007/. https://codereview.chromium.org/15294012/diff/23002/chrome/browser/errorpage_... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/15294012/diff/23002/chrome/browser/errorpage_... chrome/browser/errorpage_browsertest.cc:300: content::NOTIFICATION_LOAD_STOP, On 2013/05/23 21:53:33, creis wrote: > nit: Wrong indent. Done. https://codereview.chromium.org/15294012/diff/23002/chrome/browser/errorpage_... chrome/browser/errorpage_browsertest.cc:304: load_observer.Wait(); On 2013/05/23 21:13:39, mmenke wrote: > Sorry. Just skimmed it last time, and didn't notice the two different observer, > which is why I was confused. Done. https://codereview.chromium.org/15294012/diff/23002/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/15294012/diff/23002/content/renderer/render_v... content/renderer/render_view_impl.cc:3421: document_state->navigation_state()->pending_page_id() == -1) { On 2013/05/23 21:53:33, creis wrote: > It's hard to tell whether this fix is correct, since it's not clear how these > conditions relate to each other. > > What's the goal here? Is it that all initial iframe navigations should be > considered auto-subframe? Is pending_page_id() == -1 a good indicator of that? > If so, do we still need the isLoading clause? > > Maybe a comment about what cases this is trying to address would help. This is now completely rewritten.
https://codereview.chromium.org/15294012/diff/51001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/15294012/diff/51001/content/renderer/render_v... content/renderer/render_view_impl.cc:3553: !frame->hasCommittedRealDocument()) { I'm having a hard time understanding what the hasCommittedRealDocument() check is doing. If we haven't committed a real document, then it means we are either showing the initial empty document or we have started a provisional load on the frame that hasn't committed yet. In this latter case, how do you know it is AUTO_SUBFRAME? The case of replacesCurrentHistoryItem() returning false corresponds to the case of creating a new back/forward entry. AUTO_SUBFRAME should never be used when we create a new back/forward entry. I'm confused.
https://codereview.chromium.org/15294012/diff/51001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/15294012/diff/51001/content/renderer/render_v... content/renderer/render_view_impl.cc:3553: !frame->hasCommittedRealDocument()) { On 2013/09/20 22:09:38, darin wrote: > I'm having a hard time understanding what the hasCommittedRealDocument() check > is doing. > > If we haven't committed a real document, then it means we are either showing the > initial empty document or we have started a provisional load on the frame that > hasn't committed yet. In this latter case, how do you know it is AUTO_SUBFRAME? > The case of replacesCurrentHistoryItem() returning false corresponds to the case > of creating a new back/forward entry. AUTO_SUBFRAME should never be used when we > create a new back/forward entry. > > I'm confused. The problem with solely relying on replacesCurrentHistoryItem is with this code fragment: var f = document.createElement('iframe'); document.body.appendChild(f); This delivers a didStartProvisionalLoad for "about:blank" and replacesCurrentHistoryItem returns false. This would mean that we need to add a session history entry for this navigation, which isn't correct, as this is not a real document load. This is why I'm checking whether we have loaded a real document in the frame, so I can disambiguate this case.
On 2013/09/20 22:29:59, nasko wrote: > https://codereview.chromium.org/15294012/diff/51001/content/renderer/render_v... > File content/renderer/render_view_impl.cc (right): > > https://codereview.chromium.org/15294012/diff/51001/content/renderer/render_v... > content/renderer/render_view_impl.cc:3553: !frame->hasCommittedRealDocument()) { > On 2013/09/20 22:09:38, darin wrote: > > I'm having a hard time understanding what the hasCommittedRealDocument() check > > is doing. > > > > If we haven't committed a real document, then it means we are either showing > the > > initial empty document or we have started a provisional load on the frame that > > hasn't committed yet. In this latter case, how do you know it is > AUTO_SUBFRAME? > > The case of replacesCurrentHistoryItem() returning false corresponds to the > case > > of creating a new back/forward entry. AUTO_SUBFRAME should never be used when > we > > create a new back/forward entry. > > > > I'm confused. > > The problem with solely relying on replacesCurrentHistoryItem is with this code > fragment: > > var f = document.createElement('iframe'); > document.body.appendChild(f); > > This delivers a didStartProvisionalLoad for "about:blank" and > replacesCurrentHistoryItem returns false. This would mean that we need to add a > session history entry for this navigation, which isn't correct, as this is not a > real document load. > > This is why I'm checking whether we have loaded a real document in the frame, so > I can disambiguate this case. I see. Hmm... I guess this means that replacesCurrentHistoryItem() cannot in general be used to determine when a new b/f list entry will be generated. Afterall, it is true that we cannot replace a HistoryItem that does not exist. The initial document load is creating the first HistoryItem. Stepping back, it seems like this CL would have a bug if the first real document load is started that does not replace the current HistoryItem. It seems like you need to check if we are loading (or have only loaded) the initial empty document. I wonder if there isn't a way to evolve WebDataSource::replacesCurrentHistoryItem with something that better indicates whether or not this WebDataSource is going to result in a new b/f entry. Hmm...
On 2013/09/20 22:47:20, darin wrote: > On 2013/09/20 22:29:59, nasko wrote: > > > https://codereview.chromium.org/15294012/diff/51001/content/renderer/render_v... > > File content/renderer/render_view_impl.cc (right): > > > > > https://codereview.chromium.org/15294012/diff/51001/content/renderer/render_v... > > content/renderer/render_view_impl.cc:3553: !frame->hasCommittedRealDocument()) > { > > On 2013/09/20 22:09:38, darin wrote: > > > I'm having a hard time understanding what the hasCommittedRealDocument() > check > > > is doing. > > > > > > If we haven't committed a real document, then it means we are either showing > > the > > > initial empty document or we have started a provisional load on the frame > that > > > hasn't committed yet. In this latter case, how do you know it is > > AUTO_SUBFRAME? > > > The case of replacesCurrentHistoryItem() returning false corresponds to the > > case > > > of creating a new back/forward entry. AUTO_SUBFRAME should never be used > when > > we > > > create a new back/forward entry. > > > > > > I'm confused. > > > > The problem with solely relying on replacesCurrentHistoryItem is with this > code > > fragment: > > > > var f = document.createElement('iframe'); > > document.body.appendChild(f); > > > > This delivers a didStartProvisionalLoad for "about:blank" and > > replacesCurrentHistoryItem returns false. This would mean that we need to add > a > > session history entry for this navigation, which isn't correct, as this is not > a > > real document load. > > > > This is why I'm checking whether we have loaded a real document in the frame, > so > > I can disambiguate this case. > > I see. Hmm... > > I guess this means that replacesCurrentHistoryItem() cannot in general be used > to determine when a new b/f list entry will be generated. Afterall, it is true > that we cannot replace a HistoryItem that does not exist. The initial document > load is creating the first HistoryItem. > > Stepping back, it seems like this CL would have a bug if the first real > document load is started that does not replace the current HistoryItem. It > seems like you need to check if we are loading (or have only loaded) the > initial empty document. What is a case where we have the first real document load that doesn't replace the current history item? As referenced in HistoryController::currentItemShouldBeReplaced: // From the HTML5 spec for location.assign(): // "If the browsing context's session history contains only one Document, // and that was the about:blank Document created when the browsing context // was created, then the navigation must be done with replacement enabled." > I wonder if there isn't a way to evolve > WebDataSource::replacesCurrentHistoryItem > with something that better indicates whether or not this WebDataSource is going > to result in a new b/f entry. Hmm... It looks like the logic here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... can be augmented to do setReplacesCurrentHistoryItem if we are in state FrameLoaderStateMachine::StartedFirstRealLoad. Example patch: - m_policyDocumentLoader->setReplacesCurrentHistoryItem(type == FrameLoadTypeRedirectWithLockedBackForwardList); + m_policyDocumentLoader->setReplacesCurrentHistoryItem(type == FrameLoadTypeRedirectWithLockedBackForwardList || m_stateMachine.startedFirstRealLoad());
On 2013/09/20 23:14:02, nasko wrote: > On 2013/09/20 22:47:20, darin wrote: > > On 2013/09/20 22:29:59, nasko wrote: > > > > > > https://codereview.chromium.org/15294012/diff/51001/content/renderer/render_v... > > > File content/renderer/render_view_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/15294012/diff/51001/content/renderer/render_v... > > > content/renderer/render_view_impl.cc:3553: > !frame->hasCommittedRealDocument()) > > { > > > On 2013/09/20 22:09:38, darin wrote: > > > > I'm having a hard time understanding what the hasCommittedRealDocument() > > check > > > > is doing. > > > > > > > > If we haven't committed a real document, then it means we are either > showing > > > the > > > > initial empty document or we have started a provisional load on the frame > > that > > > > hasn't committed yet. In this latter case, how do you know it is > > > AUTO_SUBFRAME? > > > > The case of replacesCurrentHistoryItem() returning false corresponds to > the > > > case > > > > of creating a new back/forward entry. AUTO_SUBFRAME should never be used > > when > > > we > > > > create a new back/forward entry. > > > > > > > > I'm confused. > > > > > > The problem with solely relying on replacesCurrentHistoryItem is with this > > code > > > fragment: > > > > > > var f = document.createElement('iframe'); > > > document.body.appendChild(f); > > > > > > This delivers a didStartProvisionalLoad for "about:blank" and > > > replacesCurrentHistoryItem returns false. This would mean that we need to > add > > a > > > session history entry for this navigation, which isn't correct, as this is > not > > a > > > real document load. > > > > > > This is why I'm checking whether we have loaded a real document in the > frame, > > so > > > I can disambiguate this case. > > > > I see. Hmm... > > > > I guess this means that replacesCurrentHistoryItem() cannot in general be used > > to determine when a new b/f list entry will be generated. Afterall, it is true > > that we cannot replace a HistoryItem that does not exist. The initial document > > load is creating the first HistoryItem. > > > > Stepping back, it seems like this CL would have a bug if the first real > > document load is started that does not replace the current HistoryItem. It > > seems like you need to check if we are loading (or have only loaded) the > > initial empty document. > > What is a case where we have the first real document load that doesn't replace > the current history item? > As referenced in HistoryController::currentItemShouldBeReplaced: > > // From the HTML5 spec for location.assign(): > // "If the browsing context's session history contains only one Document, > // and that was the about:blank Document created when the browsing context > // was created, then the navigation must be done with replacement > enabled." > > > I wonder if there isn't a way to evolve > > WebDataSource::replacesCurrentHistoryItem > > with something that better indicates whether or not this WebDataSource is > going > > to result in a new b/f entry. Hmm... > > It looks like the logic here: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > can be augmented to do setReplacesCurrentHistoryItem if we are in state > FrameLoaderStateMachine::StartedFirstRealLoad. Example patch: > > - m_policyDocumentLoader->setReplacesCurrentHistoryItem(type == > FrameLoadTypeRedirectWithLockedBackForwardList); > + m_policyDocumentLoader->setReplacesCurrentHistoryItem(type == > FrameLoadTypeRedirectWithLockedBackForwardList || > m_stateMachine.startedFirstRealLoad()); Ok, m_stateMachine.startedFirstRealLoad() is not exactly what we need, since it is true forever after we start loading the first real load. The semantics I wanted to express is "if we are starting to load the first real document, but haven't completed doing so".
On 2013/09/20 23:24:17, nasko wrote: > On 2013/09/20 23:14:02, nasko wrote: > > On 2013/09/20 22:47:20, darin wrote: > > > On 2013/09/20 22:29:59, nasko wrote: > > > > > > > > > > https://codereview.chromium.org/15294012/diff/51001/content/renderer/render_v... > > > > File content/renderer/render_view_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/15294012/diff/51001/content/renderer/render_v... > > > > content/renderer/render_view_impl.cc:3553: > > !frame->hasCommittedRealDocument()) > > > { > > > > On 2013/09/20 22:09:38, darin wrote: > > > > > I'm having a hard time understanding what the hasCommittedRealDocument() > > > check > > > > > is doing. > > > > > > > > > > If we haven't committed a real document, then it means we are either > > showing > > > > the > > > > > initial empty document or we have started a provisional load on the > frame > > > that > > > > > hasn't committed yet. In this latter case, how do you know it is > > > > AUTO_SUBFRAME? > > > > > The case of replacesCurrentHistoryItem() returning false corresponds to > > the > > > > case > > > > > of creating a new back/forward entry. AUTO_SUBFRAME should never be used > > > when > > > > we > > > > > create a new back/forward entry. > > > > > > > > > > I'm confused. > > > > > > > > The problem with solely relying on replacesCurrentHistoryItem is with this > > > code > > > > fragment: > > > > > > > > var f = document.createElement('iframe'); > > > > document.body.appendChild(f); > > > > > > > > This delivers a didStartProvisionalLoad for "about:blank" and > > > > replacesCurrentHistoryItem returns false. This would mean that we need to > > add > > > a > > > > session history entry for this navigation, which isn't correct, as this is > > not > > > a > > > > real document load. > > > > > > > > This is why I'm checking whether we have loaded a real document in the > > frame, > > > so > > > > I can disambiguate this case. > > > > > > I see. Hmm... > > > > > > I guess this means that replacesCurrentHistoryItem() cannot in general be > used > > > to determine when a new b/f list entry will be generated. Afterall, it is > true > > > that we cannot replace a HistoryItem that does not exist. The initial > document > > > load is creating the first HistoryItem. > > > > > > Stepping back, it seems like this CL would have a bug if the first real > > > document load is started that does not replace the current HistoryItem. It > > > seems like you need to check if we are loading (or have only loaded) the > > > initial empty document. > > > > What is a case where we have the first real document load that doesn't replace > > the current history item? > > As referenced in HistoryController::currentItemShouldBeReplaced: > > > > // From the HTML5 spec for location.assign(): > > // "If the browsing context's session history contains only one Document, > > // and that was the about:blank Document created when the browsing > context > > // was created, then the navigation must be done with replacement > > enabled." > > > > > I wonder if there isn't a way to evolve > > > WebDataSource::replacesCurrentHistoryItem > > > with something that better indicates whether or not this WebDataSource is > > going > > > to result in a new b/f entry. Hmm... > > > > It looks like the logic here: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > can be augmented to do setReplacesCurrentHistoryItem if we are in state > > FrameLoaderStateMachine::StartedFirstRealLoad. Example patch: > > > > - m_policyDocumentLoader->setReplacesCurrentHistoryItem(type == > > FrameLoadTypeRedirectWithLockedBackForwardList); > > + m_policyDocumentLoader->setReplacesCurrentHistoryItem(type == > > FrameLoadTypeRedirectWithLockedBackForwardList || > > m_stateMachine.startedFirstRealLoad()); > > Ok, m_stateMachine.startedFirstRealLoad() is not exactly what we need, since it > is true forever after we start loading the first real load. The semantics I > wanted to express is "if we are starting to load the first real document, but > haven't completed doing so". Darin, should I try fixing setReplacesCurrentHistoryItem or proceed with the new API?
My inclination is to "fix" setReplacesCurrentHistoryItem, since I believe the first empty document has a HistoryItem that gets replaced when we subsequently navigate the frame. -Darin On Tue, Sep 24, 2013 at 10:25 AM, <nasko@chromium.org> wrote: > On 2013/09/20 23:24:17, nasko wrote: > >> On 2013/09/20 23:14:02, nasko wrote: >> > On 2013/09/20 22:47:20, darin wrote: >> > > On 2013/09/20 22:29:59, nasko wrote: >> > > > >> > > >> > >> > > https://codereview.chromium.**org/15294012/diff/51001/** > content/renderer/render_view_**impl.cc<https://codereview.chromium.org/15294012/diff/51001/content/renderer/render_view_impl.cc> > >> > > > File content/renderer/render_view_**impl.cc (right): >> > > > >> > > > >> > > >> > >> > > https://codereview.chromium.**org/15294012/diff/51001/** > content/renderer/render_view_**impl.cc#newcode3553<https://codereview.chromium.org/15294012/diff/51001/content/renderer/render_view_impl.cc#newcode3553> > >> > > > content/renderer/render_view_**impl.cc:3553: >> > !frame->**hasCommittedRealDocument()) >> > > { >> > > > On 2013/09/20 22:09:38, darin wrote: >> > > > > I'm having a hard time understanding what the >> > hasCommittedRealDocument() > >> > > check >> > > > > is doing. >> > > > > >> > > > > If we haven't committed a real document, then it means we are >> either >> > showing >> > > > the >> > > > > initial empty document or we have started a provisional load on >> the >> frame >> > > that >> > > > > hasn't committed yet. In this latter case, how do you know it is >> > > > AUTO_SUBFRAME? >> > > > > The case of replacesCurrentHistoryItem() returning false >> corresponds >> > to > >> > the >> > > > case >> > > > > of creating a new back/forward entry. AUTO_SUBFRAME should never >> be >> > used > >> > > when >> > > > we >> > > > > create a new back/forward entry. >> > > > > >> > > > > I'm confused. >> > > > >> > > > The problem with solely relying on replacesCurrentHistoryItem is >> with >> > this > >> > > code >> > > > fragment: >> > > > >> > > > var f = document.createElement('**iframe'); >> > > > document.body.appendChild(f); >> > > > >> > > > This delivers a didStartProvisionalLoad for "about:blank" and >> > > > replacesCurrentHistoryItem returns false. This would mean that we >> need >> > to > >> > add >> > > a >> > > > session history entry for this navigation, which isn't correct, as >> this >> > is > >> > not >> > > a >> > > > real document load. >> > > > >> > > > This is why I'm checking whether we have loaded a real document in >> the >> > frame, >> > > so >> > > > I can disambiguate this case. >> > > >> > > I see. Hmm... >> > > >> > > I guess this means that replacesCurrentHistoryItem() cannot in >> general be >> used >> > > to determine when a new b/f list entry will be generated. Afterall, >> it is >> true >> > > that we cannot replace a HistoryItem that does not exist. The initial >> document >> > > load is creating the first HistoryItem. >> > > >> > > Stepping back, it seems like this CL would have a bug if the first >> real >> > > document load is started that does not replace the current >> HistoryItem. It >> > > seems like you need to check if we are loading (or have only loaded) >> the >> > > initial empty document. >> > >> > What is a case where we have the first real document load that doesn't >> > replace > >> > the current history item? >> > As referenced in HistoryController::**currentItemShouldBeReplaced: >> > >> > // From the HTML5 spec for location.assign(): >> > // "If the browsing context's session history contains only one >> > Document, > >> > // and that was the about:blank Document created when the browsing >> context >> > // was created, then the navigation must be done with replacement >> > enabled." >> > >> > > I wonder if there isn't a way to evolve >> > > WebDataSource::**replacesCurrentHistoryItem >> > > with something that better indicates whether or not this >> WebDataSource is >> > going >> > > to result in a new b/f entry. Hmm... >> > >> > It looks like the logic here: >> > >> > >> > > https://code.google.com/p/**chromium/codesearch#chromium/** > src/third_party/WebKit/Source/**core/loader/FrameLoader.cpp&** > sq=package:chromium&rcl=**1379309038&type=cs&l=1354<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/loader/FrameLoader.cpp&sq=package:chromium&rcl=1379309038&type=cs&l=1354> > >> > >> > can be augmented to do setReplacesCurrentHistoryItem if we are in state >> > FrameLoaderStateMachine::**StartedFirstRealLoad. Example patch: >> > >> > - m_policyDocumentLoader->**setReplacesCurrentHistoryItem(**type == >> > FrameLoadTypeRedirectWithLocke**dBackForwardList); >> > + m_policyDocumentLoader->**setReplacesCurrentHistoryItem(**type == >> > FrameLoadTypeRedirectWithLocke**dBackForwardList || >> > m_stateMachine.**startedFirstRealLoad()); >> > > Ok, m_stateMachine.**startedFirstRealLoad() is not exactly what we need, >> since >> > it > >> is true forever after we start loading the first real load. The semantics >> I >> wanted to express is "if we are starting to load the first real document, >> but >> haven't completed doing so". >> > > Darin, should I try fixing setReplacesCurrentHistoryItem or proceed with > the new > API? > > https://codereview.chromium.**org/15294012/<https://codereview.chromium.org/1... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I've updated the patch to match the new approach on the Blink side. I've also added a variation to the test.
LGTM
Thanks for working through this! LGTM with nits. https://codereview.chromium.org/15294012/diff/64001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/15294012/diff/64001/content/renderer/render_v... content/renderer/render_view_impl.cc:3562: } else { Style nit: Why move away from "else if"? https://codereview.chromium.org/15294012/diff/64001/content/renderer/render_v... content/renderer/render_view_impl.cc:3564: // Subframe navigations, which don't add session history items must be nit: "navigations, which" -> "navigations that" (no comma)
Fixed nits. https://codereview.chromium.org/15294012/diff/64001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/15294012/diff/64001/content/renderer/render_v... content/renderer/render_view_impl.cc:3562: } else { On 2013/10/02 15:45:49, creis wrote: > Style nit: Why move away from "else if"? Intermediate steps where I had more statements. Fixed now. https://codereview.chromium.org/15294012/diff/64001/content/renderer/render_v... content/renderer/render_view_impl.cc:3564: // Subframe navigations, which don't add session history items must be On 2013/10/02 15:45:49, creis wrote: > nit: "navigations, which" -> "navigations that" (no comma) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/15294012/72001
Message was sent while issue was closed.
Change committed as 226669 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
