Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21544005:
Take the navigation type into account when checking if the navigation is in page. (Closed)
|
Created:
7 years, 4 months ago by pstanek Modified:
7 years, 3 months ago Reviewers:
Finnur, Nate Chapin, jochen (gone - plz use gerrit), abarth-chromium, Charlie Reis, brettw CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionTake the navigation type into account when checking if the navigation is in page.
When navigating from ref url to non ref one the navigation type has decisive voice whether the navigation should be treated as in page.
BUG=253481
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216329
Patch Set 1 #
Total comments: 1
Patch Set 2 : Changing an approach a bit. #
Total comments: 3
Patch Set 3 : More generic and brave fix #
Total comments: 8
Patch Set 4 : Addressing review comments. #
Total comments: 13
Patch Set 5 : Dropping content::, comment & fix in the test. #
Total comments: 2
Patch Set 6 : Readability. Bringing back removed TC. #
Total comments: 3
Messages
Total messages: 32 (0 generated)
On 2013/08/01 15:53:07, pstanek wrote: No test as I have no really idea how to test it automatically.
https://codereview.chromium.org/21544005/diff/1/content/browser/web_contents/... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/21544005/diff/1/content/browser/web_contents/... content/browser/web_contents/web_contents_impl.cc:2689: // is false even the navigation is in fact in page Should we fix that?
https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/ssl/ss... ^^^ Maybe this test suite is a good one to extend with a test for this issue?
On 2013/08/01 20:48:32, abarth wrote: > https://codereview.chromium.org/21544005/diff/1/content/browser/web_contents/... > File content/browser/web_contents/web_contents_impl.cc (right): > > https://codereview.chromium.org/21544005/diff/1/content/browser/web_contents/... > content/browser/web_contents/web_contents_impl.cc:2689: // is false even the > navigation is in fact in page > Should we fix that? I would guess so as there is this comment in the main culprit here a.k.a AreURLsInPageNavigation(): // TODO(jcampan): what about when navigating back from a ref URL to the top // non ref URL? Nothing is loaded in that case but we return false here. // The user could also navigate from the ref URL to the non ref URL by // entering the non ref URL in the location bar or through a bookmark, in // which case there would be a load. I am not sure if the non-load/load // scenarios can be differentiated with the TransitionType. Also there is already a test testing this sort of NavigationControllerTest::InPage. It loads some URL with a fragment and tests going back. It has this code: // is_in_page is false in that case but should be true. // See comment in AreURLsInPageNavigation() in navigation_controller.cc // EXPECT_TRUE(details.is_in_page); Maybe is_in_page should be set to true in NavigationControllerImpl::RendererDidNavigate() after classifying the navigaton type if it turns out it's of the IN_PAGE type?
You should probably engage with an expert on this code. I haven't looked at it in a while.
@jochen: Please see https://codereview.chromium.org/21544005/#msg5 (apart from the CL & issue ofc)
Adding the browser and renderer side navigation experts.
On 2013/08/02 07:52:28, jochen wrote: > Adding the browser and renderer side navigation experts. for the record, I agree that changing the underlying code to return a consistent decision for whether the navigation is in page or not is preferable over working around the inconsistencies.
On 2013/08/02 07:53:42, jochen wrote: > On 2013/08/02 07:52:28, jochen wrote: > > Adding the browser and renderer side navigation experts. > > for the record, I agree that changing the underlying code to return a consistent > decision for whether the navigation is in page or not is preferable over working > around the inconsistencies. I fully agree this is why I was wondering about changing NavigationControllerImpl::RendererDidNavigate() in https://codereview.chromium.org/21544005/#msg5
On 2013/08/02 08:04:02, pstanek wrote: > On 2013/08/02 07:53:42, jochen wrote: > > On 2013/08/02 07:52:28, jochen wrote: > > > Adding the browser and renderer side navigation experts. > > > > for the record, I agree that changing the underlying code to return a > consistent > > decision for whether the navigation is in page or not is preferable over > working > > around the inconsistencies. > > I fully agree this is why I was wondering about changing > NavigationControllerImpl::RendererDidNavigate() in > https://codereview.chromium.org/21544005/#msg5 That sounds like a good plan to me, but I'm not very familiar with the is_in_page logic. Please confirm with Brett.
On 2013/08/02 16:58:45, creis wrote: > On 2013/08/02 08:04:02, pstanek wrote: > > On 2013/08/02 07:53:42, jochen wrote: > > > On 2013/08/02 07:52:28, jochen wrote: > > > > Adding the browser and renderer side navigation experts. > > > > > > for the record, I agree that changing the underlying code to return a > > consistent > > > decision for whether the navigation is in page or not is preferable over > > working > > > around the inconsistencies. > > > > I fully agree this is why I was wondering about changing > > NavigationControllerImpl::RendererDidNavigate() in > > https://codereview.chromium.org/21544005/#msg5 > > That sounds like a good plan to me, but I'm not very familiar with the > is_in_page logic. Please confirm with Brett. I've just done that change as it's a way better anyway. It's up to you if it good enough.
Looks like it's on the right track, but this is very tricky to understand. I think we should take this opportunity to make the code easier to follow rather than introducing another corner case. One suggestion is to move this check within AreURLsInPageNavigation rather than putting it at one of the call sites. That might eliminate some of the TODOs. https://codereview.chromium.org/21544005/diff/15001/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl.cc (right): https://codereview.chromium.org/21544005/diff/15001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl.cc:764: details->is_in_page = true; Putting it here doesn't help other callers of IsURLInPageNavigation or AreURLsInPageNavigation. Those functions will still return the wrong thing for the "back to non-ref" case. Maybe you can move the IsURLInPageNavigation call after ClassifyNavigation and pass in details->type? That might let us resolve jcampan's TODO statement in AreURLsInPageNavigation. We could default to passing in NAVIGATION_TYPE_UNKNOWN if we don't have the type. https://codereview.chromium.org/21544005/diff/15001/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/21544005/diff/15001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl_unittest.cc:1943: // EXPECT_TRUE(details.is_in_page); Can we fix this now? https://codereview.chromium.org/21544005/diff/15001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl_unittest.cc:2784: other_url_with_ref)); Maybe we can add a check and comment below saying that going back from a ref to a non-ref will be considered in-page if the renderer says it is. EXPECT_TRUE(controller.IsURLInPageNavigation(url, true, NAVIGATION_TYPE_IN_PAGE));
On 2013/08/05 22:18:10, creis wrote: > Looks like it's on the right track, but this is very tricky to understand. > > I think we should take this opportunity to make the code easier to follow rather > than introducing another corner case. One suggestion is to move this check > within AreURLsInPageNavigation rather than putting it at one of the call sites. > That might eliminate some of the TODOs. > > https://codereview.chromium.org/21544005/diff/15001/content/browser/web_conte... > File content/browser/web_contents/navigation_controller_impl.cc (right): > > https://codereview.chromium.org/21544005/diff/15001/content/browser/web_conte... > content/browser/web_contents/navigation_controller_impl.cc:764: > details->is_in_page = true; > Putting it here doesn't help other callers of IsURLInPageNavigation or > AreURLsInPageNavigation. Those functions will still return the wrong thing for > the "back to non-ref" case. > > Maybe you can move the IsURLInPageNavigation call after ClassifyNavigation and > pass in details->type? That might let us resolve jcampan's TODO statement in > AreURLsInPageNavigation. We could default to passing in NAVIGATION_TYPE_UNKNOWN > if we don't have the type. > > https://codereview.chromium.org/21544005/diff/15001/content/browser/web_conte... > File content/browser/web_contents/navigation_controller_impl_unittest.cc > (right): > > https://codereview.chromium.org/21544005/diff/15001/content/browser/web_conte... > content/browser/web_contents/navigation_controller_impl_unittest.cc:1943: // > EXPECT_TRUE(details.is_in_page); > Can we fix this now? We could by removing new cases I added and pretending the renderer says it's in page. > > https://codereview.chromium.org/21544005/diff/15001/content/browser/web_conte... > content/browser/web_contents/navigation_controller_impl_unittest.cc:2784: > other_url_with_ref)); > Maybe we can add a check and comment below saying that going back from a ref to > a non-ref will be considered in-page if the renderer says it is. > > EXPECT_TRUE(controller.IsURLInPageNavigation(url, true, > NAVIGATION_TYPE_IN_PAGE)); Sounds good to me.
Great, thanks for the changes. A few more comments below. https://codereview.chromium.org/21544005/diff/24001/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl.cc (right): https://codereview.chromium.org/21544005/diff/24001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl.cc:114: return navigation_type == content::NAVIGATION_TYPE_IN_PAGE; Please include a comment about how this is useful for the back from ref to non-ref case. https://codereview.chromium.org/21544005/diff/24001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl.cc:1197: content::NavigationType navigation_type) const { Style nit: Each argument should be on its own line. (This rule applies to function declarations but not call sites.) https://codereview.chromium.org/21544005/diff/24001/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl.h (right): https://codereview.chromium.org/21544005/diff/24001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl.h:158: content::NavigationType navigation_type) const { We can leave this as a one-argument function that takes in url and passes UNKNOWN as the type. https://codereview.chromium.org/21544005/diff/24001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl.h:168: content::NavigationType navigation_type) const; Style nit: if all arguments can't fit on the same line, they should all be on their own line. See RendererDidNavigate for an example. https://codereview.chromium.org/21544005/diff/24001/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/21544005/diff/24001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl_unittest.cc:2744: content::NAVIGATION_TYPE_SAME_PAGE)); Don't need to pass the type in any of these but the last one (line 2768). https://codereview.chromium.org/21544005/diff/24001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl_unittest.cc:2767: // if the renderer says it is even the navigation type isn't IN_PAGE. nit: even if https://codereview.chromium.org/21544005/diff/24001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl_unittest.cc:2769: content::NAVIGATION_TYPE_UNKNOWN)); This case is good to test, but can you also add the one I mentioned, along with a comment about it? I want to be sure we test the back from url_with_ref to url case. EXPECT_TRUE(controller.IsURLInPageNavigation(url, true, NAVIGATION_TYPE_IN_PAGE));
https://codereview.chromium.org/21544005/diff/24001/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/21544005/diff/24001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl_unittest.cc:2769: content::NAVIGATION_TYPE_UNKNOWN)); On 2013/08/06 18:48:08, creis wrote: > This case is good to test, but can you also add the one I mentioned, along with > a comment about it? I want to be sure we test the back from url_with_ref to url > case. > > EXPECT_TRUE(controller.IsURLInPageNavigation(url, true, > NAVIGATION_TYPE_IN_PAGE)); Ok. But I'll modify it a bit since it's the navigation type which matters here not what renderer says.
Sorry, it looks like you didn't finish making the change to the test. https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl.cc (right): https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl.cc:109: content::NavigationType navigation_type) { This whole file is in the content namespace, so you don't need content:: here or below. https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl.cc:115: // is typically IN_PAGE which causes the correct value to be returned nit: drop "typically" (unless you can mention examples where it's not true) and end the sentence after IN_PAGE. https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl.cc:958: content::NAVIGATION_TYPE_UNKNOWN)) { No need for content:: https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl.cc:1201: content::NavigationType navigation_type) const { No need for content:: https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl_unittest.cc:2762: content::NAVIGATION_TYPE_UNKNOWN)); I don't understand this line. It's identical to the check below. As I suggested before, the first argument should be url and not url_with_ref, and the last argument should be IN_PAGE, not UNKNOWN. The second argument won't affect the outcome of the test, but it should be true because that's what it will be in practice. Also, I don't think you need content:: here or below, because you're already in the content namespace.
https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl_unittest.cc:2762: content::NAVIGATION_TYPE_UNKNOWN)); On 2013/08/07 16:59:27, creis wrote: > I don't understand this line. It's identical to the check below. As I > suggested before, the first argument should be url and not url_with_ref, and the > last argument should be IN_PAGE, not UNKNOWN. The second argument won't affect > the outcome of the test, but it should be true because that's what it will be in > practice. > > Also, I don't think you need content:: here or below, because you're already in > the content namespace. Looks like copy&paste without modification. I meant to test going to the same url when renderer says it's in page and second one when going to non ref url with IN_PAGE navigation type. But I'll change that to the one you've been suggesting from the very beginning.
https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl.cc (right): https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl.cc:109: content::NavigationType navigation_type) { On 2013/08/07 16:59:27, creis wrote: > This whole file is in the content namespace, so you don't need content:: here or > below. Done. https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl.cc:115: // is typically IN_PAGE which causes the correct value to be returned On 2013/08/07 16:59:27, creis wrote: > nit: drop "typically" (unless you can mention examples where it's not true) and > end the sentence after IN_PAGE. Done. https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl.cc:958: content::NAVIGATION_TYPE_UNKNOWN)) { On 2013/08/07 16:59:27, creis wrote: > No need for content:: Done. https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl.cc:1201: content::NavigationType navigation_type) const { On 2013/08/07 16:59:27, creis wrote: > No need for content:: Done. https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl_unittest.cc:2762: content::NAVIGATION_TYPE_UNKNOWN)); On 2013/08/07 16:59:27, creis wrote: > I don't understand this line. It's identical to the check below. As I > suggested before, the first argument should be url and not url_with_ref, and the > last argument should be IN_PAGE, not UNKNOWN. The second argument won't affect > the outcome of the test, but it should be true because that's what it will be in > practice. > > Also, I don't think you need content:: here or below, because you're already in > the content namespace. Done.
Thanks for the changes. LGTM with style nit, and please add your earlier IsURLInPageNavigation(url_with_ref, true, NAVIGATION_TYPE_UNKNOWN) test case as well. https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl_unittest.cc:2767: content::NAVIGATION_TYPE_UNKNOWN)); This case was a great addition, independent of the one I was suggesting. Please include it as well. https://codereview.chromium.org/21544005/diff/37001/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl.cc (right): https://codereview.chromium.org/21544005/diff/37001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl.cc:744: details->is_in_page = IsURLInPageNavigation(params.url, Style nit: This is fine, but might as well move params.url to the next line (with the other two arguments) to make it easier to read.
https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/21544005/diff/31001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl_unittest.cc:2767: content::NAVIGATION_TYPE_UNKNOWN)); On 2013/08/07 17:49:56, creis wrote: > This case was a great addition, independent of the one I was suggesting. Please > include it as well. Done. https://codereview.chromium.org/21544005/diff/37001/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl.cc (right): https://codereview.chromium.org/21544005/diff/37001/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl.cc:744: details->is_in_page = IsURLInPageNavigation(params.url, On 2013/08/07 17:49:56, creis wrote: > Style nit: This is fine, but might as well move params.url to the next line > (with the other two arguments) to make it easier to read. Done.
Thanks. Still LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pstanek@opera.com/21544005/43001
Message was sent while issue was closed.
Change committed as 216329
Message was sent while issue was closed.
This change is problematic and causes a regression in Chrome. https://code.google.com/p/chromium/issues/detail?id=281184 See FindBarController::Observe for how the Find box is closed upon navigation to a different page. That no longer happens after this change. I reverted this change locally and that makes things work as before.
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/21544005/diff/43001/content/browser/we... File content/browser/web_contents/navigation_controller_impl.cc (left): https://chromiumcodereview.appspot.com/21544005/diff/43001/content/browser/we... content/browser/web_contents/navigation_controller_impl.cc:118: // scenarios can be differentiated with the TransitionType. Are we sure that nothing meaningful is lost by deleting this comment? https://chromiumcodereview.appspot.com/21544005/diff/43001/content/browser/we... content/browser/web_contents/navigation_controller_impl.cc:747: // Do navigation-type specific actions. These will make and commit an entry. See how the comments on line 747 and 743 were meant to convey that the order of those two statements should not be swapped? :)
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/21544005/diff/43001/content/browser/we... File content/browser/web_contents/navigation_controller_impl.cc (left): https://chromiumcodereview.appspot.com/21544005/diff/43001/content/browser/we... content/browser/web_contents/navigation_controller_impl.cc:747: // Do navigation-type specific actions. These will make and commit an entry. On 2013/08/30 11:52:00, Finnur wrote: > See how the comments on line 747 and 743 were meant to convey that the order of > those two statements should not be swapped? :) I don't think ClassifyNavigation() commits the entry and this is the comment is about.
Message was sent while issue was closed.
If that's the case then the two comments need to be revised/removed.
Message was sent while issue was closed.
On 2013/08/30 12:29:39, Finnur wrote: > If that's the case then the two comments need to be revised/removed. I guess they should simply be moved just before the switch statement doing navigation type dependent stuff.
Message was sent while issue was closed.
I don't know this code well, but isn't it just enough to remove the words 'These will make and commit an entry.' ?
Message was sent while issue was closed.
On 2013/08/30 12:42:39, Finnur wrote: > I don't know this code well, but isn't it just enough to remove the words 'These > will make and commit an entry.' ? But RendererDidNavigateTo*() called in cases of the switch will commit the entry.
Message was sent while issue was closed.
Ah, I see what you mean now! I thought the comment above ClassifyNavigation applied to the ClassifyNavigation function, and not to the switch statement. So technically, the comment is still correct, it is just in a confusing location. So yes, I agree that the comment should probably live above the switch statement. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
