Handle interface to prerenders.

chrome/browser/prerender/prerender_handle.h

+// Copyright (c) 2012 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 CHROME_BROWSER_PRERENDER_PRERENDER_HANDLE_H_
+#define CHROME_BROWSER_PRERENDER_PRERENDER_HANDLE_H_
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "chrome/browser/prerender/prerender_final_status.h"
+
+namespace prerender {
+
+class PrerenderContents;
+
+// An opaque handle to a prerender.
+class PrerenderHandleInterface :

[gavinp, 2012/06/17 17:41:15]

I've now renamed this to PrerenderInterface, and put it in
prerender_interface.h.

There's still a PrerenderHandle, and it gets rid of the really confusing logic
around Destroy().

[dominich, 2012/06/18 15:32:44]

I /really/ don't understand why you need an interface for a PrerenderHandle.
There should only be one type of PrerenderHandle and it should be concrete and
non-extensible. Maybe even POD.

[gavinp, 2012/06/18 16:40:48]

Hrm.

1. A prerender can be canceled by any one of the holders of a handle (do we
support multiple handles to the same prerender?), invalidating all other
handles.
2. The prerender manager can cancel a prerender at any time, invalidating all
handles.
3. Handles should be small and PODlike, to fit in an STL container.

The abstract interface seemed like a good way to expose the interface to a
prerender outside of the prerender_manager. As well, moving methods like
IsPending() up here get rid of loops over the
PrerenderManager::contents_data_list_.

The handle seemed like a good way to let consumers grab a pointer to the running
prerender, but had to be possible to invalidate it from there, and that should
invalidate the other handles.

So if we moved the PrerenderInterface methods out of PrerenderManager, that'd be
a huge win: the PrerenderManager is pretty complicated as it is. So I'll
continue with a CL that has a PrerenderInterface and a PrerenderHandle, but I
want to talk about this more once I've done that upload.

[mmenke, 2012/06/18 18:32:03]

Seems like there are a lot of ways to implement this class.  Since this is a
non-POD class, you also have to DISALLOW_COPY_AND_ASSIGN.

What I'd pictured was something more like:

class PrerenderHandle {
 public:
  PrerenderHandle(PrerenderContents* prerender_contents);

  // No methods needed, most likely, though maybe an
  // bool IsPending() function would be useful.

 private:
  // Maybe friend PrerenderManager and add a private
  // function to access this.  Not ideal, but think it's
  // better than ref counting.
  weak_ptr<PrerenderContents> prerender_contents_;

  // Copy and assign are allowed.
};

Alternatively, you could just make it support weak pointer.  I don't see why you
need both.

+  public base::RefCounted<PrerenderHandleInterface>,
+  public base::SupportsWeakPtr<PrerenderHandleInterface> {
+ public:
+  virtual bool DidFinishLoading() const = 0;
+
+  // Is this prerender created, but not yet launched, as in a link prerender
+  // launched from a page that is itself prerendering?
+  virtual bool IsPending() const = 0;
+
+ protected:
+  PrerenderHandleInterface();
+  virtual ~PrerenderHandleInterface() = 0;
+
+ private:
+  friend class base::RefCounted<PrerenderHandleInterface>;
+  friend class PrerenderContents;  // May call Destroy.
+
+  // Called from PrerenderContents, notifies this handle that this prerender
+  // either is no longer running, or never will run if it is not yet started.
+  // TODO(gavinp): Implement WeakScopedRefPtr, and use it to make this method
+  // redundant.
+  virtual void Destroy() = 0;
+};
+
+typedef scoped_refptr<PrerenderHandleInterface> PrerenderHandle;
+typedef base::WeakPtr<PrerenderHandleInterface> WeakPrerenderHandle;

[mmenke, 2012/06/18 18:32:03]

This just makes things hard to follow, and is actually a bit of an anti-pattern,
I believe.

+
+}  // namespace prerender
+
+#endif  // CHROME_BROWSER_PRERENDER_PRERENDER_HANDLE_H_