[Webkit-unassigned] [Bug 27421] [V8] Factor V8ConsoleMessage out of V8Proxy

[bugzilla-daemon at webkit.org]

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


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33036|review?                     |review-
               Flag|                            |




--- Comment #4 from David Levin <levin at chromium.org>  2009-07-19 19:24:43 PDT ---
(From update of attachment 33036)
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"

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