Bug 42588

Summary: Web Inspector: CodeGeneratorInspector should be able to generate Backend part of Inspector interface.
Product: WebKit Reporter: Ilya Tikhonovsky <loislo>
Component: Web Inspector (Deprecated)Assignee: Ilya Tikhonovsky <loislo>
Status: RESOLVED FIXED    
Severity: Normal CC: bweinstein, eric, joepeck, keishi, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[patch] initial version.
none
sample: InspectorBackendDispatcher.cpp
none
sample: InspectorBackendDispatcher.h
none
[patch] second iteration
none
[patch] third iteration.
yurys: review+
really landed none

Ilya Tikhonovsky
Reported 2010-07-19 12:59:10 PDT
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.
Attachments
[patch] initial version. (25.69 KB, patch)
2010-07-22 02:44 PDT, Ilya Tikhonovsky
no flags
sample: InspectorBackendDispatcher.cpp (34.84 KB, application/octet-stream)
2010-07-22 02:56 PDT, Ilya Tikhonovsky
no flags
sample: InspectorBackendDispatcher.h (5.43 KB, application/octet-stream)
2010-07-22 02:56 PDT, Ilya Tikhonovsky
no flags
[patch] second iteration (27.49 KB, patch)
2010-07-22 08:12 PDT, Ilya Tikhonovsky
no flags
[patch] third iteration. (27.51 KB, patch)
2010-07-22 08:30 PDT, Ilya Tikhonovsky
yurys: review+
really landed (27.56 KB, patch)
2010-07-23 01:33 PDT, Ilya Tikhonovsky
no flags
Pavel Feldman
Comment 1 2010-07-19 23:05:25 PDT
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.
Joseph Pecoraro
Comment 2 2010-07-20 09:45:20 PDT
(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.
Ilya Tikhonovsky
Comment 3 2010-07-22 02:44:56 PDT
Created attachment 62278 [details] [patch] initial version.
Ilya Tikhonovsky
Comment 4 2010-07-22 02:56:02 PDT
Created attachment 62279 [details] sample: InspectorBackendDispatcher.cpp
Ilya Tikhonovsky
Comment 5 2010-07-22 02:56:39 PDT
Created attachment 62280 [details] sample: InspectorBackendDispatcher.h
Yury Semikhatsky
Comment 6 2010-07-22 03:15:44 PDT
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.
Yury Semikhatsky
Comment 7 2010-07-22 04:31:52 PDT
All (In reply to comment #5) > Created an attachment (id=62280) [details] > sample: InspectorBackendDispatcher.h All methods but constructor and dispatch should be private.
Ilya Tikhonovsky
Comment 8 2010-07-22 08:12:19 PDT
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
Eric Seidel (no email)
Comment 9 2010-07-22 08:20:47 PDT
Ilya Tikhonovsky
Comment 10 2010-07-22 08:30:21 PDT
Created attachment 62301 [details] [patch] third iteration. x64 compile error was fixed.
Ilya Tikhonovsky
Comment 11 2010-07-22 08:36:28 PDT
*** Bug 40167 has been marked as a duplicate of this bug. ***
Joseph Pecoraro
Comment 12 2010-07-22 12:10:09 PDT
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.
Ilya Tikhonovsky
Comment 13 2010-07-23 01:10:30 PDT
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)
Ilya Tikhonovsky
Comment 14 2010-07-23 01:33:30 PDT
Created attachment 62394 [details] really landed
Note You need to log in before you can comment on or make changes to this bug.