Add tests for navigation type classifications.

Add tests for navigation type classifications.

BUG=369661
TEST=no code changes; this adds new tests

Committed: https://crrev.com/1d07590021ab4b5781cefa1595d297ccf475d7fe
Cr-Commit-Position: refs/heads/master@{#319894}

[Avi (use Gerrit), 2015-03-06 23:49:12]

avi@chromium.org changed reviewers:
+ creis@chromium.org

[Avi (use Gerrit), 2015-03-06 23:49:13]

Everything but the client-side redirects. That's the next CL.

[Charlie Reis, 2015-03-09 20:39:58]

A few thoughts on the things we've been confused about so far.

https://codereview.chromium.org/987923002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/987923002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:324: //
TODO(avi,creis): Why is this not NAVIGATION_TYPE_IN_PAGE? It makes sense
Yes, I think we'll probably accept the current behavior (since both in-page and
cross-page create new NavEntries in the same way and only the back/forward cases
differ) and should do a better job of documenting it.

https://codereview.chromium.org/987923002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:347:
EXPECT_EQ(ui::PAGE_TRANSITION_LINK | ui::PAGE_TRANSITION_CLIENT_REDIRECT,
This gets treated as a client redirect?  I'm somewhat surprised.  I thought it
stopped being a client redirect once the onload event of the previous navigation
runs.

(e.g., Client redirect: commit page 1, then navigate to page 2 before onload.
Not client redirect: commit page 1, run onload event, then at some later time
navigate to page 2.)

Guess I was wrong?  Is any non-user-initiated navigation treated as a client
redirect?  Maybe we should mention that in a comment.

https://codereview.chromium.org/987923002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:405: //
Reload.
Can we also test a browser-initiated reload?  I would expect that to not have a
client redirect transition type.

https://codereview.chromium.org/987923002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:410:
EXPECT_EQ(ui::PAGE_TRANSITION_LINK | ui::PAGE_TRANSITION_CLIENT_REDIRECT,
Wow, this is a client redirect too?   I guess that's consistent with it being a
non-user-initiated navigation.

https://codereview.chromium.org/987923002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:473: //
TODO(avi,creis): Huh? Why is this not NAVIGATION_TYPE_EXISTING_PAGE like
Ok, let's think this through.

history.replaceState doesn't change the current document.  It just updates the
current NavigationEntry's URL, etc.  In that sense, it's an in-page navigation
because we don't leave the page.  It's also kind of a history navigation, since
it updates an existing NavigationEntry rather than creating a new one.

location.replace does change the current document, but it replaces the current
NavigationEntry.  In that sense, it's not an in-page navigation.

Maybe the current behavior makes sense?

[Avi (use Gerrit), 2015-03-09 21:35:56]

https://codereview.chromium.org/987923002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/987923002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:324: //
TODO(avi,creis): Why is this not NAVIGATION_TYPE_IN_PAGE? It makes sense
On 2015/03/09 20:39:58, Charlie Reis wrote:
> Yes, I think we'll probably accept the current behavior (since both in-page
and
> cross-page create new NavEntries in the same way and only the back/forward
cases
> differ) and should do a better job of documenting it.

Acknowledged.

https://codereview.chromium.org/987923002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:347:
EXPECT_EQ(ui::PAGE_TRANSITION_LINK | ui::PAGE_TRANSITION_CLIENT_REDIRECT,
On 2015/03/09 20:39:58, Charlie Reis wrote:
> Is any non-user-initiated navigation treated as a client redirect?

I don't know what their intention is. It might be intentional, it might be an
accident of how they implement the handling of real client-side redirects.

We need to very much clarify what the transitions are and when they are
expected. There should be some unit tests here. (See my transition type question
on line 311, etc.) In any case, let's document what is.

https://codereview.chromium.org/987923002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:405: //
Reload.
On 2015/03/09 20:39:58, Charlie Reis wrote:
> Can we also test a browser-initiated reload?  I would expect that to not have
a
> client redirect transition type.

Done.

Also, haha it's PAGE_TRANSITION_RELOAD, while renderer-side is
PAGE_TRANSITION_LINK | PAGE_TRANSITION_CLIENT_REDIRECT. My position is that the
page transition assignments are only vaguely intentional.

https://codereview.chromium.org/987923002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:410:
EXPECT_EQ(ui::PAGE_TRANSITION_LINK | ui::PAGE_TRANSITION_CLIENT_REDIRECT,
On 2015/03/09 20:39:58, Charlie Reis wrote:
> Wow, this is a client redirect too?   I guess that's consistent with it being
a
> non-user-initiated navigation.

Again, it's not clear how intentional this is.

https://codereview.chromium.org/987923002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:473: //
TODO(avi,creis): Huh? Why is this not NAVIGATION_TYPE_EXISTING_PAGE like
On 2015/03/09 20:39:58, Charlie Reis wrote:
> Ok, let's think this through.
> 
> history.replaceState doesn't change the current document.  It just updates the
> current NavigationEntry's URL, etc.  In that sense, it's an in-page navigation
> because we don't leave the page.  It's also kind of a history navigation,
since
> it updates an existing NavigationEntry rather than creating a new one.
> 
> location.replace does change the current document, but it replaces the current
> NavigationEntry.  In that sense, it's not an in-page navigation.
> 
> Maybe the current behavior makes sense?

Acknowledged.

[Charlie Reis, 2015-03-09 22:04:05]

New tests (and remaining questions to answer) LGTM.  Do you want to make some
updates to navigation_type.h in this CL with some of the things we've learned
(e.g., NEW_PAGE includes in-page, IN_PAGE is for back/forward only), or should
we do that in a separate CL?

https://codereview.chromium.org/987923002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/987923002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:347:
EXPECT_EQ(ui::PAGE_TRANSITION_LINK | ui::PAGE_TRANSITION_CLIENT_REDIRECT,
On 2015/03/09 21:35:56, Avi wrote:
> On 2015/03/09 20:39:58, Charlie Reis wrote:
> > Is any non-user-initiated navigation treated as a client redirect?
> 
> I don't know what their intention is. It might be intentional, it might be an
> accident of how they implement the handling of real client-side redirects.
> 
> We need to very much clarify what the transitions are and when they are
> expected. There should be some unit tests here. (See my transition type
question
> on line 311, etc.) In any case, let's document what is.

Agreed.  (I also don't know the answer to line 311.  It seems like LINK is the
default and may be used in more places than it should.)

https://codereview.chromium.org/987923002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/987923002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:480: //
TODO(avi,creis): Huh? Why is this not NAVIGATION_TYPE_EXISTING_PAGE like
Remove this TODO?

[Avi (use Gerrit), 2015-03-09 22:50:19]

PTAL

https://codereview.chromium.org/987923002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/987923002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:480: //
TODO(avi,creis): Huh? Why is this not NAVIGATION_TYPE_EXISTING_PAGE like
On 2015/03/09 22:04:05, Charlie Reis wrote:
> Remove this TODO?

Done.

[Charlie Reis, 2015-03-09 23:28:26]

Thanks, LGTM.

https://codereview.chromium.org/987923002/diff/60001/content/public/browser/n...
File content/public/browser/navigation_type.h (left):

https://codereview.chromium.org/987923002/diff/60001/content/public/browser/n...
content/public/browser/navigation_type.h:31: // subframe navigations). It may be
for any page, not necessarily the last
Man, that last sentence was confusing.  It sounds like it was saying it could be
for a different document (which is impossible, right?).  I think it meant "It
may be for any entry."  Anyway, glad that part is gone.

https://codereview.chromium.org/987923002/diff/60001/content/public/browser/n...
File content/public/browser/navigation_type.h (right):

https://codereview.chromium.org/987923002/diff/60001/content/public/browser/n...
content/public/browser/navigation_type.h:35: // case for history.replaceState(),
and back and forward across fragment
nit: s/and/as well as/
(just to better divide the replaceState case from the rest)

[Avi (use Gerrit), 2015-03-10 14:24:52]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2015-03-10 14:24:53]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/987923002/#ps80001 (title:
"small creis tweak")

[commit-bot: I haz the power, 2015-03-10 14:25:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/987923002/80001

[commit-bot: I haz the power, 2015-03-10 16:06:51]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-10 16:07:38]

Patchset 5 (id:??) landed as
https://crrev.com/1d07590021ab4b5781cefa1595d297ccf475d7fe
Cr-Commit-Position: refs/heads/master@{#319894}