Fix wrong ui::PAGE_TRANSITION_FORWARD_BACK in tests.

Fix wrong ui::PAGE_TRANSITION_FORWARD_BACK in tests.

This CL modify some unit tests such that the PageTransition qualifier:
ui::PAGE_TRANSITION_FORWARD_BACK matches when the browser askes for a
new navigation and the renderer navigates.

Context: https://crrev.com/2958573002/
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=736772
Review-Url: https://codereview.chromium.org/2957783002
Cr-Commit-Position: refs/heads/master@{#482581}
Committed: https://chromium.googlesource.com/chromium/src/+/2504f838819d321e4bf62e3da29ec258260356e3

[arthursonzogni, 2017-06-26 13:56:59]

Description was changed from

==========
Fix wrong ui::PAGE_TRANSITION_FORWARD_BACK in tests.

This CL modify some unit tests such that the PageTransition qualifier:
ui::PAGE_TRANSITION_FORWARD_BACK matches when the browser askes for a
new navigation and the renderer navigates.

Context: https://crrev.com/2958573002/
==========

to

==========
Fix wrong ui::PAGE_TRANSITION_FORWARD_BACK in tests.

This CL modify some unit tests such that the PageTransition qualifier:
ui::PAGE_TRANSITION_FORWARD_BACK matches when the browser askes for a
new navigation and the renderer navigates.

Context: https://crrev.com/2958573002/
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[arthursonzogni, 2017-06-26 14:01:51]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 14:01:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 14:29:09]

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

[commit-bot: I haz the power, 2017-06-26 14:29:10]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-06-26 14:59:10]

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

[arthursonzogni, 2017-06-26 14:59:11]

Hi Charlie,

Could you please review this CL?

I am not sure the part with SendNavigate => SendNavigateWithTransition worth
being committed. We probably don't care having the right PageTransition. Let me
know what do think.

https://codereview.chromium.org/2957783002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (left):

https://codereview.chromium.org/2957783002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3009:
our_controller.GoToIndex(0);
This line does a back/forward navigation. But we are trying here to restore a
NavigationEntry.

[Avi (use Gerrit), 2017-06-26 15:03:10]

On 2017/06/26 14:59:11, arthursonzogni wrote:
> Hi Charlie,
> 
> Could you please review this CL?
> 
> I am not sure the part with SendNavigate => SendNavigateWithTransition worth
> being committed. We probably don't care having the right PageTransition. Let
me
> know what do think.
> 
>
https://codereview.chromium.org/2957783002/diff/1/content/browser/frame_host/...
> File content/browser/frame_host/navigation_controller_impl_unittest.cc (left):
> 
>
https://codereview.chromium.org/2957783002/diff/1/content/browser/frame_host/...
> content/browser/frame_host/navigation_controller_impl_unittest.cc:3009:
> our_controller.GoToIndex(0);
> This line does a back/forward navigation. But we are trying here to restore a
> NavigationEntry.

This is a bit of code that I've worked on, so I'm going to add myself as a
reviewer. Hope that's OK.

I haven't yet looked at the changes, but you definitely need to file a bug. You
have a line "Context: https://crrev.com/2958573002/", but that CL exposes what's
clearly an issue, the PageTransition getting BackForward dropped. So file a bug
and tag everything related to that bug with that bug number.

[Avi (use Gerrit), 2017-06-26 15:06:52]

The adding of transition types to the navigations lgtm. I don't know much about
the restoration stuff. Charlie, hope you have thoughts about that.

This looks like good stuff; PageTransitions need some attention.

[arthursonzogni, 2017-06-26 15:20:35]

On 2017/06/26 15:03:10, Avi (ping after 24h) wrote:
> This is a bit of code that I've worked on, so I'm going to add myself as a
> reviewer. Hope that's OK.
Okay thanks!

On 2017/06/26 15:03:10, Avi (ping after 24h) wrote:
> I haven't yet looked at the changes, but you definitely need to file a bug.
You
> have a line "Context: https://crrev.com/2958573002/%22, but that CL exposes
what's
> clearly an issue, the PageTransition getting BackForward dropped. So file a
bug
> and tag everything related to that bug with that bug number.

All the modified tests here are unit tests. The renderer part is simulated and
the problem is that the default value for ui::PageTransition is used
(ui::PAGE_TRANSITION_LINK instead of the correct one for instance).

There is a real problem in a non-simulated part below, but to catch it, I need
to look at the failing browsertests instead.

But whatever, I will create a new bug, even if this CL does not fixes the real
issue.

[arthursonzogni, 2017-06-26 15:34:35]

Description was changed from

==========
Fix wrong ui::PAGE_TRANSITION_FORWARD_BACK in tests.

This CL modify some unit tests such that the PageTransition qualifier:
ui::PAGE_TRANSITION_FORWARD_BACK matches when the browser askes for a
new navigation and the renderer navigates.

Context: https://crrev.com/2958573002/
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix wrong ui::PAGE_TRANSITION_FORWARD_BACK in tests.

