[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

--- Comment #3 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #2)
> Comment on attachment 246665 [details]
> 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.

Thanks, hopefully it's also correct :-)

> 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.

Ok.

> > Source/WebCore/bindings/gobject/DOMObjectCache.cpp:87
> > +    DOMObjectCacheFrameObserver(WebCore::Frame* frame)
> 
> This should also be ideally a reference but the interface is a pointer :(

We can still receive a Ref and pass the address to the parent constructor.

> > Source/WebCore/bindings/gobject/DOMObjectCache.cpp:97
> > +    void addObjectCacheData(DOMObjectCacheData* data)
> 
> This can be a reference.

Ok.

> > 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.

Ok.

> > 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).

It was a HashSet initially, I don't remember why I changed to a Vector.

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

Actually domObjects().contains(objectHandle), since we want to return early when the wrapped object is already cached, but yes, contains is better thnn get in this case.

> > 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

Sure.

-- 
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/49e8adee/attachment-0002.html>