NoState Prefetch: nostate prefetch browser tests.

chrome/browser/prerender/prerender_manager_factory.h

Index: chrome/browser/prerender/prerender_manager_factory.h
diff --git a/chrome/browser/prerender/prerender_manager_factory.h b/chrome/browser/prerender/prerender_manager_factory.h
index 46e7e67f2db756bb316e4ada0a1de322763340b4..b56a166e61a1b5b5c60f6b0e864d5a4f4f868243 100644
--- a/chrome/browser/prerender/prerender_manager_factory.h
+++ b/chrome/browser/prerender/prerender_manager_factory.h
@@ -30,6 +30,17 @@ class PrerenderManagerFactory : public BrowserContextKeyedServiceFactory {
 
   static PrerenderManagerFactory* GetInstance();
 
+  // As the prerender manager gets used very early in browser creation (eg, by
+  // the prerender tab helper), we can't use
+  // KeyedServiceFactory::SetTestingFactory, as the PrerenderManager has been
+  // created already by the time in-process browser tests are able to see a
+  // profile.

[droger, 2016/09/22 14:54:58]

I don't think this is accurate.

BuildServiceInstanceFor() will be called only once per profile.

If the prerender manager was created before the test starts, then it won't be
created after and the testing_create_function_ will not be called.

I don't understand why exactly you need this, and how the test could fail
without this but pass with it.

I discussed a bit with sdefresne (which is our local expert on service
factories), and it seems that the natural approach would be to add the necessary
customizations in the vanilla PrerenderManager instead of trying to instantiate
a test one.

I guess this would be something like adding a
SetPrefetchCacheOverrideTimeoutForTesting() method and setting it to a very low
value for the test, to get a "cold" prefetch.

[mattcary, 2016/09/23 07:33:18]

I think this issue is specific to the in-process browser tests. It is indeed
true that this is called once per profile, but we don't get into the testing
infrastructure code until after the browser and profile have been created, and
by that time the prerender manager has already been created due (I think) to the
prerender tab helper.

So it seemed the easiest way to do it was to make a change that could be used
without having a profile, which I could hook before the browser is created.

In my experience, trying to use "low" values for timing issues ends up being
very fragile, because of different test setups, machines etc. On my desktop
there's about 1/2 second between the two navigations, so I don't think we could
consistently set a non-trivial low value for the cold prefetch.

Plus there's my aversion to having testing harnesses mixed up with production
code.

[droger, 2016/09/23 08:15:36]

My point is:

- if the prerender manager is created earlier than the test, then your new code
is not called at all.

Another way to say the same thing:

- if your code is called, then it means that the prerender manager is not
created earlier.


I think at minimum the comment is wrong. Let me try to patch the CL to
investigate a bit.

+  using PrerenderCreateFunction = PrerenderManager*(Profile*);

[droger, 2016/09/23 09:09:33]

We don't need a profile here, BrowserContext is enough. This will also simplify
the implementation.

Could we use BrowserContextKeyedServiceFactory::TestingFactoryFunction

?

+  static void set_testing_create_function(
+      PrerenderCreateFunction create_function) {
+    testing_create_function_ = create_function;
+  }
+
  private:
   friend struct base::DefaultSingletonTraits<PrerenderManagerFactory>;
 
@@ -41,6 +52,8 @@ class PrerenderManagerFactory : public BrowserContextKeyedServiceFactory {
       content::BrowserContext* profile) const override;
   content::BrowserContext* GetBrowserContextToUse(
       content::BrowserContext* context) const override;
+
+  static PrerenderCreateFunction* testing_create_function_;
 };
 
 }  // namespace prerender