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
Ilya Tikhonovsky
2010-07-19 12:59:10 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. (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
|