[webkit-reviews] review denied: [Bug 28156] [V8] Fix memory leak in node event listeners. See http://crbug.com/17400. : [Attachment 34482] patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Vitaly Repeshko
<vitalyr at chromium.org>'s request for review:
Bug 28156: [V8] Fix memory leak in node event listeners. See
http://crbug.com/17400.
https://bugs.webkit.org/show_bug.cgi?id=28156

Attachment 34482: patch
https://bugs.webkit.org/attachment.cgi?id=34482&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
Just a few style nits, LG overall:

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index ded636d..0b05faa 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,32 @@
> +2009-08-10  Vitaly Repeshko	<vitalyr at quad.spb.corp.google.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   [V8] Fix memory leak in node event listeners. See
http://crbug.com/17400.
> +
> +	https://bugs.webkit.org/show_bug.cgi?id=28156

Indent w/same spacing as summary, no need for an extra line break between URL
and summary.

> +	   * bindings/scripts/CodeGeneratorV8.pm:
> +	   * bindings/v8/V8AbstractEventListener.h:
> +	   (WebCore::V8AbstractEventListener::isObjectListener):
> +	   * bindings/v8/V8DOMWrapper.cpp:
> +	   (WebCore::V8DOMWrapper::getTemplate):
> +	   * bindings/v8/V8ObjectEventListener.cpp:
> +	   * bindings/v8/V8ObjectEventListener.h:
> +	   (WebCore::V8ObjectEventListener::isObjectListener):
> +	   * bindings/v8/V8Utilities.cpp:
> +	   (WebCore::removeHiddenDependency):
> +	   * bindings/v8/custom/V8CustomBinding.h:
> +	   * bindings/v8/custom/V8ElementCustom.cpp:
> +	   * bindings/v8/custom/V8NodeCustom.cpp:
> +	   (WebCore::toEventType):
> +	   (WebCore::getEventListener):
> +	   (WebCore::ACCESSOR_SETTER):
> +	   (WebCore::ACCESSOR_GETTER):
> +	   (WebCore::CALLBACK_FUNC_DECL):
> +	   * bindings/v8/custom/V8XMLHttpRequestCustom.cpp:
> +	   (WebCore::getEventListener):

Please provide brief explanation of what changes were made for each method (or
file, if the methods aren't listed). For example:

* bindings/scripts/CodeGeneratorV8.pm: Changed event handler to live at Node,
rather than Element.

> +END
> +    if (IsNodeSubType($dataNode)) {
> +	   push(@implContent, <<END);
> +  instance->SetInternalFieldCount(V8Custom::kNodeMinimumInternalFieldCount);


Is spacing correct here?

> +END
> +    } else {
> +	   push(@implContent, <<END);
> + 
instance->SetInternalFieldCount(V8Custom::kDefaultWrapperInternalFieldCount);

Ditto.

> +    return PassRefPtr<EventListener>();

return 0;