Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2196333002:
PlzNavigate: Clear provisional history item on redirects. (Closed)
|
Created:
4 years, 4 months ago by arthursonzogni Modified:
4 years, 4 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, tyoshino+watch_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, loading-reviews_chromium.org, dglazkov+blink, darin-cc_chromium.org, gavinp+loader_chromium.org, blink-reviews, kinuko+watch, Nate Chapin, blink-reviews-api_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@timing_api Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: Clear provisional history item on redirects.
When a redirect happens during a back/forward navigation. We mustn't
reuse the provisional history item.
This patch is close to this one : https://codereview.chromium.org/2159093002,
With PlzNavigate enabled, redirections happen on the browser-side.
The provisional history item must still be discarded in this case too.
The two following tests pass and have been reactivated.
TEST=NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect,
NavigationControllerBrowserTest.FrameNavigationEntry_SameOriginBackWithRedirect
in --enabled-browser-side-navigation mode
R=creis@chromium.org, japhet@chromium.org
BUG=585194
Committed: https://crrev.com/932235bf7cf90cb2a5fe001dde45e5904786155d
Cr-Commit-Position: refs/heads/master@{#413730}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Rebasing... #Patch Set 3 : Addressed comments #
Total comments: 8
Patch Set 4 : Addressed comments (2) #
Total comments: 6
Patch Set 5 : Refactoring. #Patch Set 6 : Change tests filter #
Total comments: 2
Patch Set 7 : Use protected visibility. #Patch Set 8 : Rebasing + merge conflicts #Patch Set 9 : Rebasing again, Trybot issues. #Messages
Total messages: 39 (18 generated)
Description was changed from ========== PlzNavigate: Clear provisional history item on redirects. When a redirect happen during a back/forward navigation. We mustn't reuse the provisional history item. This patch is close to this one : https://codereview.chromium.org/2159093002, But this time, it's done one the browser side (with PlzNavigate enabled). The two following tests pass and have been reactivated. TEST=NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect, NavigationControllerBrowserTest.FrameNavigationEntry_SameOriginBackWithRedirect in --enabled-browser-side-navigation mode R=mkwst@chromium.org, nasko@chromium.org BUG= ========== to ========== PlzNavigate: Clear provisional history item on redirects. When a redirect happens during a back/forward navigation. We mustn't reuse the provisional history item. This patch is close to this one : https://codereview.chromium.org/2159093002, But this time, it's done one the browser side (with PlzNavigate enabled). The two following tests pass and have been reactivated. TEST=NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect, NavigationControllerBrowserTest.FrameNavigationEntry_SameOriginBackWithRedirect in --enabled-browser-side-navigation mode R=mkwst@chromium.org, nasko@chromium.org BUG= ==========
Please take a look at this bugfix. During a back/forward navigation, the FrameLoader was reusing the previous HistoryItem, even if there was a redirection this time. Thanks.
I don't think I'm the best reviewer for this CL. japhet@ is much more knowledgeable about this area of Blink and creis@ knows the browser side too (plus he wrote the patch you point to). https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebDataSourceImpl.cpp (right): https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebDataSourceImpl.cpp:88: if (redirectChain.size() >= 2) How do we know this is a back/forward navigation here? Wouldn't this be hit by just doing a regular navigation with redirects?
arthursonzogni@chromium.org changed reviewers: + creis@chromium.org, japhet@chromium.org - mkwst@chromium.org, nasko@chromium.org
Hi creis@ and japhet@. Please take a look at this bugfix. Thanks! Creis@ normally, this patch is very similar to another one you did, but this time on the browser-side. https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebDataSourceImpl.cpp (right): https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebDataSourceImpl.cpp:88: if (redirectChain.size() >= 2) On 2016/08/03 16:31:25, nasko (slow) wrote: > How do we know this is a back/forward navigation here? Wouldn't this be hit by > just doing a regular navigation with redirects? Yes it is. But from what I understood, m_provisionalItem is non-NULL and used only when the navigation is back/forward. Thus, during regular navigation the provisionalItem is already NULL. But I will add japhet@ and creis@ to be sure.
On 2016/08/04 14:04:53, arthur.sonzogni wrote: > Hi creis@ and japhet@. > Please take a look at this bugfix. Thanks! > > Creis@ normally, this patch is very similar to another one you did, but this > time on the browser-side. > > https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/w... > File third_party/WebKit/Source/web/WebDataSourceImpl.cpp (right): > > https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/web/WebDataSourceImpl.cpp:88: if (redirectChain.size() > >= 2) > On 2016/08/03 16:31:25, nasko (slow) wrote: > > How do we know this is a back/forward navigation here? Wouldn't this be hit by > > just doing a regular navigation with redirects? > > Yes it is. But from what I understood, m_provisionalItem is non-NULL and used > only when the navigation is back/forward. Thus, during regular navigation the > provisionalItem is already NULL. > > But I will add japhet@ and creis@ to be sure. Happy to review it! I'm at an offsite for the next two days, but I can take a look on Monday (unless I get some free time before).
Thanks for getting this to work in PlzNavigate. I think we just want to confirm the fix is in the right place. Maybe Nate can help. I'll note that the CL description is a bit confusing about saying the fix is on the browser side, since there's no changes to browser process code. Maybe you can clarify it? Also, please list a bug number on CLs when you can. In this case, 585194 would do. https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/DocumentLoader.h (right): https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/DocumentLoader.h:155: void discardProvisionalHistoryItem(); It doesn't seem like DocumentLoader should know about provisional history items. If FrameLoader isn't hearing about the redirect directly in receivedMainResourceRedirect, then maybe this name should be more about redirects than history items. (I'll defer to Nate, though, who's more of a Blink reviewer than I am.) https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameLoader.cpp:358: void FrameLoader::receivedMainResourceRedirect(const KURL& newURL) Can you explain why this doesn't get called in PlzNavigate but WebDataSourceImpl::updateNavigation does? I don't know as much about this part of PlzNavigate. https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebDataSourceImpl.cpp (right): https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebDataSourceImpl.cpp:78: void WebDataSourceImpl::updateNavigation(double redirectStartTime, double redirectEndTime, double fetchStartTime, const WebVector<WebURL>& redirectChain) It's not clear to me whether this is the right place to put the check, so maybe Nate can weigh in here. https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebDataSourceImpl.cpp:87: // any state from the previous HistoryItem nit: End comment sentences with a period. https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebDataSourceImpl.cpp:88: if (redirectChain.size() >= 2) On 2016/08/04 14:04:53, arthur.sonzogni wrote: > On 2016/08/03 16:31:25, nasko (slow) wrote: > > How do we know this is a back/forward navigation here? Wouldn't this be hit by > > just doing a regular navigation with redirects? > > Yes it is. But from what I understood, m_provisionalItem is non-NULL and used > only when the navigation is back/forward. Thus, during regular navigation the > provisionalItem is already NULL. > > But I will add japhet@ and creis@ to be sure. Right. That seems consistent with how FrameLoader::receivedMainResourceRedirect works, and it looks like FrameLoader only sets m_provisionalItem for back/forwards.
Description was changed from ========== PlzNavigate: Clear provisional history item on redirects. When a redirect happens during a back/forward navigation. We mustn't reuse the provisional history item. This patch is close to this one : https://codereview.chromium.org/2159093002, But this time, it's done one the browser side (with PlzNavigate enabled). The two following tests pass and have been reactivated. TEST=NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect, NavigationControllerBrowserTest.FrameNavigationEntry_SameOriginBackWithRedirect in --enabled-browser-side-navigation mode R=mkwst@chromium.org, nasko@chromium.org BUG= ========== to ========== PlzNavigate: Clear provisional history item on redirects. When a redirect happens during a back/forward navigation. We mustn't reuse the provisional history item. This patch is close to this one : https://codereview.chromium.org/2159093002, But this time, it's done one the browser side (with PlzNavigate enabled). The two following tests pass and have been reactivated. TEST=NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect, NavigationControllerBrowserTest.FrameNavigationEntry_SameOriginBackWithRedirect in --enabled-browser-side-navigation mode R=creis@chromium.org, japhet@chromium.org BUG=585194 ==========
Description was changed from ========== PlzNavigate: Clear provisional history item on redirects. When a redirect happens during a back/forward navigation. We mustn't reuse the provisional history item. This patch is close to this one : https://codereview.chromium.org/2159093002, But this time, it's done one the browser side (with PlzNavigate enabled). The two following tests pass and have been reactivated. TEST=NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect, NavigationControllerBrowserTest.FrameNavigationEntry_SameOriginBackWithRedirect in --enabled-browser-side-navigation mode R=creis@chromium.org, japhet@chromium.org BUG=585194 ========== to ========== PlzNavigate: Clear provisional history item on redirects. When a redirect happens during a back/forward navigation. We mustn't reuse the provisional history item. This patch is close to this one : https://codereview.chromium.org/2159093002, With PlzNavigate enabled, redirections happen on the browser-side. The provisional history item must still be discarded in this case too. The two following tests pass and have been reactivated. TEST=NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect, NavigationControllerBrowserTest.FrameNavigationEntry_SameOriginBackWithRedirect in --enabled-browser-side-navigation mode R=creis@chromium.org, japhet@chromium.org BUG=585194 ==========
Thanks for your review creis@. I answered some of your questions and renamed the method discardProvisionalItem() into addExternalRedirect(old_url, new_url) in the DocumentLoader. https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/DocumentLoader.h (right): https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/DocumentLoader.h:155: void discardProvisionalHistoryItem(); On 2016/08/09 05:07:31, Charlie Reis wrote: > It doesn't seem like DocumentLoader should know about provisional history items. > If FrameLoader isn't hearing about the redirect directly in > receivedMainResourceRedirect, then maybe this name should be more about > redirects than history items. > > (I'll defer to Nate, though, who's more of a Blink reviewer than I am.) I should use another name. You are right. https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameLoader.cpp:358: void FrameLoader::receivedMainResourceRedirect(const KURL& newURL) On 2016/08/09 05:07:31, Charlie Reis wrote: > Can you explain why this doesn't get called in PlzNavigate but > WebDataSourceImpl::updateNavigation does? I don't know as much about this part > of PlzNavigate. With PlzNavigate enabled, the whole navigation, redirection included, is done before the DocumentLoader is created. Thus, DocumentLoader::receivedMainResourceRedirect is not called anymore. The method DocumentLoader::updateNavigation is here to keep the internal properties up-to-date (timings, redirect counts, ...), as if the navigation had taken the old code path. https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebDataSourceImpl.cpp (right): https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebDataSourceImpl.cpp:78: void WebDataSourceImpl::updateNavigation(double redirectStartTime, double redirectEndTime, double fetchStartTime, const WebVector<WebURL>& redirectChain) On 2016/08/09 05:07:31, Charlie Reis wrote: > It's not clear to me whether this is the right place to put the check, so maybe > Nate can weigh in here. See another reply above. https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebDataSourceImpl.cpp:87: // any state from the previous HistoryItem On 2016/08/09 05:07:31, Charlie Reis wrote: > nit: End comment sentences with a period. Done.
Thanks. LGTM with nits, but please have Nate review as well. https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2196333002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameLoader.cpp:358: void FrameLoader::receivedMainResourceRedirect(const KURL& newURL) On 2016/08/10 10:02:24, arthur.sonzogni wrote: > On 2016/08/09 05:07:31, Charlie Reis wrote: > > Can you explain why this doesn't get called in PlzNavigate but > > WebDataSourceImpl::updateNavigation does? I don't know as much about this > part > > of PlzNavigate. > > With PlzNavigate enabled, the whole navigation, redirection included, is done > before the DocumentLoader is created. Thus, > DocumentLoader::receivedMainResourceRedirect is not called anymore. > > The method DocumentLoader::updateNavigation is here to keep the internal > properties up-to-date (timings, redirect counts, ...), as if the navigation had > taken the old code path. Acknowledged. https://codereview.chromium.org/2196333002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2196333002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:217: // any state from the old HistoryItem. It's probably worth mentioning that we only have a provisional history item for back/forward navigations, since this wasn't obvious at first glance. https://codereview.chromium.org/2196333002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:218: FrameLoader* frameloader = frameLoader(); nit: No need to declare this. (If we did have it, the spelling would be frameLoader.) https://codereview.chromium.org/2196333002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:219: if (frameloader) Is it possible for frameLoader() to return null here? Most of the other uses don't have a check for it. https://codereview.chromium.org/2196333002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.h (right): https://codereview.chromium.org/2196333002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.h:155: void addExternalRedirect(const KURL& oldURL, const KURL& newURL); Maybe didRedirect? "External redirect" isn't a term that most people will be familiar with, I think.
Thanks! I agree with all your comments. I will wait for Nate's review. https://codereview.chromium.org/2196333002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2196333002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:217: // any state from the old HistoryItem. On 2016/08/11 21:05:01, Charlie Reis (OOO til 8-30) wrote: > It's probably worth mentioning that we only have a provisional history item for > back/forward navigations, since this wasn't obvious at first glance. Done. https://codereview.chromium.org/2196333002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:218: FrameLoader* frameloader = frameLoader(); On 2016/08/11 21:05:01, Charlie Reis (OOO til 8-30) wrote: > nit: No need to declare this. (If we did have it, the spelling would be > frameLoader.) Done. https://codereview.chromium.org/2196333002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:219: if (frameloader) On 2016/08/11 21:05:01, Charlie Reis (OOO til 8-30) wrote: > Is it possible for frameLoader() to return null here? Most of the other uses > don't have a check for it. I am sure that where the function is called (in RenderFrameImpl::didCreateDataSource), the frameloader is not null. I think I should replace the condition with a DCHECK(...) https://codereview.chromium.org/2196333002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.h (right): https://codereview.chromium.org/2196333002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.h:155: void addExternalRedirect(const KURL& oldURL, const KURL& newURL); On 2016/08/11 21:05:01, Charlie Reis (OOO til 8-30) wrote: > Maybe didRedirect? "External redirect" isn't a term that most people will be > familiar with, I think. Done.
Sorry for the delay, I've been sick the last week. Couple of code health nitpicks... https://codereview.chromium.org/2196333002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2196333002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:212: void DocumentLoader::didRedirect(const KURL& oldURL, const KURL& newURL) Is there some redundant code between this and redirectReceived now? Can we have redirectReceived call didRedirect, esp. if FrameLoader::receivedMainResourceRedirect gets inlined in responseReceived? https://codereview.chromium.org/2196333002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2196333002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:374: void FrameLoader::receivedMainResourceRedirect(const KURL& newURL) This helper no longer seems useful now that a way to clear m_provisionalItem is exposed. Maybe inline it in DocumentLoader::redirectReceived? https://codereview.chromium.org/2196333002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:382: void FrameLoader::discardProvisionalHistoryItem() Nit: clearProvisionalHistoryItem(), for maximum accuracy? :)
Thank you Nate! I did the small refactoring you suggest. It is better now. Note: I had to disable one test I enabled in my first patch. It seems that the rebasing makes this test fail again. This test is not part of two tests I wanted to solve initially. https://codereview.chromium.org/2196333002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2196333002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:212: void DocumentLoader::didRedirect(const KURL& oldURL, const KURL& newURL) On 2016/08/17 23:45:52, Nate Chapin wrote: > Is there some redundant code between this and redirectReceived now? Can we have > redirectReceived call didRedirect, esp. if > FrameLoader::receivedMainResourceRedirect gets inlined in responseReceived? I tried to use didRedirect in redirectReceived. Now the provisionalHistoryItem is cleared in only one place. It is much better like this. Thanks! https://codereview.chromium.org/2196333002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2196333002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:374: void FrameLoader::receivedMainResourceRedirect(const KURL& newURL) On 2016/08/17 23:45:52, Nate Chapin wrote: > This helper no longer seems useful now that a way to clear m_provisionalItem is > exposed. Maybe inline it in DocumentLoader::redirectReceived? Done. https://codereview.chromium.org/2196333002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:382: void FrameLoader::discardProvisionalHistoryItem() On 2016/08/17 23:45:52, Nate Chapin wrote: > Nit: clearProvisionalHistoryItem(), for maximum accuracy? :) Done.
LGTM, thanks, one last question: https://codereview.chromium.org/2196333002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.h (right): https://codereview.chromium.org/2196333002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.h:158: void didRedirect(const KURL& oldURL, const KURL& newURL); Sorry, missed this earlier, should this be protected instead of public?
Thank you Nate. I think I can commit now. https://codereview.chromium.org/2196333002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.h (right): https://codereview.chromium.org/2196333002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.h:158: void didRedirect(const KURL& oldURL, const KURL& newURL); On 2016/08/18 16:49:57, Nate Chapin wrote: > Sorry, missed this earlier, should this be protected instead of public? Yes, you are right. I also removed the comment saying it is used to declare "external" redirection, since it is no more always used for this case.
The CQ bit was checked by arthursonzogni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, japhet@chromium.org Link to the patchset: https://codereview.chromium.org/2196333002/#ps120001 (title: "Use protected visibility.")
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 commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by arthursonzogni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, japhet@chromium.org Link to the patchset: https://codereview.chromium.org/2196333002/#ps140001 (title: "Rebasing + solve conflicts")
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 commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #8 (id:140001) has been deleted
The CQ bit was checked by arthursonzogni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, japhet@chromium.org Link to the patchset: https://codereview.chromium.org/2196333002/#ps160001 (title: "Rebasing + merge conflicts")
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 commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by arthursonzogni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, japhet@chromium.org Link to the patchset: https://codereview.chromium.org/2196333002/#ps180001 (title: "Rebasing again, Trybot issues.")
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 ========== PlzNavigate: Clear provisional history item on redirects. When a redirect happens during a back/forward navigation. We mustn't reuse the provisional history item. This patch is close to this one : https://codereview.chromium.org/2159093002, With PlzNavigate enabled, redirections happen on the browser-side. The provisional history item must still be discarded in this case too. The two following tests pass and have been reactivated. TEST=NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect, NavigationControllerBrowserTest.FrameNavigationEntry_SameOriginBackWithRedirect in --enabled-browser-side-navigation mode R=creis@chromium.org, japhet@chromium.org BUG=585194 ========== to ========== PlzNavigate: Clear provisional history item on redirects. When a redirect happens during a back/forward navigation. We mustn't reuse the provisional history item. This patch is close to this one : https://codereview.chromium.org/2159093002, With PlzNavigate enabled, redirections happen on the browser-side. The provisional history item must still be discarded in this case too. The two following tests pass and have been reactivated. TEST=NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect, NavigationControllerBrowserTest.FrameNavigationEntry_SameOriginBackWithRedirect in --enabled-browser-side-navigation mode R=creis@chromium.org, japhet@chromium.org BUG=585194 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Clear provisional history item on redirects. When a redirect happens during a back/forward navigation. We mustn't reuse the provisional history item. This patch is close to this one : https://codereview.chromium.org/2159093002, With PlzNavigate enabled, redirections happen on the browser-side. The provisional history item must still be discarded in this case too. The two following tests pass and have been reactivated. TEST=NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect, NavigationControllerBrowserTest.FrameNavigationEntry_SameOriginBackWithRedirect in --enabled-browser-side-navigation mode R=creis@chromium.org, japhet@chromium.org BUG=585194 ========== to ========== PlzNavigate: Clear provisional history item on redirects. When a redirect happens during a back/forward navigation. We mustn't reuse the provisional history item. This patch is close to this one : https://codereview.chromium.org/2159093002, With PlzNavigate enabled, redirections happen on the browser-side. The provisional history item must still be discarded in this case too. The two following tests pass and have been reactivated. TEST=NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect, NavigationControllerBrowserTest.FrameNavigationEntry_SameOriginBackWithRedirect in --enabled-browser-side-navigation mode R=creis@chromium.org, japhet@chromium.org BUG=585194 Committed: https://crrev.com/932235bf7cf90cb2a5fe001dde45e5904786155d Cr-Commit-Position: refs/heads/master@{#413730} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/932235bf7cf90cb2a5fe001dde45e5904786155d Cr-Commit-Position: refs/heads/master@{#413730} |