This CL modify some unit tests such that the PageTransition qualifier:
ui::PAGE_TRANSITION_FORWARD_BACK matches when the browser askes for a
new navigation and the renderer navigates.

Context: https://crrev.com/2958573002/
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=736772
==========

[arthursonzogni, 2017-06-26 15:35:17]

I created the new issue: http://crbug.com/736772

[Avi (use Gerrit), 2017-06-26 15:43:34]

On 2017/06/26 15:35:17, arthursonzogni wrote:
> I created the new issue: http://crbug.com/736772

Awesome, thank you.

If this CL doesn't fix the tests, then we need a bug even more to coordinate the
fixes across the CLs that actually fix it, and the ones that set up the fix but
aren't it.

In any case, thanks again for looking into this.

[arthursonzogni, 2017-06-26 15:56:11]

On 2017/06/26 15:43:34, Avi (ping after 24h) wrote:
> On 2017/06/26 15:35:17, arthursonzogni wrote:
> > I created the new issue: http://crbug.com/736772
> 
> Awesome, thank you.
> 
> If this CL doesn't fix the tests, then we need a bug even more to coordinate
the
> fixes across the CLs that actually fix it, and the ones that set up the fix
but
> aren't it.
> 
> In any case, thanks again for looking into this.

This CL fixes most of the content_unittests (i.e. does not fail with :
https://crrev.com/2958573002/ anymore)
* RenderFrameHostManagerTest.NavigateAfterMissingSwapOutACK
* NavigationControllerTest.GoBackWithUserAgentOverrideChange
* NavigationControllerTest.RemoveEntry
* OverscrollNavigationOverlayTest.Navigation_PaintUpdate
* NavigationControllerTest.GoToOffset
* NavigationControllerTest.Forward
* NavigationControllerTest.RestoreNavigateAfterFailure
* NavigationControllerTest.RestoreNavigate
* NavigationControllerTest.TransientEntry
* RenderFrameHostManagerTest.DisownOpenerAfterNavigation
* NavigationControllerTest.Forward_GeneratesNewPage
* NavigationControllerTest.Back_GeneratesNewPage
* WebContentsImplTest.NavigationEntryContentState
* OverscrollNavigationOverlayTest.CancelAfterSuccessfulNavigation
* WebContentsImplTest.NavigateFromRestoredRegularUrl
* WebContentsImplTest.NavigateFromRestoredSitelessUrl
* NavigationControllerTest.LoadURL_WithBindings
* NavigationControllerTest.RemoveEntryWithPending
* RenderFrameHostManagerTest.DisownOpenerDuringNavigation

The remaining content_unittests are:
* NavigationControllerTest.BackSubframe
  => TestRenderFrameHost::SimulateNavigationStart([..]) always uses
ui::PAGE_TRANSITION_LINK.

* RenderProcessHostUnitTest.DoNotReuseError
  => I still haven't looked at it.

[Charlie Reis, 2017-06-27 03:59:14]

Thanks-- I agree with making the unit tests more accurately reflect what happens
in practice (by adding the correct transitions).  LGTM.

https://codereview.chromium.org/2957783002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (left):

https://codereview.chromium.org/2957783002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3009:
our_controller.GoToIndex(0);
On 2017/06/26 14:59:11, arthursonzogni wrote:
> This line does a back/forward navigation. But we are trying here to restore a
> NavigationEntry.

Thanks-- your change here looks good.

[arthursonzogni, 2017-06-27 09:14:39]

The CQ bit was checked by arthursonzogni@chromium.org

[commit-bot: I haz the power, 2017-06-27 09:14:51]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2017-06-27 09:14:54]

Thanks for the reviews!

[commit-bot: I haz the power, 2017-06-27 09:19:00]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1498554879596480, "parent_rev":
"717b58ffd06c514834037b488fe21280e55bda10", "commit_rev":
"2504f838819d321e4bf62e3da29ec258260356e3"}

[commit-bot: I haz the power, 2017-06-27 09:19:12]

Description was changed from

==========
Fix wrong ui::PAGE_TRANSITION_FORWARD_BACK in tests.

This CL modify some unit tests such that the PageTransition qualifier:
ui::PAGE_TRANSITION_FORWARD_BACK matches when the browser askes for a
new navigation and the renderer navigates.

Context: https://crrev.com/2958573002/
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=736772
==========

to

==========
Fix wrong ui::PAGE_TRANSITION_FORWARD_BACK in tests.

This CL modify some unit tests such that the PageTransition qualifier:
ui::PAGE_TRANSITION_FORWARD_BACK matches when the browser askes for a
new navigation and the renderer navigates.

Context: https://crrev.com/2958573002/
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=736772

Review-Url: https://codereview.chromium.org/2957783002
Cr-Commit-Position: refs/heads/master@{#482581}
Committed:
https://chromium.googlesource.com/chromium/src/+/2504f838819d321e4bf62e3da29e...
==========

[commit-bot: I haz the power, 2017-06-27 09:19:13]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/2504f838819d321e4bf62e3da29e...