[Oilpan] Migrate classes under module/webgl onto oilpan heap

Source/modules/webgl/WebGLFramebuffer.cpp

Index: Source/modules/webgl/WebGLFramebuffer.cpp
diff --git a/Source/modules/webgl/WebGLFramebuffer.cpp b/Source/modules/webgl/WebGLFramebuffer.cpp
index e9dee4049417d46953014e526d414bf565c1c9bd..5853a219d8470c43ea8739db1798e220e2337186 100644
--- a/Source/modules/webgl/WebGLFramebuffer.cpp
+++ b/Source/modules/webgl/WebGLFramebuffer.cpp
@@ -38,7 +38,7 @@ namespace {
 
 class WebGLRenderbufferAttachment final : public WebGLFramebuffer::WebGLAttachment {
 public:
-    static PassRefPtrWillBeRawPtr<WebGLFramebuffer::WebGLAttachment> create(WebGLRenderbuffer*);
+    static WebGLFramebuffer::WebGLAttachment* create(WebGLRenderbuffer*);
 
     DECLARE_VIRTUAL_TRACE();
 
@@ -57,12 +57,12 @@ private:
     void attach(WebGraphicsContext3D*, GLenum target, GLenum attachment) override;
     void unattach(WebGraphicsContext3D*, GLenum target, GLenum attachment) override;
 
-    RefPtrWillBeMember<WebGLRenderbuffer> m_renderbuffer;
+    Member<WebGLRenderbuffer> m_renderbuffer;
 };
 
-PassRefPtrWillBeRawPtr<WebGLFramebuffer::WebGLAttachment> WebGLRenderbufferAttachment::create(WebGLRenderbuffer* renderbuffer)
+WebGLFramebuffer::WebGLAttachment* WebGLRenderbufferAttachment::create(WebGLRenderbuffer* renderbuffer)
 {
-    return adoptRefWillBeNoop(new WebGLRenderbufferAttachment(renderbuffer));
+    return new WebGLRenderbufferAttachment(renderbuffer);
 }
 
 DEFINE_TRACE(WebGLRenderbufferAttachment)
@@ -140,12 +140,13 @@ void WebGLRenderbufferAttachment::unattach(WebGraphicsContext3D* context, GLenum
 
 GLenum WebGLRenderbufferAttachment::type() const
 {
+    notImplemented();
     return 0;
 }
 
 class WebGLTextureAttachment final : public WebGLFramebuffer::WebGLAttachment {
 public:
-    static PassRefPtrWillBeRawPtr<WebGLFramebuffer::WebGLAttachment> create(WebGLTexture*, GLenum target, GLint level);
+    static WebGLFramebuffer::WebGLAttachment* create(WebGLTexture*, GLenum target, GLint level);
 
     DECLARE_VIRTUAL_TRACE();
 
@@ -164,14 +165,14 @@ private:
     void attach(WebGraphicsContext3D*, GLenum target, GLenum attachment) override;
     void unattach(WebGraphicsContext3D*, GLenum target, GLenum attachment) override;
 
-    RefPtrWillBeMember<WebGLTexture> m_texture;
+    Member<WebGLTexture> m_texture;
     GLenum m_target;
     GLint m_level;
 };
 
-PassRefPtrWillBeRawPtr<WebGLFramebuffer::WebGLAttachment> WebGLTextureAttachment::create(WebGLTexture* texture, GLenum target, GLint level)
+WebGLFramebuffer::WebGLAttachment* WebGLTextureAttachment::create(WebGLTexture* texture, GLenum target, GLint level)
 {
-    return adoptRefWillBeNoop(new WebGLTextureAttachment(texture, target, level));
+    return new WebGLTextureAttachment(texture, target, level);
 }
 
 DEFINE_TRACE(WebGLTextureAttachment)
@@ -324,9 +325,9 @@ WebGLFramebuffer::WebGLAttachment::~WebGLAttachment()
 {
 }
 
-PassRefPtrWillBeRawPtr<WebGLFramebuffer> WebGLFramebuffer::create(WebGLRenderingContextBase* ctx)
+WebGLFramebuffer* WebGLFramebuffer::create(WebGLRenderingContextBase* ctx)
 {
-    return adoptRefWillBeNoop(new WebGLFramebuffer(ctx));
+    return new WebGLFramebuffer(ctx);
 }
 
 WebGLFramebuffer::WebGLFramebuffer(WebGLRenderingContextBase* ctx)
@@ -339,14 +340,11 @@ WebGLFramebuffer::WebGLFramebuffer(WebGLRenderingContextBase* ctx)
 
 WebGLFramebuffer::~WebGLFramebuffer()
 {
-    // Delete the platform framebuffer resource. Explicit detachment
-    // is for the benefit of Oilpan, where the framebuffer object
-    // isn't detached when it and the WebGLRenderingContextBase object
-    // it is registered with are both finalized. Without Oilpan, the
-    // object will have been detached.
-    //
-    // To keep the code regular, the trivial detach()ment is always
-    // performed.
+    // Attachments in |m_attachments| will be deleted from other places, so we
+    // clear it to avoid deleting those attachments in detachAndDeleteObject().
+    m_attachments.clear();

[sof, 2015/08/09 08:35:21]

I don't think is quite correct with Oilpan:

 - if the context is swept at the same time, it will take care of detaching
WebGLFramebuffer. Hence, this map will be empty & the dtor a no-op.
 - if the context isn't swept at the same time, we will clear m_attachments,
causing onDetached() not to be called for its elements.

So, there are no guarantees that onDetached() will always be called for its
WebGLAttachments with Oilpan on destruction. I think that implies that they need
to handle detachment on their own (I believe they do already.) And if they do,
the clearing here isn't needed and the iteration over them in deleteObjectImpl()
can be removed.

[haraken, 2015/08/09 14:16:45]

Yeah, you're right -- I think we can just remove the onDetach call (though I'd
prefer doing that in a follow-up CL since this CL is already complex).

[sof, 2015/08/09 15:07:37]

That's fine, I think. The Oilpan changes made here do no harm, but are not
needed.

[peria, 2015/08/10 03:15:23]

Thanks, I filed an issue to follow it up.
http://crbug.com/518706

+
+    // See the comment in WebGLObject::detachAndDeleteObject().
     detachAndDeleteObject();
 }
 
@@ -581,17 +579,17 @@ bool WebGLFramebuffer::hasStencilBuffer() const
 
 void WebGLFramebuffer::deleteObjectImpl(WebGraphicsContext3D* context3d)
 {
-#if !ENABLE(OILPAN)
-    // With Oilpan, both the AttachmentMap and its WebGLAttachment objects are
-    // GCed objects and cannot be accessed, as they may have been finalized
+    // Both the AttachmentMap and its WebGLAttachment objects are GCed

[sof, 2015/08/09 08:35:20]

The code and the comment is inconsistent now; do these really need to be
notified of onDetached() with Oilpan?

[peria, 2015/08/10 03:15:23]

Removed latter half comment.
I think it is not needed to call onDetached(), but I will work for it in a
follow-up CL.

+    // objects and cannot be accessed, as they may have been finalized
     // already during the same GC sweep.
     //
     // The WebGLAttachment-derived classes instead handle detachment
     // on their own when finalizing, so the explicit notification is
     // not needed.
-    for (const auto& attachment : m_attachments)
-        attachment.value->onDetached(context3d);
-#endif
+    if (!m_attachments.isEmpty()) {

[haraken, 2015/08/07 07:43:58]

This check won't be needed, since if the m_attachements is empty, the following
loop does nothing.

[peria, 2015/08/10 03:15:23]

Done.

+        for (const auto& attachment : m_attachments)
+            attachment.value->onDetached(context3d);
+    }
 
     context3d->deleteFramebuffer(m_object);
     m_object = 0;
@@ -663,9 +661,7 @@ bool WebGLFramebuffer::getReadBufferFormatAndType(GLenum* format, GLenum* type)
 
 DEFINE_TRACE(WebGLFramebuffer)
 {
-#if ENABLE(OILPAN)
     visitor->trace(m_attachments);
-#endif
     WebGLContextObject::trace(visitor);
 }