[Webkit-unassigned] [Bug 141558] [GTK] GObject DOM bindings object are cached forever

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=141558

Sergio Villar Senin <svillar at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #246665|review?                     |review+
              Flags|                            |

--- Comment #2 from Sergio Villar Senin <svillar at igalia.com> ---
Comment on attachment 246665
  --> https://bugs.webkit.org/attachment.cgi?id=246665
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246665&action=review

Awesome patch. The code is highly simplified and the new test coverage is simply superb.

I have a few comments.

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:73
> +static DOMObjectCacheFrameObserver& getOrCreateDOMObjectCacheFrameObserver(WebCore::Frame* frame)

As frame is never null this should be a reference.

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:87
> +    DOMObjectCacheFrameObserver(WebCore::Frame* frame)

This should also be ideally a reference but the interface is a pointer :(

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:97
> +    void addObjectCacheData(DOMObjectCacheData* data)

This can be a reference.

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:101
> +        m_objects.append(data);

See bellow my comment about m_objects.

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:109
> +        observer->m_objects.removeFirstMatching([finalizedObject](DOMObjectCacheData* data) {

See bellow my comment about m_objects.

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:120
> +        for (auto data : objects) {

I think it's preferred to be explicit about the type being a pointer, so "auto*" it's slightly better in this case.

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:139
> +    Vector<DOMObjectCacheData*> m_objects;

Since respecting the insertion order is not needed, I think we should use a HashSet here. Being a set means that we can be sure that the objects are only once in the data structure and will also help with searches (contains() and the like).

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:164
>      if (domObjects().get(objectHandle))

!domObjects().contains(objectHandle) ?

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:171
> +    if (domObjects().get(objectHandle))

Ditto.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/DOMNodeTest.cpp:216
> +        // Get the same elment twice should return the same pointer.

Nit: element

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150217/0987020c/attachment-0002.html>