Trust the renderer's same-document navigation flag if it is a same-origin nav.

Trust the renderer's same-document navigation flag if it is a same-origin nav.

Currently in AreURLsInPageNavigation, we only trust renderer_says_in_page if
the before and after urls are identical. This prevents us from correctly
classifying history.pushState and history.replaceState navigations as in-page.
Navigations via the history API are required to be same-origin, but can differ
by more than just the ref component, so we get the correct behavior without
the renderer process being able to lie about a cross-origin navigation.

BUG=138324
TEST=Added cases to NavigationControllerTest.IsInPageNavigation

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274734

[Tom Sepez, 2014-05-29 18:39:23]

lgtm

[nasko, 2014-05-29 22:59:30]

Overall looks fine, but the test harness change isn't quite clear to me.

https://codereview.chromium.org/304763002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller_unittest.cc (right):

https://codereview.chromium.org/304763002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller_unittest.cc:222: Reload();
nit: Either the action or the comment is incorrect. They should be in sync.

https://codereview.chromium.org/304763002/diff/60001/content/test/test_render...
File content/test/test_render_frame_host.cc (right):

https://codereview.chromium.org/304763002/diff/60001/content/test/test_render...
content/test/test_render_frame_host.cc:124: params.was_within_same_page =
transition != PAGE_TRANSITION_RELOAD &&
Why have the transition type being checked? Wouldn't PAGE_TRANSITION_LINK to a
#ref on the page still be considered "within_same_page"?

[Nate Chapin, 2014-05-29 23:03:45]

https://codereview.chromium.org/304763002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller_unittest.cc (right):

https://codereview.chromium.org/304763002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller_unittest.cc:222: Reload();
On 2014/05/29 22:59:30, nasko wrote:
> nit: Either the action or the comment is incorrect. They should be in sync.

Depends on your definition of "away" :)

Will clarify.

https://codereview.chromium.org/304763002/diff/60001/content/test/test_render...
File content/test/test_render_frame_host.cc (right):

https://codereview.chromium.org/304763002/diff/60001/content/test/test_render...
content/test/test_render_frame_host.cc:124: params.was_within_same_page =
transition != PAGE_TRANSITION_RELOAD &&
On 2014/05/29 22:59:30, nasko wrote:
> Why have the transition type being checked? Wouldn't PAGE_TRANSITION_LINK to a
> #ref on the page still be considered "within_same_page"?

A link transition can be based on this code.

On the other hand, reloads are never within_same_page, nor are typed urls (i.e.,
the user focusing the address bar and pressing enter).

[nasko, 2014-05-29 23:12:59]

lgtm

[Nate Chapin, 2014-05-29 23:47:05]

+jochen, would you mind giving this an OWNERS look?

[jam, 2014-05-30 17:20:28]

lgtm

[Nate Chapin, 2014-05-30 17:22:46]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-05-30 17:25:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/japhet@chromium.org/304763002/80001

[commit-bot: I haz the power, 2014-05-31 04:39:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-31 05:54:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-31 05:54:41]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Nate Chapin, 2014-05-31 22:30:34]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-05-31 22:31:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/japhet@chromium.org/304763002/80001

[commit-bot: I haz the power, 2014-06-01 00:11:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-06-01 01:43:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-01 01:43:22]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Nate Chapin, 2014-06-01 19:02:56]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-06-01 19:03:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/japhet@chromium.org/304763002/80001

[commit-bot: I haz the power, 2014-06-01 20:40:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-06-01 22:20:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-01 22:20:46]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Nate Chapin, 2014-06-02 21:21:48]

mkosiba@, I'd appreciate your opinion on the change to
content/browser/android/web_contents_observer_android.cc

[mkosiba (inactive), 2014-06-03 11:31:08]

On 2014/06/02 21:21:48, Nate Chapin wrote:
> mkosiba@, I'd appreciate your opinion on the change to
> content/browser/android/web_contents_observer_android.cc

LGTM

https://codereview.chromium.org/304763002/diff/110001/content/browser/android...
File content/browser/android/web_contents_observer_android.cc (right):

https://codereview.chromium.org/304763002/diff/110001/content/browser/android...
content/browser/android/web_contents_observer_android.cc:140: bool
in_page_navigation = urls_same_ignoring_fragment &&
thanks! I guess we could just call this is_fragment_navigation.

Could you also update the Java files as well?
should be a matter of %s/isNavigationInPage/isFragmentNavigation/

android_webview/java/src/org/chromium/android_webview/AwContentsClient.java line
91
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
line 620
content/public/android/java/src/org/chromium/content/browser/WebContentsObserverAndroid.java
line 62

[Nate Chapin, 2014-06-03 16:48:51]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-06-03 16:49:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/japhet@chromium.org/304763002/130001

[commit-bot: I haz the power, 2014-06-03 20:20:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Nate Chapin, 2014-06-03 20:48:10]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-06-03 20:49:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/japhet@chromium.org/304763002/130001

[commit-bot: I haz the power, 2014-06-04 09:00:48]

Change committed as 274734