Add a NavigationThrottle to the public content/ interface

content/test/test_navigation_handle.h

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_TEST_TEST_NAVIGATION_HANDLE_H_
+#define CONTENT_TEST_TEST_NAVIGATION_HANDLE_H_
+
+#include <string>
+
+#include "content/browser/frame_host/navigation_handle_impl.h"
+#include "content/public/test/navigation_handle_tester.h"
+
+namespace content {
+
+// A test version of the NavigationHandle that can be used to simulate various
+// navigation call sby embedders of content/.

[Charlie Reis, 2015/09/16 00:09:25]

nit: calls by

[clamy, 2015/09/16 01:03:22]

Done.

+class TestNavigationHandle : public NavigationHandleImpl,

[Charlie Reis, 2015/09/16 00:09:25]

I'm probably missing something here, but why do we need this class?  It looks
like it's just wrapping methods of NavigationHandleImpl which are already
public.

I'm lamenting all the overhead with this test-specific class and the
NavigationHandleFactory.  It would be nice to avoid TestNavigationHandle and
NavigationHandleFactory, if NavigationHandleImpl itself is primarily a data
structure.

[clamy, 2015/09/16 01:03:22]

Well the goal here is to have embedders of content be able to write unit tests
for NavigationThrottles, which is considerably simpler if you have a
NavigationHandleTester class that allows you to have these methods explicitly
called on the NavigationHandleImpl. Since NavigationHandleTester is a test
interface, it can't be implemented by NavigationHandleImpl. And I added the
factories to make sure that if people try to static_cast a NavigationHandleImpl
in its test version it does not create an error.

I guess an alternative would be to have static methods on
WebContentsTester/RenderFrameHostTester to create a NavigationHandle and have
the methods called on it.

[Charlie Reis, 2015/09/16 17:39:27]

[+nick]

I see that you're mostly using the TestFoo/FooTester/etc pattern correctly
(apart from the RegisterThrottleForTesting method on NavigationHandle), but I'm
just not a fan of that pattern.  It's so heavyweight to read in the code, and
Nick points out how undiscoverable it is for people wanting to write tests, as
well as how likely it is to get out of date as the impl class changes.

One straw man solution (if we just need to expose WillStartRequest and
WillRedirectRequest to tests outside content/) is to put
WillStartRequestForTesting and WillRedirectRequestForTesting on the public
NavigationHandle.  (Maybe a creation method as well?)  In that approach, we
don't need any of the extra test classes and it's obvious to test writers what
functionality is available to them in NavigationHandle.  It also makes the tests
themselves easier to read (e.g., wouldn't have to wonder whether
SimulateWillStartRequest does other things than just wrapping WillStartRequest).

If that doesn't work, Nick has some ideas for an alternative approach that
doesn't have some of the drawbacks of the TestFoo/FooTester pattern.

I can be overruled here, but the current approach seems verbose and cumbersome
to me.

[nasko, 2015/09/16 20:33:39]

I'd second that a simpler way of testing is going to be better. It took me quite
some time to even understand how it all works.

+                             public NavigationHandleTester {
+ public:
+  ~TestNavigationHandle() override;
+
+  static TestNavigationHandle* Create(const GURL& url,
+                                      const GURL& validated_url,
+                                      const bool is_main_frame,
+                                      NavigatorDelegate* delegate);
+
+  // NavigationHandlerTester implementation.
+  NavigationThrottle::ThrottleCheckResult SimulateWillStartRequest(
+      bool is_post,
+      const Referrer& sanitized_referrer,
+      bool has_user_gesture,
+      ui::PageTransition transition,
+      bool is_external_protocol) override;
+  NavigationThrottle::ThrottleCheckResult SimulateWillRedirectRequest(
+      const GURL& new_url,
+      const GURL& new_validated_url,
+      bool new_method_is_post,
+      const GURL& new_referrer_url,
+      bool new_is_external_protocol) override;
+
+ private:
+  TestNavigationHandle(const GURL& url,
+                       const GURL& validated_url,
+                       const bool is_main_frame,
+                       NavigatorDelegate* delegate);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_TEST_TEST_NAVIGATION_HANDLE_H_