After big discussion with Pavel it was decided to keep original InspectorBackend functionality as is, and generate separate class RemoteInspectorBackend for parsing transfered arguments and pass these arguments to the current implementation of InspectorBackend. This implementation will be an adoptation layer between Remote Debugging interface, which should be stable for customers, and flexible internal implementation of that interface.
The name RemoteInspectorBackend sounds confusing to me. How can backend be remote wrt WebCore. I'd call the new class InspectorBackendDispatcher. It might be created with backend instance in the constructor and have single dispatch(String) method.
(In reply to comment #1) > The name RemoteInspectorBackend sounds confusing to me. How can backend be > remote wrt WebCore. I'd call the new class InspectorBackendDispatcher. It might > be created with backend instance in the constructor and have single > dispatch(String) method. +1, I like that.
Created attachment 62278 [details] [patch] initial version.
Created attachment 62279 [details] sample: InspectorBackendDispatcher.cpp
Created attachment 62280 [details] sample: InspectorBackendDispatcher.h
Comment on attachment 62278 [details] [patch] initial version. WebCore/GNUmakefile.am:553 + DerivedSources/WebCore/InspectorBackendDispatcher.cpp \ mind alphabetic order WebCore/WebCore.gyp/WebCore.gyp:481 + '../inspector/CodeGeneratorInspector.pm', Could you add a comment for this? WebCore/WebCore.gypi: + 'webcore_inspector_idl_files': [ is it referenced from other places? WebCore/inspector/CodeGeneratorInspector.pm:168 + push(@backendHead, " ${backendClassName}* create(InspectorBackend* backend) { return new ${backendClassName}(backend); }"); either remote create or make the constructor private WebCore/inspector/CodeGeneratorInspector.pm:241 + push(@function, "void ${backendClassName}::${functionName}(PassRefPtr<InspectorArray>" . ( scalar(@argsFiltered) ? " args)" : ")")); Please add comment about "unused argument" warning/ WebCore/inspector/CodeGeneratorInspector.pm:248 + $type = "double"; can you take it from the type description? WebCore/inspector/CodeGeneratorInspector.pm:271 + my @mapEntries = map("dispatchMap.add(String(\"$_\"), &${backendClassName}::$_);", @methods); Is explicit check really needed here? WebCore/inspector/CodeGeneratorInspector.pm:271 + my @mapEntries = map("dispatchMap.add(String(\"$_\"), &${backendClassName}::$_);", @methods); I think calling .set( would be more appropriate here. WebCore/inspector/front-end/ElementsPanel.js:225 + InspectorBackend.pushNodeByPathToFrontend(callId, this._selectedPathOnReset.join(",")); We should eventually pass an array instead of string here. WebKit/chromium/src/js/InspectorControllerImpl.js:157 + args.unshift(methodName); just pass 2 as the second parameter above.
All (In reply to comment #5) > Created an attachment (id=62280) [details] > sample: InspectorBackendDispatcher.h All methods but constructor and dispatch should be private.
Comment on attachment 62278 [details] [patch] initial version. WebCore/GNUmakefile.am:553 + DerivedSources/WebCore/InspectorBackendDispatcher.cpp \ mind alphabetic order fixed. WebCore/WebCore.gyp/WebCore.gyp:481 + '../inspector/CodeGeneratorInspector.pm', Could you add a comment for this? done. WebCore/WebCore.gypi: + 'webcore_inspector_idl_files': [ is it referenced from other places? no other references. WebCore/inspector/CodeGeneratorInspector.pm:168 + push(@backendHead, " ${backendClassName}* create(InspectorBackend* backend) { return new ${backendClassName}(backend); }"); either remote create or make the constructor private removed. WebCore/inspector/CodeGeneratorInspector.pm:241 + push(@function, "void ${backendClassName}::${functionName}(PassRefPtr<InspectorArray>" . ( scalar(@argsFiltered) ? " args)" : ")")); Please add comment about "unused argument" warning/ done. WebCore/inspector/CodeGeneratorInspector.pm:248 + $type = "double"; can you take it from the type description? fixed. WebCore/inspector/CodeGeneratorInspector.pm:271 + my @mapEntries = map("dispatchMap.add(String(\"$_\"), &${backendClassName}::$_);", @methods); Is explicit check really needed here? removed. WebCore/inspector/CodeGeneratorInspector.pm:271 + my @mapEntries = map("dispatchMap.add(String(\"$_\"), &${backendClassName}::$_);", @methods); I think calling .set( would be more appropriate here. all these entries are assigning at the first run. WebCore/inspector/front-end/ElementsPanel.js:225 + InspectorBackend.pushNodeByPathToFrontend(callId, this._selectedPathOnReset.join(",")); We should eventually pass an array instead of string here. In generally it is possible but other platforms are not migrated yet. > All methods but constructor and dispatch should be private. done
Attachment 62298 [details] did not build on mac: Build output: http://queues.webkit.org/results/3577349
Created attachment 62301 [details] [patch] third iteration. x64 compile error was fixed.
*** Bug 40167 has been marked as a duplicate of this bug. ***
Comment on attachment 62301 [details] [patch] third iteration. Excellent! This is just what I thought it would look like after talking with Pavel. You did it very well, with nice error messages. My usual minor comments below. > WebCore/WebCore.gyp/WebCore.gyp > 479 # The second input item will be used as item name in vcproj. > 480 # It is not possible to put there Inspector.idl because > 481 # all idl files are marking as excluded by gyp generator. This could be reworded. "It is not possible to put Inspector.idl there". > InspectorBackend* inspectorBackend() { return m_inspectorBackend.get(); } > + InspectorBackendDispatcher* inspectorBackendDispatcher() { return m_inspectorBackendDispatcher.get(); } > InjectedScriptHost* injectedScriptHost() { return m_injectedScriptHost.get(); } These could all use "const". I think I mentioned this before. No big deal. > +++ b/WebCore/inspector/InspectorValues.cpp > + *output = (long)m_doubleValue; > + *output = (unsigned long)m_doubleValue; Should these have used static_cast<...>(...)? > *exception = \"Error: Invalid message format. The message should be jsonified array of arguments.\";"); I would say "The message should be a JSONified array of arguments.", Because JSON was capitalized before, but mostly the "a" in front of JSONified.
Committed r63952 M WebKit/chromium/ChangeLog M WebKit/chromium/src/WebDevToolsAgentImpl.cpp M WebKit/chromium/src/js/InspectorControllerImpl.js M WebKit/chromium/src/WebDevToolsAgentImpl.h M WebKit/chromium/src/ToolsAgent.h M WebCore/WebCore.pri M WebCore/ChangeLog M WebCore/WebCore.vcproj/WebCore.vcproj M WebCore/GNUmakefile.am M WebCore/WebCore.gyp/WebCore.gyp M WebCore/WebCore.gypi M WebCore/inspector/CodeGeneratorInspector.pm M WebCore/inspector/InspectorController.cpp M WebCore/inspector/InspectorValues.h M WebCore/inspector/front-end/ElementsPanel.js M WebCore/inspector/InspectorController.h M WebCore/inspector/InspectorValues.cpp M WebCore/WebCore.xcodeproj/project.pbxproj r63952 = d923b0a72242f6f9ae060d20a3e391f6ed5da461 (refs/remotes/trunk)
Created attachment 62394 [details] really landed