[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 #9 from Eric Seidel <eric at webkit.org>  2009-07-23 09:16:41 PDT ---
(From update of attachment 33327)
I totally agree.  The PODType unification stuff can be a separate change.  Had
I been paying more attention long ago, I would not have let them diverge in the
first place. ;)

Should this $wrapper use my?

                 $wrapper =
"V8SVGStaticPODTypeWrapperWithPODTypeParent<$nativeType,
$implClassName>::create($getterString, imp_wrapper)";
 589                 push(@implContentDecls, "   
RefPtr<V8SVGStaticPODTypeWrapperWithPODTypeParent<$nativeType, $implClassName>
> wrapper = $wrapper;\n");

You certainly don't want $wrapper used again outside that block, or you'd end
up with two calls to create...

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


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

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

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

This change looks fine as is.  There are some nits above which you could fix.

Since you're not a committer, I'm going to r- this to ask for one more round.
If you were, I could r+ this and you could fix any nits you deemed appropriate
as you went to commit.

Thannks for this great cleanup!  Eventually I would like to re-write our
autogen system to not be so horribly ugly (aka not perl)... but that's a ways
off yet.

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