patch forthcoming
Created attachment 33034 [details] patch
Created attachment 33035 [details] patch
Created attachment 33036 [details] patch
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"
> > 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.
Created attachment 33070 [details] patch
> 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.
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