[webkit-reviews] review denied: [Bug 27421] [V8] Factor V8ConsoleMessage out of V8Proxy : [Attachment 33036] patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 27421: [V8] Factor V8ConsoleMessage out of V8Proxy
https://bugs.webkit.org/show_bug.cgi?id=27421

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

------- Additional Comments from David Levin <levin at chromium.org>
This one was a lot more understandable to me.  In general, it looks great. Just
a few nits and but r- for the one concern to address.



> Index: WebCore/bindings/v8/V8ConsoleMessage.cpp
> +    for (int i = 0; i < size; ++i)
> +	   delayedMessages->at(i).dispatchNow(page);

I'm concerned about the call to dispatchNow because it in turn calls
V8ConsoleMessage::processDelayed.

At first, I thought this was ok because no new messages should get added to the
console but then I saw this note about dispatchNow
   48	      // Add a message to the console. May end up calling JavaScript
code
   49	      // indirectly through the inspector so only call this function
when
   50	      // it is safe to do allocations.
so it looks like it can end up running JavaScript (which may result in added
messages?).

If that is true, then it would seem good if this code called
   console->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel,
m_string, m_lineNumber, m_source);
more directly in some manner (without calling processDelayed).


> + }  // namespace WebCore

Extra space before the // here.


> Index: WebCore/bindings/v8/V8ConsoleMessage.h
> +	   static void handler(v8::Handle<v8::Message> message,
v8::Handle<v8::Value> data);

"message" param name looks redundant.

> +	   // All delayed messages are stored in this vector. If the vector
> +	   // is NULL, there are no delayed messages.

Since no NULLs are in the code, it would be good just to make this comment say
"0" as well.

> + }

Change to "} // namespace WebCore"