Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2742923002:
Use SameDocument term in tests instead of SamePage. (Closed)
|
Created:
3 years, 9 months ago by Eugene But (OOO till 7-30) Modified:
3 years, 9 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, feature-media-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse SameDocument term in tests instead of SamePage.
This is follow up cleanup after renaming NavigationHandle::IsSamePage to
NavigationHandle::IsSameDocument in this CL:
https://codereview.chromium.org/2716493004/
BUG=695189
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2742923002
Cr-Commit-Position: refs/heads/master@{#456231}
Committed: https://chromium.googlesource.com/chromium/src/+/d62ba1f5529bb7bfa48ea80ea131c1b9fd0f17b6
Patch Set 1 #Patch Set 2 : Fixed compilation #
Total comments: 6
Patch Set 3 : Addressed review comments #
Messages
Total messages: 19 (13 generated)
Description was changed from ========== Use SameDocument term in tests instead of SamePage. This is follow up cleanup after renaming NavigationHandle::IsSamePage to NavigationHandle::IsSameDocument in this CL: https://codereview.chromium.org/2716493004/ BUG=695189 ========== to ========== Use SameDocument term in tests instead of SamePage. This is follow up cleanup after renaming NavigationHandle::IsSamePage to NavigationHandle::IsSameDocument in this CL: https://codereview.chromium.org/2716493004/ BUG=695189 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by eugenebut@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 eugenebut@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
eugenebut@chromium.org changed reviewers: + creis@chromium.org
Thanks! Almost everything in here looks right except for the cases I called out (having to do with NAVIGATION_TYPE_SAME_PAGE). This is even more motivation for your change in general, but we should be careful to distinguish between these two cases. https://codereview.chromium.org/2742923002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (left): https://codereview.chromium.org/2742923002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6041: // Test that navigations classified as SAME_PAGE properly update all the We should leave this as SAME_PAGE. (See below for details.) https://codereview.chromium.org/2742923002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6046: EnsureSamePageNavigationUpdatesFrameNavigationEntry) { We should leave the test name here as is, unless we plan to change NAVIGATION_TYPE_SAME_PAGE. (See below.) https://codereview.chromium.org/2742923002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2742923002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6083: // classified as same document) was leaving the FrameNavigationEntry with the No, this needs to stay SAME_PAGE for now, because it refers to a particular NAVIGATION_TYPE_SAME_PAGE classification that was important in reproducing the bug. Note that NAVIGATION_SAME_PAGE is not the same as the ones we're changing (e.g, fragments, pushstate, etc). It refers to cases like hitting enter in the address bar, where we reload the whole page by navigating to it again, without treating it as a reload. See this declaration comment: // The same page has been reloaded as a result of the user requesting // navigation to that same page (like pressing Enter in the URL bar). This // is not the same as an in-page navigation because we'll actually have a // pending entry for the load, which is then meaningless. NAVIGATION_TYPE_SAME_PAGE, It's possible we could update the phrasing of this, but I think it's important to distinguish it from the "same document" navigations we're referring to everywhere else (fragments, etc).
Yeah, I figured that NAVIGATION_TYPE_SAME_PAGE is not same-document navigation, but still was not sure if I'm making the right changes in tests :( PTAL https://codereview.chromium.org/2742923002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (left): https://codereview.chromium.org/2742923002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6041: // Test that navigations classified as SAME_PAGE properly update all the On 2017/03/10 21:01:38, Charlie Reis (slow) wrote: > We should leave this as SAME_PAGE. (See below for details.) Done. https://codereview.chromium.org/2742923002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6046: EnsureSamePageNavigationUpdatesFrameNavigationEntry) { On 2017/03/10 21:01:38, Charlie Reis (slow) wrote: > We should leave the test name here as is, unless we plan to change > NAVIGATION_TYPE_SAME_PAGE. (See below.) Done. https://codereview.chromium.org/2742923002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2742923002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6083: // classified as same document) was leaving the FrameNavigationEntry with the On 2017/03/10 21:01:38, Charlie Reis (slow) wrote: > No, this needs to stay SAME_PAGE for now, because it refers to a particular > NAVIGATION_TYPE_SAME_PAGE classification that was important in reproducing the > bug. > > Note that NAVIGATION_SAME_PAGE is not the same as the ones we're changing (e.g, > fragments, pushstate, etc). It refers to cases like hitting enter in the > address bar, where we reload the whole page by navigating to it again, without > treating it as a reload. See this declaration comment: > > // The same page has been reloaded as a result of the user requesting > // navigation to that same page (like pressing Enter in the URL bar). This > // is not the same as an in-page navigation because we'll actually have a > // pending entry for the load, which is then meaningless. > NAVIGATION_TYPE_SAME_PAGE, > > > It's possible we could update the phrasing of this, but I think it's important > to distinguish it from the "same document" navigations we're referring to > everywhere else (fragments, etc). Done.
Thanks! LGTM
The CQ bit was checked by eugenebut@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489189406994900,
"parent_rev": "8bcaea14967dab3bbfd0a13ccbb567c70614f149", "commit_rev":
"d62ba1f5529bb7bfa48ea80ea131c1b9fd0f17b6"}
Message was sent while issue was closed.
Description was changed from ========== Use SameDocument term in tests instead of SamePage. This is follow up cleanup after renaming NavigationHandle::IsSamePage to NavigationHandle::IsSameDocument in this CL: https://codereview.chromium.org/2716493004/ BUG=695189 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Use SameDocument term in tests instead of SamePage. This is follow up cleanup after renaming NavigationHandle::IsSamePage to NavigationHandle::IsSameDocument in this CL: https://codereview.chromium.org/2716493004/ BUG=695189 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2742923002 Cr-Commit-Position: refs/heads/master@{#456231} Committed: https://chromium.googlesource.com/chromium/src/+/d62ba1f5529bb7bfa48ea80ea131... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d62ba1f5529bb7bfa48ea80ea131... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
