PlzNavigate: Refactor unit test helper functions

PlzNavigate: Refactor unit test helper functions

This CL extracts unit test helper classes from navigator_impl_unittests so that
they can be used in other places. It also makes
TestWebContents::NavigateAndCommit work when browser side navigation is
enabled.

PlzNavigate: improve unit test helper functions

BUG=434383
Committed: https://crrev.com/72c7279ae8a307aba091bafc188cebd97a33b899
Cr-Commit-Position: refs/heads/master@{#304640}

[clamy, 2014-11-14 15:39:26]

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

[clamy, 2014-11-14 15:45:26]

@carlosk, davidben, nasko: PTAL. This is a refactoring of our unit tests so that
TestWebContents::NavigateAndCommit function works with PlzNavigate and some
PlzNavigate specific unit tests can be added to unit tests files other than
navigator_impl_unittests.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/browser/frame...
File content/browser/frame_host/navigator_impl_unittest.cc (left):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl_unittest.cc:425: TEST_F(NavigatorTest,
BrowserSideNavigationHistogramTest) {
I removed this test because it did not fit in the new model of
NavigatorTestWithBrowserSideNavigation, where PlzNavigate is enabled during the
whole test. On top of that, we care mainly about the specialized histograms,
which are not being tested here.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
File content/test/browser_side_navigation_test_utils.h (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
content/test/browser_side_navigation_test_utils.h:31: //
TestWebContents::NavigateAndCommit).
I am not sure if a global LazyInstance is destroyed at the end of each unit
test, hence the Init and TearDown functions (we don't want this class to exist
for the NavigationURLLoaderImpl unit tests). If it is, then I'll get rid of them
and there will just be a static Get method.

[nasko, 2014-11-15 00:32:51]

Thanks for refactoring this! A few comments and questions. Also, please put a
bug number in the CL description.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/browser/frame...
File content/browser/frame_host/navigator_impl.h (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.h:41: FrameTreeNode* frame_tree_node);
nit: Move this after the Navigator interface overrides.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/browser/frame...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl_unittest.cc:32: class
NavigatorTestWithBrowserSideNavigation
Why not just "BrowserSideNavigationTest"?

https://chromiumcodereview.appspot.com/730553002/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl_unittest.cc:150: response,
BrowserSideNavigationTestUtils::Get()->MakeEmptyStream());
I wonder if we need the whole singleton part, as it makes it less readable.
MakeEmptyStream can easily be a static method.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
File content/test/browser_side_navigation_test_utils.cc (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
content/test/browser_side_navigation_test_utils.cc:60:
BrowserSideNavigationTestUtils::BrowserSideNavigationTestUtils()
nit: Constructor should probably live closer to the destructor. I know the
ordering in the header file is different, but looks weird here. Your call.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
File content/test/browser_side_navigation_test_utils.h (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
content/test/browser_side_navigation_test_utils.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
No "(c)" needed.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
content/test/browser_side_navigation_test_utils.h:23: // Returns a pointer to
the BrowserSideNavigationTestUtils. Note: this will be
nit: Note: on new line.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
content/test/browser_side_navigation_test_utils.h:31: //
TestWebContents::NavigateAndCommit).
On 2014/11/14 15:45:26, clamy wrote:
> I am not sure if a global LazyInstance is destroyed at the end of each unit
> test, hence the Init and TearDown functions (we don't want this class to exist
> for the NavigationURLLoaderImpl unit tests). If it is, then I'll get rid of
them
> and there will just be a static Get method.

Unittests don't tear down the process, so any global LazyInstance objects will
be shared across tests. I guess you don't want that behavior, right?

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
content/test/browser_side_navigation_test_utils.h:32: static void Init();
If you are using TearDown, let's use SetUp instead of Init, so it matches other
test code.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
File content/test/test_navigation_url_loader_factory.h (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
content/test/test_navigation_url_loader_factory.h:26: protected:
Why protected?

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_web...
File content/test/test_web_contents.cc (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_web...
content/test/test_web_contents.cc:159: response,
BrowserSideNavigationTestUtils::Get()->MakeEmptyStream());
I thought the two codepaths should be distinct. It doesn't seem right to fall
through and call CommitPendingNavigation.

[carlosk, 2014-11-17 10:46:56]

Looking good.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
File content/test/browser_side_navigation_test_utils.cc (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
content/test/browser_side_navigation_test_utils.cc:21:
base::LazyInstance<scoped_ptr<BrowserSideNavigationTestUtils>>
As we need to define an Init method anyway to properly initialize the scoped
pointer with the instance why wrap it with a LazyInstance? What's the benefit it
brings in this case?

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
File content/test/browser_side_navigation_test_utils.h (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
content/test/browser_side_navigation_test_utils.h:31: //
TestWebContents::NavigateAndCommit).
On 2014/11/15 00:32:51, nasko wrote:
> On 2014/11/14 15:45:26, clamy wrote:
> > I am not sure if a global LazyInstance is destroyed at the end of each unit
> > test, hence the Init and TearDown functions (we don't want this class to
exist
> > for the NavigationURLLoaderImpl unit tests). If it is, then I'll get rid of
> them
> > and there will just be a static Get method.
> 
> Unittests don't tear down the process, so any global LazyInstance objects will
> be shared across tests. I guess you don't want that behavior, right?

There was a similar situation a while ago where I had a statically initialized
value that was indeed reset between tests. So I'd say confirming this is in good
order.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
File content/test/test_navigation_url_loader.h (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
content/test/test_navigation_url_loader.h:15: 
nit: remove this line?

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
File content/test/test_navigation_url_loader_factory.cc (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
content/test/test_navigation_url_loader_factory.cc:13:
NavigationURLLoader::SetFactoryForTesting(this);
Should we add a DCHECK in the implementation of SetFactoryForTesting to confirm
there isn't a factory already set?

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
File content/test/test_navigation_url_loader_factory.h (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
content/test/test_navigation_url_loader_factory.h:27: // RenderFrameHostFactory
implementation.
s/RenderFrameHostFactory/NavigationURLLoaderFactory/

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_web...
File content/test/test_web_contents.cc (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_web...
content/test/test_web_contents.cc:159: response,
BrowserSideNavigationTestUtils::Get()->MakeEmptyStream());
On 2014/11/15 00:32:51, nasko wrote:
> I thought the two codepaths should be distinct. It doesn't seem right to fall
> through and call CommitPendingNavigation.

If we reuse as much as possible the current API and adapt it to work with
PlzNavigate, that should make it easier to "fix" all unittests once we enable it
in the trybots.

[clamy, 2014-11-17 12:51:13]

Thanks!

https://chromiumcodereview.appspot.com/730553002/diff/1/content/browser/frame...
File content/browser/frame_host/navigator_impl.h (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.h:41: FrameTreeNode* frame_tree_node);
On 2014/11/15 00:32:51, nasko wrote:
> nit: Move this after the Navigator interface overrides.

Done.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/browser/frame...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl_unittest.cc:32: class
NavigatorTestWithBrowserSideNavigation
On 2014/11/15 00:32:51, nasko wrote:
> Why not just "BrowserSideNavigationTest"?

Well they are test about NavigatorImpl functionality - though at the same time
they all enable browser side navigation. I could try
BrowserSideNavigationNavigatorTest, not sure if it's better :).

https://chromiumcodereview.appspot.com/730553002/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl_unittest.cc:150: response,
BrowserSideNavigationTestUtils::Get()->MakeEmptyStream());
On 2014/11/15 00:32:51, nasko wrote:
> I wonder if we need the whole singleton part, as it makes it less readable.
> MakeEmptyStream can easily be a static method.

Done.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
File content/test/browser_side_navigation_test_utils.cc (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
content/test/browser_side_navigation_test_utils.cc:21:
base::LazyInstance<scoped_ptr<BrowserSideNavigationTestUtils>>
On 2014/11/17 10:46:56, carlosk wrote:
> As we need to define an Init method anyway to properly initialize the scoped
> pointer with the instance why wrap it with a LazyInstance? What's the benefit
it
> brings in this case?

It allows to have a global variable of non POD type. Basically it ensures that
you won't run into weird global initialization bugs.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
content/test/browser_side_navigation_test_utils.cc:60:
BrowserSideNavigationTestUtils::BrowserSideNavigationTestUtils()
On 2014/11/15 00:32:51, nasko wrote:
> nit: Constructor should probably live closer to the destructor. I know the
> ordering in the header file is different, but looks weird here. Your call.

Changed the ordering in the header file due to all methods now being static, so
they are close together.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
File content/test/browser_side_navigation_test_utils.h (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
content/test/browser_side_navigation_test_utils.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/11/15 00:32:51, nasko wrote:
> No "(c)" needed.

Done.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
content/test/browser_side_navigation_test_utils.h:23: // Returns a pointer to
the BrowserSideNavigationTestUtils. Note: this will be
On 2014/11/15 00:32:51, nasko wrote:
> nit: Note: on new line.

Made all the method static and removed the singleton getter.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
content/test/browser_side_navigation_test_utils.h:31: //
TestWebContents::NavigateAndCommit).
On 2014/11/15 00:32:51, nasko wrote:
> On 2014/11/14 15:45:26, clamy wrote:
> > I am not sure if a global LazyInstance is destroyed at the end of each unit
> > test, hence the Init and TearDown functions (we don't want this class to
exist
> > for the NavigationURLLoaderImpl unit tests). If it is, then I'll get rid of
> them
> > and there will just be a static Get method.
> 
> Unittests don't tear down the process, so any global LazyInstance objects will
> be shared across tests. I guess you don't want that behavior, right?

Yes. In particular, we need no to have the singleton during the unit tests of
the NavigationURLLoader (because we need not to have a
TestNavigationURLLoaderFactory registered).

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/browser_...
content/test/browser_side_navigation_test_utils.h:32: static void Init();
On 2014/11/15 00:32:51, nasko wrote:
> If you are using TearDown, let's use SetUp instead of Init, so it matches
other
> test code.

Done.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
File content/test/test_navigation_url_loader.h (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
content/test/test_navigation_url_loader.h:15: 
On 2014/11/17 10:46:56, carlosk wrote:
> nit: remove this line?

Will do in the next patch set.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
File content/test/test_navigation_url_loader_factory.h (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
content/test/test_navigation_url_loader_factory.h:26: protected:
On 2014/11/15 00:32:51, nasko wrote:
> Why protected?

I followed TestRenderFrameHostFactory which had it protected, but I don't think
it is very useful. Switched it to public.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_web...
File content/test/test_web_contents.cc (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_web...
content/test/test_web_contents.cc:159: response,
BrowserSideNavigationTestUtils::Get()->MakeEmptyStream());
On 2014/11/15 00:32:51, nasko wrote:
> I thought the two codepaths should be distinct. It doesn't seem right to fall
> through and call CommitPendingNavigation.

For now we have to call CommitPendingNavigation to actually switch the
RenderFrameHost. When Carlos' change with the dependency on the
pending_render_frame_host lands, we won't need to execute it anymore, so we can
have two distinct code paths.

[nasko, 2014-11-17 22:40:02]

Just one minor comment, which I'm not certain about. I think it is good to go
once Carlos' review feedback is incorporated.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_web...
File content/test/test_web_contents.cc (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_web...
content/test/test_web_contents.cc:159: response,
BrowserSideNavigationTestUtils::Get()->MakeEmptyStream());
On 2014/11/17 12:51:13, clamy wrote:
> On 2014/11/15 00:32:51, nasko wrote:
> > I thought the two codepaths should be distinct. It doesn't seem right to
fall
> > through and call CommitPendingNavigation.
> 
> For now we have to call CommitPendingNavigation to actually switch the
> RenderFrameHost. When Carlos' change with the dependency on the
> pending_render_frame_host lands, we won't need to execute it anymore, so we
can
> have two distinct code paths.

Acknowledged.

https://chromiumcodereview.appspot.com/730553002/diff/20001/content/test/brow...
File content/test/browser_side_navigation_test_utils.h (right):

https://chromiumcodereview.appspot.com/730553002/diff/20001/content/test/brow...
content/test/browser_side_navigation_test_utils.h:36: static
scoped_ptr<StreamHandle> MakeEmptyStream();
A class full of static methods is equivalent to a bunch of methods in a
namespace. This is what other utils files do, so I wonder which one is a better
approach.

[clamy, 2014-11-18 12:33:22]

Thanks. I have uploaded a new version which removes the class. Let me know what
you prefer.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
File content/test/test_navigation_url_loader.h (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
content/test/test_navigation_url_loader.h:15: 
On 2014/11/17 12:51:13, clamy wrote:
> On 2014/11/17 10:46:56, carlosk wrote:
> > nit: remove this line?
> 
> Will do in the next patch set.

Done.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
File content/test/test_navigation_url_loader_factory.cc (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
content/test/test_navigation_url_loader_factory.cc:13:
NavigationURLLoader::SetFactoryForTesting(this);
On 2014/11/17 10:46:56, carlosk wrote:
> Should we add a DCHECK in the implementation of SetFactoryForTesting to
confirm
> there isn't a factory already set?

Done.

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
File content/test/test_navigation_url_loader_factory.h (right):

https://chromiumcodereview.appspot.com/730553002/diff/1/content/test/test_nav...
content/test/test_navigation_url_loader_factory.h:27: // RenderFrameHostFactory
implementation.
On 2014/11/17 10:46:56, carlosk wrote:
> s/RenderFrameHostFactory/NavigationURLLoaderFactory/

Done.

https://chromiumcodereview.appspot.com/730553002/diff/20001/content/test/brow...
File content/test/browser_side_navigation_test_utils.h (right):

https://chromiumcodereview.appspot.com/730553002/diff/20001/content/test/brow...
content/test/browser_side_navigation_test_utils.h:36: static
scoped_ptr<StreamHandle> MakeEmptyStream();
On 2014/11/17 22:40:02, nasko wrote:
> A class full of static methods is equivalent to a bunch of methods in a
> namespace. This is what other utils files do, so I wonder which one is a
better
> approach.
When writing the web_url_request_util, jam@ specifically commented on this
subject that we do not use nested namespaces inside content. So unless I have
another namespace outside of content (which looks a bit weird IMO), I can't
bundle them in a namespace. I have uploaded a new version with more specific
functions names not in a class. Do you like it better?

[carlosk, 2014-11-18 12:41:34]

LGTM

[nasko, 2014-11-18 17:10:21]

LGTM

[clamy, 2014-11-18 17:17:09]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-11-18 17:18:50]

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

[commit-bot: I haz the power, 2014-11-18 18:09:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-18 18:10:49]

Patchset 3 (id:??) landed as
https://crrev.com/72c7279ae8a307aba091bafc188cebd97a33b899
Cr-Commit-Position: refs/heads/master@{#304640}