[Webkit-unassigned] [Bug 27488] SVG and XPath memory leaks in V8 bindings

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from Mads Ager <ager at chromium.org>  2009-07-24 02:37:17 PDT ---
(In reply to comment #9)
> Should this $wrapper use my?

yes, done.

> I would have probably used a different set of variables to make that whole
> block use less copy/paste.
> Like adding a  my $wrapperType = "V8SVGStaticPODTypeWrapperWithPODTypeParent"
> instead of pulling the "push" statement into every else clause.
> it's OK as is, just probably could be less code written slightly differently.

I left this alone.  Trying it out I didn't manage to make it more readable than
it is now.

> I would have probably written all of the:
>  623             $resultObject = $resultObject . ".get()";
>  633         $result = $result . ".release()" if (IsRefPtrType($attrType));
>  1556         $return = $return . ".release()" if (IsRefPtrType($returnType));
>
> As:
> $return .= ".release()"
> Initially I thought you were assigning the refptrs to themselves in the c++
> code. ;)

Thanks, done!

> These can all be a template, like the JSC bindings:
>   static v8::Handle<v8::Value> convertNodeToV8Object(PassRefPtr<Node> node)
>  145         {
>  146             return convertNodeToV8Object(node.get());
>  147         }
>  148 
> 
> see toJS PassRefPtr<T>:
> http://trac.webkit.org/browser/trunk/WebCore/bindings/js/JSDOMBinding.h#L253

I see what you mean.  That would be a larger cleanup because we use no
overloading for this currently and I'm unsure if we could get bitten by
changing it.  I'd like to leave that for a separate cleanup.

> In this case there is no need for the intermediary RefPtr:
> RefPtr<HTMLCollection> collection = htmlDocument->all();
>  184     return V8DOMWrapper::convertToV8Object(V8ClassIndex::HTMLCOLLECTION,
> collection.release());
> unless you're trying to shorten then line...

Thanks. done.

> Same here:
>   RefPtr<V8SVGStaticPODTypeWrapper<TransformationMatrix> > wrapper =
> V8SVGStaticPODTypeWrapper<TransformationMatrix>::create(result);
>  62     return V8DOMWrapper::convertToV8Object(V8ClassIndex::SVGMATRIX,
> wrapper.release());

Thanks, done.

I'm running layout tests now to make sure that there are no issues with my
latest patch.  I'll update the patch later today.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.