[webkit-reviews] review denied: [Bug 65778] [WebWorkers][chromium] Make statics thread-safe and make sure V8 API accesses correct isolates : [Attachment 103102] Release compilation fixed

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Dmitry Lomov <dslomov at google.com>'s
request for review:
Bug 65778: [WebWorkers][chromium] Make statics thread-safe and make sure V8 API
accesses correct isolates
https://bugs.webkit.org/show_bug.cgi?id=65778

Attachment 103102: Release compilation fixed
https://bugs.webkit.org/attachment.cgi?id=103102&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103102&action=review


Just a few things to clear/clean up.

> Source/WebCore/ChangeLog:11
> +	  
(WebCore::V8BindingPerIsolateData::lazyEventListenerToStringTemplate):

Why are all of these items being moved?

Ideally they would be moved in one change and if any changes were needed, then
that would happen in another change.

> Source/WebCore/bindings/v8/V8Binding.h:176
> +	   inline AllowAllocation()

Isn't inline redundant here (since you put the function body in the class
definition)?

> Source/WebCore/bindings/v8/V8Binding.h:183
> +	   inline ~AllowAllocation()

Ditto.

> Source/WebCore/bindings/v8/V8Binding.h:199
> +	 static inline v8::Local<v8::Object>
newInstance(v8::Handle<v8::Function>, int argc, v8::Handle<v8::Value> argv[]);

The indentation is following Chromium style as opposed to WebKit style.

> Source/WebCore/bindings/v8/V8LazyEventListener.cpp:143
>		   toStringTemplate =
v8::Persistent<v8::FunctionTemplate>::New(v8::FunctionTemplate::New(V8LazyEvent
ListenerToString));

I wonder why this isn't part of the method lazyEventListenerToStringTemplate.

> Source/WebKit/chromium/ChangeLog:8
> +	   * src/BoundObject.cpp:Headers

Would be nice to explain why this change was needed. (Now I see why bur it is
still nice to add the info here to make it quicker for people to understand.)