Bug 27421 - [V8] Factor V8ConsoleMessage out of V8Proxy
Summary: [V8] Factor V8ConsoleMessage out of V8Proxy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Evangelism (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-18 21:35 PDT by Adam Barth
Modified: 2009-07-19 20:56 PDT (History)
3 users (show)

See Also:


Attachments
patch (19.38 KB, patch)
2009-07-18 21:36 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
patch (19.38 KB, patch)
2009-07-18 21:42 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
patch (19.38 KB, patch)
2009-07-18 23:13 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
patch (19.38 KB, patch)
2009-07-19 19:56 PDT, Adam Barth
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2009-07-18 21:35:03 PDT
patch forthcoming
Comment 1 Adam Barth 2009-07-18 21:36:35 PDT
Created attachment 33034 [details]
patch
Comment 2 Adam Barth 2009-07-18 21:42:18 PDT
Created attachment 33035 [details]
patch
Comment 3 Adam Barth 2009-07-18 23:13:28 PDT
Created attachment 33036 [details]
patch
Comment 4 David Levin 2009-07-19 19:24:43 PDT
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"
Comment 5 Adam Barth 2009-07-19 19:55:22 PDT
> > 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.
Comment 6 Adam Barth 2009-07-19 19:56:06 PDT
Created attachment 33070 [details]
patch
Comment 7 David Levin 2009-07-19 20:26:05 PDT
> 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.
Comment 8 Adam Barth 2009-07-19 20:56:40 PDT
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