[DevTools] ConsoleMessage storage moved from ConsoleAgent

Source/core/frame/FrameConsole.cpp

Index: Source/core/frame/FrameConsole.cpp
diff --git a/Source/core/frame/FrameConsole.cpp b/Source/core/frame/FrameConsole.cpp
index ba05c78164929afe0b995721fbb353325abaae43..b53f7338d85ac247a715f9231adc0baa4c6880ae 100644
--- a/Source/core/frame/FrameConsole.cpp
+++ b/Source/core/frame/FrameConsole.cpp
@@ -66,14 +66,9 @@ void FrameConsole::addMessage(PassRefPtrWillBeRawPtr<ConsoleMessage> prpConsoleM
     if (!context)
         return;
 
-    InspectorInstrumentation::addMessageToConsole(context, consoleMessage.get());
-
-    if (consoleMessage->source() == CSSMessageSource)
-        return;
-
     String messageURL;
     unsigned lineNumber = 0;
-    if (consoleMessage->callStack()) {
+    if (consoleMessage->callStack() && consoleMessage->callStack()->size()) {
         lineNumber = consoleMessage->callStack()->at(0).lineNumber();
         messageURL = consoleMessage->callStack()->at(0).sourceURL();
     } else {
@@ -81,6 +76,11 @@ void FrameConsole::addMessage(PassRefPtrWillBeRawPtr<ConsoleMessage> prpConsoleM
         messageURL = consoleMessage->url();
     }
 
+    messageStorage()->reportMessage(consoleMessage);

[aandrey, 2014/08/26 08:56:42]

why this moved after the if- check?

[kozyatinskiy1, 2014/08/26 09:39:29]

Before this patch, in instrumentation call addMessageToConsole we call
autogenerateMetadata in new instance of InspectorConsoleMessage and don't change
lineNumber and url of ConsoleMessage object. Now, we need to save lineNumber and
messageURL for sending to client, otherwise we changes these values ​​in the
reportMessage call.

[aandrey, 2014/08/26 10:12:27]

The fact that reportMessage(consoleMessage) has side effects of changing
lineNumber and url is very error prone. One may well break something in future
by moving this code back and forth. Can we avoid side effects in reportMessage?

[vsevik, 2014/08/26 11:44:39]

The data that is stored in console message has the best possible quality after
the reportMessage call, so using it is always good, it is not an error.
The only reason we still have the code above is because we are afraid that the
too many test expectations might change if we remove it and we want to check
them carefully (in a separate patch).
Alexey, would you mind checking if we actually need the if-else statement above?

[kozyatinskiy1, 2014/08/26 12:37:50]

Yes, we have 26 devtools tests and I think a lot of other tests that depend on
this. I can remove this if-else in a separate patch.

+
+    if (consoleMessage->source() == CSSMessageSource)
+        return;
+
     RefPtrWillBeRawPtr<ScriptCallStack> reportedCallStack = nullptr;
     if (consoleMessage->source() != ConsoleAPIMessageSource) {
         if (consoleMessage->callStack() && m_frame.chromeClient().shouldReportDetailedMessageForSource(messageURL))
@@ -132,6 +132,17 @@ void FrameConsole::unmute()
     muteCount--;
 }
 
+ConsoleMessageStorage* FrameConsole::messageStorage()
+{
+    LocalFrame* curFrame = &m_frame;
+    LocalFrame* localTopFrame = toLocalFrame(curFrame->tree().top());

[yurys, 2014/08/26 07:38:55]

What if top is not a local frame, should we check it here?

+    if (localTopFrame != curFrame)
+        return localTopFrame->console().messageStorage();
+    if (!m_consoleMessageStorage.get())

[yurys, 2014/08/26 07:38:56]

Maybe ASSERT(localTopFrame) ?

[aandrey, 2014/08/26 08:56:42]

if (!m_consoleMessageStorage.get()) -> if (!m_consoleMessageStorage)

[kozyatinskiy1, 2014/08/26 09:39:29]

Done.

[kozyatinskiy1, 2014/08/26 09:39:29]

Done.

+        m_consoleMessageStorage = ConsoleMessageStorage::create(m_frame.document(), &m_frame);
+    return m_consoleMessageStorage.get();
+}
+
 void FrameConsole::adoptWorkerConsoleMessages(WorkerGlobalScopeProxy* proxy)
 {
     InspectorInstrumentation::adoptWorkerConsoleMessages(m_frame.document(), proxy);