Make WebPrivatePtr capable of wrapping garbage collected objects.

public/platform/WebPrivateGarbageCollectedPtr.h

+/* Copyright (c) 2014 The Chromium Authors. All rihts reserved.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#ifndef WebPrivateGarbageCollectedPtr_h

[Mads Ager (chromium), 2014/02/17 12:26:29]

We had a chat about this just now. We can probably get rid of most of the ifdef
spaghetti here.

The proposal would be to introduce a transition type for this:

WillBeWebPrivateGarbageCollectedPtr

Then in this file we can have:

#if ENABLE(OILPAN)
#define WillBeWebPrivateGarbageCollectedPtr WebPrivateGarbageCollectedPtr
#else
#define WillBeWebPrivateGarbageCollectedPtr WebPrivatePtr
#endif

(and use template aliases for clang to keep us honest)

Then there will be no need for further ifdefs in this file. This file only has
to provide the implementation that uses Persistent and will only be used when
compiling with ENABLE(OILPAN).

[sof, 2014/02/17 12:45:03]

There's a slight problem with that: uses of WebPrivateGarbageCollectedPtr is
compiled outside of Blink (e.g., WebSpeechGrammar.h), hence won't have access to
ENABLE(OILPAN) -- which it needs to in order to resolve its use of the
transition type WillBeWebPrivateGarbageCollectedPtr.

[Mads Ager (chromium), 2014/02/17 12:54:30]

Yeah, I see that now. Hmm. I'll give it a bit more thought.

If we need to keep it this way I think we might need another type down the line.
If we want to ship some of these with oilpan and not others we will need a type
that is always a persistent independently of the ENABLE(OILPAN) define.

If we need to keep it this way, maybe we should make it a bit more verbose by
repeating the method signatures instead of having so many ifdefs?

#if INSIDE_BLINK
#if ENABLE(OILPAN)
// all the oilpan code
#else
// all the ref counting code
#endif
#endif

It would duplicate a bit of code but I think it would be easier to reason about
what is going on in the two implementations?

[sof, 2014/02/17 14:09:00]

I think we get out ahead by doing so, switched over.

+#define WebPrivateGarbageCollectedPtr_h
+
+#include "WebCommon.h"
+
+#if INSIDE_BLINK
+#include "wtf/PassRefPtr.h"
+
+namespace WebCore {

[Mads Ager (chromium), 2014/02/17 13:31:31]

We should be able to remove all of the forward declarations and the duplicate
definitions of PassRefPtrWillBeRawPtr if we just have:

#if INSIDE_BLINK
#include "wtf/PassRefPtr.h"
#include "heap/Handle.h"
#endif

?

[sof, 2014/02/17 14:09:00]

That would be cleaner, the reason why it's like this atm is that DEPS don't
allow heap/ to be included here. 

The wish/need to reuse an existing Persistent made the duplication slightly
worse, so I've switched over to a direct include of heap/Handle.h -- might this
be allowed by the platform owners?

+template<typename T> class Persistent;
+}
+
+namespace WTF {
+template<typename T> class RawPtr;
+}
+
+# if ENABLE(OILPAN)

[Mads Ager (chromium), 2014/02/17 12:26:29]

Please remove the spaces between # and if/define/else. Also, please use template
aliases when using clang to keep us honest.

[sof, 2014/02/17 14:09:00]

Done.

+# define PassRefPtrWillBeRawPtr RawPtr
+# else
+# define PassRefPtrWillBeRawPtr PassRefPtr
+# endif
+#endif
+
+namespace blink {
+
+// A variant of WebPrivatePtr that holds a Blink object that's garbage
+// collected, without exposing the details of how (and with what
+// types.) If no support for having such objects (i.e., no Oilpan),
+// WebPrivateGarbageCollectedPtr is equal to WebPrivatePtr and the
+// object will be reference counted.
+//
+// See WebPrivatePtr.h comment for how to make use of
+// WebPrivateGarbageCollectedPtr; same pattern applies, but for one
+// detail. Instead of declaring & defining,
+//
+//    #if INSIDE_BLINK
+//        WebFoo(const WTF::PassRefPtr<WebCore::Foo>&);
+//    #endif
+//
+// provide
+//
+//    #if INSIDE_BLINK
+//        WebFoo(const WTF::PassRefPtrWillBeRawPtr<WebCore::Foo>&);
+//    #endif
+//
+// FIXME: oilpan: when transition types are no more, remove the
+// unseemly #if ENABLE(OILPAN) blocks.
+template <typename T>
+class WebPrivateGarbageCollectedPtr {
+public:
+    WebPrivateGarbageCollectedPtr() : m_ptr(0) { }
+    ~WebPrivateGarbageCollectedPtr()
+    {
+        // We don't destruct the object pointed by m_ptr here because we don't
+        // want to expose destructors of core classes to embedders. We should
+        // call reset() manually in destructors of classes with WebPrivateGarbageCollectedPtr
+        // members.
+        BLINK_ASSERT(!m_ptr);
+    }
+
+    bool isNull() const { return !m_ptr; }
+
+#if INSIDE_BLINK
+    WebPrivateGarbageCollectedPtr(const WTF::PassRefPtrWillBeRawPtr<T>& p)
+    {
+# if ENABLE(OILPAN)
+        assign(p);
+# else
+        m_ptr = p.leakRef();
+# endif
+    }
+
+    void reset()
+    {
+# if ENABLE(OILPAN)
+        if (m_ptr) {
+            delete reinterpret_cast<WebCore::Persistent<T>*>(m_ptr);
+            m_ptr = 0;
+        }
+# else
+        assign(0);
+# endif
+    }
+
+    WebPrivateGarbageCollectedPtr<T>& operator=(const WebPrivateGarbageCollectedPtr<T>& other)
+    {
+# if ENABLE(OILPAN)
+        assign(other.get());
+# else
+        T* p = other.m_ptr;
+        if (p)
+            p->ref();
+        assign(p);
+# endif
+        return *this;
+    }
+
+    WebPrivateGarbageCollectedPtr<T>& operator=(const WTF::PassRefPtrWillBeRawPtr<T>& p)
+    {
+# if ENABLE(OILPAN)
+        assign(p);
+# else
+        assign(p.leakRef());
+# endif
+        return *this;
+    }
+
+    T* get() const
+    {
+# if ENABLE(OILPAN)
+        if (!m_ptr)
+            return 0;
+        return (reinterpret_cast<WebCore::Persistent<T>* >(m_ptr))->get();
+# else
+        return m_ptr;
+# endif
+    }
+
+    T* operator->() const
+    {
+        ASSERT(m_ptr);
+# if ENABLE(OILPAN)
+        return (reinterpret_cast<WebCore::Persistent<T>* >(m_ptr))->get();
+# else
+        return m_ptr;
+# endif
+    }
+#endif
+
+private:
+#if INSIDE_BLINK
+    void assign(T* p)
+    {
+# if ENABLE(OILPAN)
+        reset();
+        // Keep an opaque pointer object pointer to avoid exposing
+        // representation details. (Relatively speaking, it's
+        // acceptable to have this be as a T* rather than a
+        // generic void*.
+        m_ptr = reinterpret_cast<T*>(new WebCore::Persistent<T>(p));

[Mads Ager (chromium), 2014/02/17 12:26:29]

We really shouldn't have to do this. We should allocate only one persistent and
then we should just use assignment. We shouldn't have to deallocate the
persistent and allocate a new one. We just need to have that one persistent
point to a new object. So, instead of using reset here, we should cast m_ptr to
a Persistent<T>* and assign to it.

[Mads Ager (chromium), 2014/02/17 13:31:31]

In addition to reusing the persistent we should be careful to make this work
with isNull. If we are assigning a null pointer to this one we need to
deallocate the persistent. Currently this code will always allocate a persistent
which will break isNull if assigning null. 

[sof, 2014/02/17 14:09:00]

Oh yes, it completely failed to consider the null case. Addressed now + reusing
the persistent in assign().

+# else
+        // p is already ref'd for us by the caller
+        if (m_ptr)
+            m_ptr->deref();
+        m_ptr = p;
+# endif
+    }
+#else
+    // Disable the assignment operator; we define it above for when
+    // INSIDE_BLINK is set, but we need to make sure that it is not
+    // used outside there; the compiler-provided version won't handle reference
+    // counting properly.
+    WebPrivateGarbageCollectedPtr<T>& operator=(const WebPrivateGarbageCollectedPtr<T>& other);
+#endif
+    // Disable the copy constructor; classes that contain a
+    // WebPrivateGarbageCollectedPtr should implement their copy
+    // constructor using assign().
+    WebPrivateGarbageCollectedPtr(const WebPrivateGarbageCollectedPtr<T>&);
+
+    T* m_ptr;
+};
+
+} // namespace blink
+
+#endif