Bug 27421 – [V8] Factor V8ConsoleMessage out of V8Proxy

• Status: RESOLVED
• Resolution: FIXED
• Component: Evangelism

[Adam Barth]

patch forthcoming

[Adam Barth]

Created attachment 33034 [details]
patch

[Adam Barth]

Created attachment 33035 [details]
patch

[Adam Barth]

Created attachment 33036 [details]
patch

[David Levin]

Comment on attachment 33036 [details]
patch

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"

[Adam Barth]

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

I think this stuff is all fine.  The only one we have to worry about is |dispatchLater|.  We're not supposed to call JavaScript while running that function.  Both dispatchNow and processDelayed are supposed to be called when it's safe to do allocations.

> > + }  // namespace WebCore
> 
> Extra space before the // here.

Fixed.

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

Fixed.

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

Fixed.

> > + }
> 
> Change to "} // namespace WebCore"

Fixed.

[Adam Barth]

Created attachment 33070 [details]
patch

[David Levin]

> I think this stuff is all fine. 

But couldn't it lead to console messages getting display in a (wrong and) confusing order. 

Message 1 and Message 2 are in the queue.
Message 3 gets created and message3.dispatchNow() is called.

Message 1 gets dispatched and results in js being executed which posts another message (Message 4  -- I'm not sure if the js can do something which causes another message to get posted, but it seems like any messages happening here should get posted.)

Message 2 is about to get dispatched but first processes messages in the queue, so Message 4 goes to the console.

End result:

Message 1
Message 4
Message 2
Message 3

Actually, it looks like this can't happen currently. Message 4 would actually happen immediately without getting posted, and the new code is more robust to possible badness by taking ownership of the pointer immediately and setting it to 0.

[Adam Barth]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/WebCore.gypi
	A	WebCore/bindings/v8/V8ConsoleMessage.cpp
	A	WebCore/bindings/v8/V8ConsoleMessage.h
	M	WebCore/bindings/v8/V8Proxy.cpp
Committed r46106
	M	WebCore/ChangeLog
	M	WebCore/WebCore.gypi
	A	WebCore/bindings/v8/V8ConsoleMessage.cpp
	M	WebCore/bindings/v8/V8Proxy.cpp
	A	WebCore/bindings/v8/V8ConsoleMessage.h
r46106 = d6c8a595074589399f53fa597a1a895bbab8b29d (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46106