[webkit-reviews] review denied: [Bug 40302] [GTK] Memory managament for DOM GObject wrappers : [Attachment 74295] domcache.diff

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Xan Lopez
<xan.lopez at gmail.com>'s request for review:
Bug 40302: [GTK] Memory managament for DOM GObject wrappers
https://bugs.webkit.org/show_bug.cgi?id=40302

Attachment 74295: domcache.diff
https://bugs.webkit.org/attachment.cgi?id=74295&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=74295&action=review

Looks good, though I have a few nagging concerns (some of which I've outlined
below). Another issue is what happens when a developer uses a DOM object to
change the frame location. Isn't there the possibility that it will unref the
DOM wrapper out from under them?

> WebCore/bindings/gobject/DOMObjectCache.cpp:72
> +    // Unrefing the objects removes them from the cache in their

Should be "Unreffing" here.

> WebCore/bindings/gobject/DOMObjectCache.cpp:77
> +	   DOMObjectCacheData* data =
static_cast<DOMObjectCacheData*>(iter->second);

I don't think this static_cast is necessary.

> WebCore/bindings/gobject/DOMObjectCache.cpp:78
> +	   if (data && (!view || data->view == view))

Would ASSERT(data) be more appropriate, or are there cases where the data can,
in fact, be NULL?

> WebCore/bindings/gobject/DOMObjectCache.cpp:101
> +    return g_object_ref(data->object);

Won't this lead to leaks, since the cache only does one unref when cleared and
DOMObjectCache::get can be called multiple times before that?

> WebCore/bindings/gobject/DOMObjectCache.cpp:119
> +    put(static_cast<void*>(objectHandle), wrapper);

I don't think the cast is necessary to get void*.

> WebCore/bindings/gobject/DOMObjectCache.h:22
> +#include <glib-object.h>

Using void* instead of gpointer would prevent the need for this include, which
would be a win for compile time.

> WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:703
> -    // this is only interesting once we provide an external API for the DOM
> +    if (!ready)
> +	   DOMObjectCache::clearByView(getViewFromFrame(m_frame));

Doesn't this mean that when an iframe reloads, all cached DOM objects will be
removed for the entire view? I think it would be better to track per-frame.