Summary: | Web Inspector: migrate WebCore from InspectorBackend binding to InspectorBackendDispatcher | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ilya Tikhonovsky <loislo> | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Ilya Tikhonovsky <loislo> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bweinstein, joepeck, keishi, pfeldman, pmuellr, rik, timothy, yurys | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Ilya Tikhonovsky
2010-07-26 11:19:32 PDT
Created attachment 62589 [details]
[patch] initial version.
Comment on attachment 62589 [details] [patch] initial version. > +++ b/WebCore/inspector/Inspector.idl > + [notify] void didDispatch(out long callId, out String e); WebKit typically doesn't abbreviate variable names. "e" should be "exception". The rest of this looked good to me =) (In reply to comment #2) > (From update of attachment 62589 [details]) > > +++ b/WebCore/inspector/Inspector.idl > > + [notify] void didDispatch(out long callId, out String e); > > WebKit typically doesn't abbreviate variable names. > "e" should be "exception". Unfortunately exception is a reserved word of IDLParser. > > The rest of this looked good to me =) Created attachment 62595 [details]
[patch] second iteration. fixed windows build.
> > WebKit typically doesn't abbreviate variable names.
> > "e" should be "exception".
>
> Unfortunately exception is a reserved word of IDLParser.
How about "exceptionMessage" or "exceptionString" than? =)
Comment on attachment 62595 [details]
[patch] second iteration. fixed windows build.
WebCore/inspector/Inspector.idl:42
+ [notify] void didDispatch(out long callId, out String e);
Also, we should not have a generic ack / failure reporting. Each of the backend methods should ack (either with the results or with failure reporting). The protocol should say that request with type 'foo' and sequence number 'bar' has failed.
WebCore/inspector/InspectorController.cpp:2210
+ m_inspectorBackendDispatcher->dispatch(message, &exception);
Nit: should this guy be static and receive backend instance as an argument to emphasize its stateless nature?
WebCore/inspector/InspectorController.h:125
+ void dispatchOnBackend(long callId, const String& message);
This should be dispatchOnBackend(const String& message). We should serialize all the call data into a single message.
WebCore/inspector/front-end/InspectorBackendStub.js:35
+ this._registerDelegate("addInspectedNode");
Can you generate these?
WebCore/inspector/front-end/InspectorBackendStub.js:124
+ var callbackId = WebInspector.Callback.wrap(function(){});
You don't need this artificial call id / empty response callback anymore. Just send the message.
WebKit/chromium/src/InspectorFrontendClientImpl.cpp:132
+ WebVector<WebString> args((size_t)2);
We only allow static_cast<> in such places.
WebKit/chromium/src/InspectorFrontendClientImpl.cpp:136
+ m_client->sendMessageToAgent(messageData);
I'd introduce new sendWebKitMessageToBackend(string) and use it here. It will eventually become the main transport for the interaction.
I understand that this patch is growing and you'd like to land it. Please address everything in the follow up patches, but following I'd like to see fixed prior to landing this one: > WebCore/inspector/Inspector.idl:42 > + [notify] void didDispatch(out long callId, out String e); > Also, we should not have a generic ack / failure reporting. Each of the backend methods should ack (either with the results or with failure reporting). The protocol should say that request with type 'foo' and sequence number 'bar' has failed. Please fix this ^^. > WebCore/inspector/InspectorController.h:125 > + void dispatchOnBackend(long callId, const String& message); > This should be dispatchOnBackend(const String& message). We should serialize all the call data into a single message. Please fix this ^^. > WebCore/inspector/front-end/InspectorBackendStub.js:124 > + var callbackId = WebInspector.Callback.wrap(function(){}); > You don't need this artificial call id / empty response callback anymore. Just send the message. Please fix this ^^. > WebKit/chromium/src/InspectorFrontendClientImpl.cpp:132 > + WebVector<WebString> args((size_t)2); > We only allow static_cast<> in such places. Please fix this ^^. Created attachment 62675 [details]
[patch] third iteration.
generic callId and related things were removed.
Committed r64124 M WebKit/chromium/ChangeLog M WebKit/chromium/src/WebDevToolsAgentImpl.cpp M WebKit/chromium/src/InspectorFrontendClientImpl.cpp D WebKit/chromium/src/js/InspectorControllerImpl.js M WebKit/chromium/src/WebDevToolsAgentImpl.h M WebKit/chromium/src/ToolsAgent.h M WebKit/chromium/src/InspectorFrontendClientImpl.h M WebKit/chromium/WebKit.gypi M WebCore/WebCore.exp.in M WebCore/DerivedSources.cpp M WebCore/WebCore.pri M WebCore/Android.derived.v8bindings.mk M WebCore/ChangeLog M WebCore/WebCore.vcproj/WebCore.vcproj M WebCore/GNUmakefile.am M WebCore/WebCore.gypi M WebCore/bindings/js/ScriptObject.h M WebCore/bindings/js/ScriptObject.cpp M WebCore/bindings/v8/ScriptObject.h M WebCore/bindings/v8/ScriptObject.cpp M WebCore/DerivedSources.make D WebCore/inspector/InspectorBackend.idl M WebCore/inspector/CodeGeneratorInspector.pm M WebCore/inspector/InspectorFrontendHost.cpp M WebCore/inspector/InspectorFrontendClient.h M WebCore/inspector/InspectorFrontendClientLocal.cpp M WebCore/inspector/InspectorFrontendHost.h M WebCore/inspector/front-end/InspectorBackendStub.js M WebCore/inspector/InspectorFrontendClientLocal.h M WebCore/inspector/InspectorFrontendHost.idl M WebCore/CMakeLists.txt M WebCore/WebCore.xcodeproj/project.pbxproj W: -empty_dir: trunk/WebCore/inspector/InspectorBackend.idl W: -empty_dir: trunk/WebKit/chromium/src/js/InspectorControllerImpl.js r64124 = a471a4cc0749dddd35a779ad3abff89150aa6cce (refs/remotes/trunk) |