Bug 39302 – [Chromium] WebDevToolsAgentClient::sendMessageToFrontendOnIOThread needs to be implemented as virtual function

• Component: WebKit API
• Status: RESOLVED FIXED

[Victor Wang]

Here is IORPCDelegate code in WebDevToolsAgentImpl.cpp:

class IORPCDelegate : public DevToolsRPC::Delegate, public Noncopyable {
public:
    IORPCDelegate() { }
    virtual ~IORPCDelegate() { }
    virtual void sendRpcMessage(const WebDevToolsMessageData& data)
    {
        WebDevToolsAgentClient::sendMessageToFrontendOnIOThread(data);
    }
};

WebDevToolsAgentClient::sendMessageToFrontendOnIOThread is implemented in chromium code base (devtools_agent.cc). This prevents us from building webkit as a dll. We should change sendMessageToFrontendOnIOThread to be a virtual function and access it through WebDevToolsAgentClient object.

[Yury Semikhatsky]

Mikhail, could you look into this?

[Mikhail Naganov]

Yes, will do.

[Victor Wang]

Hi Mikhail,
Any update on this? I am splitting webkit to be a dll for chromium dev build, this issue needs to be fixed in order to move forward. Thanks!

(In reply to comment #2)
> Yes, will do.

[Mikhail Naganov]

Victor, I'm sorry for holding you! I already tried once to fix it, but discovered that it's not trivial, as the call path goes all the way through static methods (that's why in the current state of affairs it is natural for the method to be static). Then I was distracted, etc. I will try harder, I promise!

[Mikhail Naganov]

Created attachment 58812 [details]
proposed patch

Below is the corresponding Chromium change. After applying it, a cleanup change to WebKit will be required.
Also, after applying the WebKit patch, Heap snapshots will stop working until Chromium change will be applied.
I see no other good way of dealing with it, as sendMessageToFrontendOnIOThread is declared and used in
WebKit, but defined in Chromium. Thus, to remove it, we first need to remove its usage (WebKit), then
definition (Chromium), then declaration (WebKit again).

diff --git a/chrome/renderer/devtools_agent.cc b/chrome/renderer/devtools_agent.cc
index 5adfa09..42f32e2 100644
--- a/chrome/renderer/devtools_agent.cc
+++ b/chrome/renderer/devtools_agent.cc
@@ -57,12 +57,14 @@ DevToolsAgent::DevToolsAgent(int routing_id, RenderView* render_view)
     : routing_id_(routing_id),
       render_view_(render_view) {
   agent_for_routing_id_[routing_id] = this;
+  DevToolsAgentFilter::SetDevToolsAgent(this);
 
   CommandLine* cmd = CommandLine::ForCurrentProcess();
   expose_v8_debugger_protocol_ =cmd->HasSwitch(switches::kRemoteShellPort);
 }
 
 DevToolsAgent::~DevToolsAgent() {
+  DevToolsAgentFilter::SetDevToolsAgent(NULL);
   agent_for_routing_id_.erase(routing_id_);
 }
 
@@ -96,6 +98,13 @@ void DevToolsAgent::sendMessageToFrontend(
   render_view_->Send(m);
 }
 
+
+void DevToolsAgent::sendMessageToFrontendFromIOThread(
+    const WebKit::WebDevToolsMessageData& data) {
+  DevToolsAgentFilter::SendRpcMessage(DevToolsMessageData(data));
+}
+
+
 int DevToolsAgent::hostIdentifier() {
   return routing_id_;
 }
@@ -195,9 +204,3 @@ WebDevToolsAgent* DevToolsAgent::GetWebAgent() {
     return NULL;
   return web_view->devToolsAgent();
 }
-
-// static
-void WebKit::WebDevToolsAgentClient::sendMessageToFrontendOnIOThread(
-    const WebDevToolsMessageData& data) {
-  DevToolsAgentFilter::SendRpcMessage(DevToolsMessageData(data));
-}
diff --git a/chrome/renderer/devtools_agent.h b/chrome/renderer/devtools_agent.h
index 23fa10c..f1262d0 100644
--- a/chrome/renderer/devtools_agent.h
+++ b/chrome/renderer/devtools_agent.h
@@ -40,6 +40,8 @@ class DevToolsAgent : public WebKit::WebDevToolsAgentClient {
   // WebDevToolsAgentClient implementation
   virtual void sendMessageToFrontend(
       const WebKit::WebDevToolsMessageData& data);
+  virtual void sendMessageToFrontendFromIOThread(
+      const WebKit::WebDevToolsMessageData& data);
 
   virtual int hostIdentifier();
   virtual void forceRepaint();
diff --git a/chrome/renderer/devtools_agent_filter.cc b/chrome/renderer/devtools_agent_filter.cc
index 5eb54ed..01c1be0 100644
--- a/chrome/renderer/devtools_agent_filter.cc
+++ b/chrome/renderer/devtools_agent_filter.cc
@@ -28,6 +28,8 @@ void DevToolsAgentFilter::DispatchMessageLoop() {
 }
 
 // static
+DevToolsAgent* DevToolsAgentFilter::agent_ = NULL;
+// static
 IPC::Channel* DevToolsAgentFilter::channel_ = NULL;
 // static
 int DevToolsAgentFilter::current_routing_id_ = 0;
@@ -65,8 +67,13 @@ void DevToolsAgentFilter::OnDebuggerPauseScript() {
 }
 
 void DevToolsAgentFilter::OnRpcMessage(const DevToolsMessageData& data) {
-  message_handled_ = WebDevToolsAgent::dispatchMessageFromFrontendOnIOThread(
-      data.ToWebDevToolsMessageData());
+  if (agent_ != NULL) {
+    message_handled_ = WebDevToolsAgent::dispatchMessageFromFrontendOnIOThread(
+        agent_,
+        data.ToWebDevToolsMessageData());
+  } else {
+    message_handled_ = false;
+  }
 }
 
 // static
diff --git a/chrome/renderer/devtools_agent_filter.h b/chrome/renderer/devtools_agent_filter.h
index 8d23918..caae82b 100644
--- a/chrome/renderer/devtools_agent_filter.h
+++ b/chrome/renderer/devtools_agent_filter.h
@@ -10,6 +10,7 @@
 
 #include "ipc/ipc_channel_proxy.h"
 
+class DevToolsAgent;
 struct DevToolsMessageData;
 
 // DevToolsAgentFilter is registered as an IPC filter in order to be able to
@@ -25,6 +26,7 @@ class DevToolsAgentFilter : public IPC::ChannelProxy::MessageFilter {
   virtual ~DevToolsAgentFilter();
 
   static void SendRpcMessage(const DevToolsMessageData& data);
+  static void SetDevToolsAgent(DevToolsAgent* agent) { agent_ = agent; }
 
  private:
   // IPC::ChannelProxy::MessageFilter override. Called on IO thread.
@@ -46,6 +48,7 @@ class DevToolsAgentFilter : public IPC::ChannelProxy::MessageFilter {
   // from IO thread.
   static int current_routing_id_;
   static IPC::Channel* channel_;
+  static DevToolsAgent* agent_;
 
   DISALLOW_COPY_AND_ASSIGN(DevToolsAgentFilter);
 };

[WebKit Review Bot]

Attachment 58812 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebDevToolsAgentImpl.cpp:126:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mikhail Naganov]

Created attachment 58859 [details]
fixed style error

[Mikhail Naganov]

Created attachment 58868 [details]
as we discussed offline

Chromium change is now smaller:


diff --git a/chrome/renderer/devtools_agent.cc b/chrome/renderer/devtools_agent.cc
index 5adfa09..01c66e4 100644
--- a/chrome/renderer/devtools_agent.cc
+++ b/chrome/renderer/devtools_agent.cc
@@ -195,9 +195,3 @@ WebDevToolsAgent* DevToolsAgent::GetWebAgent() {
     return NULL;
   return web_view->devToolsAgent();
 }
-
-// static
-void WebKit::WebDevToolsAgentClient::sendMessageToFrontendOnIOThread(
-    const WebDevToolsMessageData& data) {
-  DevToolsAgentFilter::SendRpcMessage(DevToolsMessageData(data));
-}
diff --git a/chrome/renderer/devtools_agent_filter.cc b/chrome/renderer/devtools_agent_filter.cc
index 5eb54ed..4c98cbe 100644
--- a/chrome/renderer/devtools_agent_filter.cc
+++ b/chrome/renderer/devtools_agent_filter.cc
@@ -64,8 +64,21 @@ void DevToolsAgentFilter::OnDebuggerPauseScript() {
   WebDevToolsAgent::debuggerPauseScript();
 }
 
+namespace {
+
+class WebDevToolsMessageTransportImpl : public WebKit::WebDevToolsMessageTransport {
+ public:
+  void sendMessageToFrontendOnIOThread(const WebKit::WebDevToolsMessageData& data) {
+    DevToolsAgentFilter::SendRpcMessage(DevToolsMessageData(data));
+  }
+};
+
+}  // namespace
+
 void DevToolsAgentFilter::OnRpcMessage(const DevToolsMessageData& data) {
+  WebDevToolsMessageTransportImpl transport;
   message_handled_ = WebDevToolsAgent::dispatchMessageFromFrontendOnIOThread(
+      &transport,
       data.ToWebDevToolsMessageData());
 }

[Mikhail Naganov]

Created attachment 58880 [details]
will not break functionality, but requires 4 steps to commit

Chromium part:


diff --git a/chrome/renderer/devtools_agent_filter.cc b/chrome/renderer/devtools_agent_filter.cc
index 5eb54ed..4c98cbe 100644
--- a/chrome/renderer/devtools_agent_filter.cc
+++ b/chrome/renderer/devtools_agent_filter.cc
@@ -64,8 +64,21 @@ void DevToolsAgentFilter::OnDebuggerPauseScript() {
   WebDevToolsAgent::debuggerPauseScript();
 }
 
+namespace {
+
+class WebDevToolsMessageTransportImpl : public WebKit::WebDevToolsMessageTransport {
+ public:
+  void sendMessageToFrontendOnIOThread(const WebKit::WebDevToolsMessageData& data) {
+    DevToolsAgentFilter::SendRpcMessage(DevToolsMessageData(data));
+  }
+};
+
+}  // namespace
+
 void DevToolsAgentFilter::OnRpcMessage(const DevToolsMessageData& data) {
+  WebDevToolsMessageTransportImpl transport;
   message_handled_ = WebDevToolsAgent::dispatchMessageFromFrontendOnIOThread(
+      &transport,
       data.ToWebDevToolsMessageData());
 }

[Darin Fisher (:fishd, Google)]

Comment on attachment 58880 [details]
will not break functionality, but requires 4 steps to commit

WebKit/chromium/public/WebDevToolsAgentClient.h:74
 +  class WebDevToolsMessageTransport {
one file per class please.

[Mikhail Naganov]

OK, will fix this in the next commit.

I wanted to make this class an inner of WebDevToolsAgentClient, but forgot that C++ doesn't allow forward declarations for inner classes (except inside its containing class).

[Yury Semikhatsky]

Comment on attachment 58880 [details]
will not break functionality, but requires 4 steps to commit

WebKit/chromium/public/WebDevToolsAgentClient.h:75
 +    public:
style: wrong indentation

[Mikhail Naganov]

Fixed Yury's style comment.

Manually committed http://trac.webkit.org/changeset/61320

2010-06-17  Mikhail Naganov  <[e-mail]>

        Reviewed by Pavel Feldman.

        [Chromium] Prepare to making WebDevToolsAgentClient::sendMessageToFrontendOnIOThread virtual

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

        * public/WebDevToolsAgent.h:
        * public/WebDevToolsAgentClient.h:
        (WebKit::WebDevToolsMessageTransport::~WebDevToolsMessageTransport):
        * src/WebDevToolsAgentImpl.cpp:
        (WebKit::):
        (WebKit::WebDevToolsAgent::dispatchMessageFromFrontendOnIOThread):