PlzNavigate: disable RenderFrameHostManagerTest::PageDoesBackAndReload.

PlzNavigate: disable RenderFrameHostManagerTest::PageDoesBackAndReload.

PlzNavigate has a very different way of carrying out renderer initiated
navigations and of dealing with navigation cancellation. So this change disables
RenderFrameHostManagerTest::PageDoesBackAndReload because adapting it to work
with the two implementations would make it full of special cases. There are
other tests (listed in a comment in the change itself) that exercise the
equivalent logic in PlzNavigate.

BUG=439423
Committed: https://crrev.com/a224ea2413228bd7a781309959bfdccb26282573
Cr-Commit-Position: refs/heads/master@{#341581}

[carlosk, 2015-08-03 11:03:40]

carlosk@chromium.org changed reviewers:
+ clamy@chromium.org, creis@chromium.org, fdegans@chromium.org,
nasko@chromium.org

[carlosk, 2015-08-03 11:03:41]

creis@: PTAL

clamy@, fdegans@, nasko@: FYI

[Charlie Reis, 2015-08-03 17:59:42]

LGTM with nits.  Glad we have that covered in other tests.

https://codereview.chromium.org/1270843002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1270843002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1221: //
PlzNavigate follows a significantly different logic for renderer
s/logic/path/

("follows a...path" or "uses...logic")

https://codereview.chromium.org/1270843002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1231: SUCCEED()
<< "Test is not applicable with browser side navigation enabled";
We've just been returning early when tests don't apply (e.g., see
DropCreateChildFrameWhileSwappedOut above).  What does the SUCCEED macro do, and
why is it necessary?

[carlosk, 2015-08-03 18:45:00]

Thanks.

https://codereview.chromium.org/1270843002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1270843002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1221: //
PlzNavigate follows a significantly different logic for renderer
On 2015/08/03 17:59:42, Charlie Reis wrote:
> s/logic/path/
> 
> ("follows a...path" or "uses...logic")

Done using "uses...logic".

https://codereview.chromium.org/1270843002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1231: SUCCEED()
<< "Test is not applicable with browser side navigation enabled";
On 2015/08/03 17:59:42, Charlie Reis wrote:
> We've just been returning early when tests don't apply (e.g., see
> DropCreateChildFrameWhileSwappedOut above).  What does the SUCCEED macro do,
and
> why is it necessary?

It is not necessary and is currently purely for documenting the code. But it is
recommended by the gtest documentation as they might output the message at some
point:

https://code.google.com/p/googletest/wiki/AdvancedGuide#Explicit_Success_and_...

I also used that call in http://crbug.com/1234563002/

[carlosk, 2015-08-03 18:45:08]

The CQ bit was checked by carlosk@chromium.org

[carlosk, 2015-08-03 18:45:08]

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/1270843002/#ps20001
(title: "Comment change.")

[commit-bot: I haz the power, 2015-08-03 18:47:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1270843002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1270843002/20001

[commit-bot: I haz the power, 2015-08-03 19:26:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-08-03 19:26:32]

Patchset 2 (id:??) landed as
https://crrev.com/a224ea2413228bd7a781309959bfdccb26282573
Cr-Commit-Position: refs/heads/master@{#341581}

[carlosk, 2015-08-04 15:52:23]

While working on crrev.com/1259623003 I revisited this test and I think there
are in fact a couple of issues with PlzNavigate that this might be touching. One
is that those equivalent tests I mentioned are not simulating receiving renderer
commit messages from previously cancelled navigations.

The second is in regards on how PlzNavigate handles the "history back"
navigation from this test. If I understood correctly it is supposedly triggered
by the renderer without any user input, is that right? If that's the case what
is wrong is that it is not being considered renderer-initiated,
non-user-initiated. Instead it is being handled through the same code path as
browser-initiated navigations are, giving it a higher priority in regards to
cancelling other navigations or being cancelled itself.

I'll do some more investigation on this and see how to handle these issues.

[carlosk, 2015-11-02 10:41:11]

Description was changed from

==========
PlzNavigate: disable RenderFrameHostManagerTest::PageDoesBackAndReload.

PlzNavigate has a very different way of carrying out renderer initiated
navigations and of dealing with navigation cancellation. So this change disables
RenderFrameHostManagerTest::PageDoesBackAndReload because adapting it to work
with the two implementations would make it full of special cases. There are
other tests (listed in a comment in the change itself) that exercise the
equivalent logic in PlzNavigate.

BUG=439423

Committed: https://crrev.com/a224ea2413228bd7a781309959bfdccb26282573
Cr-Commit-Position: refs/heads/master@{#341581}
==========

to

==========
PlzNavigate: disable RenderFrameHostManagerTest::PageDoesBackAndReload.

PlzNavigate has a very different way of carrying out renderer initiated
navigations and of dealing with navigation cancellation. So this change disables
RenderFrameHostManagerTest::PageDoesBackAndReload because adapting it to work
with the two implementations would make it full of special cases. There are
other tests (listed in a comment in the change itself) that exercise the
equivalent logic in PlzNavigate.

BUG=439423

Committed: https://crrev.com/a224ea2413228bd7a781309959bfdccb26282573
Cr-Commit-Position: refs/heads/master@{#341581}
==========

[carlosk, 2015-11-02 10:41:11]

carlosk@chromium.org changed reviewers:
- clamy@chromium.org, creis@chromium.org, fdegans@chromium.org,
nasko@chromium.org