Bug 39302 - [Chromium] WebDevToolsAgentClient::sendMessageToFrontendOnIOThread needs to be implemented as virtual function
Summary: [Chromium] WebDevToolsAgentClient::sendMessageToFrontendOnIOThread needs to b...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Mikhail Naganov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-18 11:22 PDT by Victor Wang
Modified: 2010-06-17 05:27 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (3.41 KB, patch)
2010-06-15 13:29 PDT, Mikhail Naganov
pfeldman: review-
Details | Formatted Diff | Diff
fixed style error (3.40 KB, patch)
2010-06-16 01:20 PDT, Mikhail Naganov
no flags Details | Formatted Diff | Diff
as we discussed offline (3.55 KB, patch)
2010-06-16 03:19 PDT, Mikhail Naganov
no flags Details | Formatted Diff | Diff
will not break functionality, but requires 4 steps to commit (3.64 KB, patch)
2010-06-16 04:52 PDT, Mikhail Naganov
pfeldman: review+
mnaganov: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Wang 2010-05-18 11:22:29 PDT
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.
Comment 1 Yury Semikhatsky 2010-05-19 00:28:10 PDT
Mikhail, could you look into this?
Comment 2 Mikhail Naganov 2010-05-19 08:58:34 PDT
Yes, will do.
Comment 3 Victor Wang 2010-06-14 15:46:07 PDT
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.
Comment 4 Mikhail Naganov 2010-06-15 01:45:38 PDT
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!
Comment 5 Mikhail Naganov 2010-06-15 13:29:02 PDT
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);
 };
Comment 6 WebKit Review Bot 2010-06-15 13:31:43 PDT
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.
Comment 7 Mikhail Naganov 2010-06-16 01:20:52 PDT
Created attachment 58859 [details]
fixed style error
Comment 8 Mikhail Naganov 2010-06-16 03:19:23 PDT
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());
 }
Comment 9 Mikhail Naganov 2010-06-16 04:52:28 PDT
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());
 }
Comment 10 Darin Fisher (:fishd, Google) 2010-06-16 10:41:38 PDT
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.
Comment 11 Mikhail Naganov 2010-06-16 12:17:33 PDT
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).
Comment 12 Yury Semikhatsky 2010-06-17 01:52:13 PDT
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
Comment 13 Mikhail Naganov 2010-06-17 05:27:58 PDT
Fixed Yury's style comment.

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

2010-06-17  Mikhail Naganov  <mnaganov@chromium.org>

        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):