Bug 43489 - Web Inspector: It would be better to make did* method calls automatically
Summary: Web Inspector: It would be better to make did* method calls automatically
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ilya Tikhonovsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-04 08:49 PDT by Ilya Tikhonovsky
Modified: 2010-08-10 23:05 PDT (History)
11 users (show)

See Also:


Attachments
[patch] preliminary version. Not for landing. (59.80 KB, patch)
2010-08-04 08:51 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
InspectorBackendDispatcher.cpp - sample of generated file. (97.71 KB, application/octet-stream)
2010-08-04 08:52 PDT, Ilya Tikhonovsky
no flags Details
[patch] initial version. (80.78 KB, patch)
2010-08-05 03:34 PDT, Ilya Tikhonovsky
yurys: review-
Details | Formatted Diff | Diff
InspectorBackendDispatcher.cpp - sample of generated file. (97.37 KB, application/octet-stream)
2010-08-05 03:35 PDT, Ilya Tikhonovsky
no flags Details
[patch] second iteration (78.53 KB, patch)
2010-08-05 07:49 PDT, Ilya Tikhonovsky
yurys: review+
Details | Formatted Diff | Diff
sample: InspectorBackendDispatcher.cpp (99.67 KB, application/octet-stream)
2010-08-05 07:50 PDT, Ilya Tikhonovsky
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Tikhonovsky 2010-08-04 08:49:27 PDT
In the current implementation of inspector agent's we're calling did methods manually.
That is error prone and bad from protocol point of view.
It would be better to call did methods automatically  from the corresponding methods of InspectorBackendDispatcher and report the results.
As far as our protocol is statefull this trick will keep it in the consistent state.
Comment 1 Ilya Tikhonovsky 2010-08-04 08:51:22 PDT
Created attachment 63453 [details]
[patch] preliminary version. Not for landing.
Comment 2 Ilya Tikhonovsky 2010-08-04 08:52:12 PDT
Created attachment 63454 [details]
InspectorBackendDispatcher.cpp - sample of generated file.
Comment 3 Joseph Pecoraro 2010-08-04 10:04:10 PDT
Is this missing the frontend's implementation of protocolError? I'm assuming
it just removes the wrapped Callback so it doesn't leak.
Comment 4 Ilya Tikhonovsky 2010-08-05 03:34:44 PDT
Created attachment 63570 [details]
[patch] initial version.
Comment 5 Ilya Tikhonovsky 2010-08-05 03:35:16 PDT
Created attachment 63571 [details]
InspectorBackendDispatcher.cpp - sample of generated file.
Comment 6 Ilya Tikhonovsky 2010-08-05 03:52:09 PDT
Comment on attachment 63570 [details]
[patch] initial version.

Double entry in ChangeLog.
Comment 7 Yury Semikhatsky 2010-08-05 06:17:12 PDT
Comment on attachment 63570 [details]
[patch] initial version.

WebCore/ChangeLog:5
 +          WebInspector: In the current implementation of inspector agent's we're calling
typo: agent's -> agents

WebCore/inspector/CodeGeneratorInspector.pm:306
 +      push(@function, "        protocolError(callId, backendFunctionName, formatWrongArgumentsCountMessage(args->length() - 1, $expectedParametersCount));");
I think it should be retportProtocolError according to the style guide

WebCore/inspector/InspectorBackend.cpp:129
 +      RefPtr<InspectorArray> result = InspectorArray::create();
this should be *names = InspectorArray::create()

WebCore/inspector/Inspector.idl:131
 +          [handler=DOM, customResponse=didApplyDomChange] void setTextNodeValue(in long callId, in long nodeId, in String value, out boolean success);
Would be nice if we could avoid custom responses

WebCore/inspector/Inspector.idl:134
 +          [handler=DOM] void removeNode(in long callId, in long nodeId, out long outNodeId);
I think we don't need out outNodeId in this method.

WebCore/inspector/InspectorDOMAgent.cpp:1303
 +  void InspectorDOMAgent::addRule(long, const String& selector, long selectedNodeId, RefPtr<InspectorValue>* ruleObject, bool* selectorAffectsNode)
remove first parameter completely?

WebCore/inspector/InspectorController.cpp:1452
 +  void InspectorController::removeDOMStorageItem(long, long storageId, const String& key, bool* success)
remove first parameter completely?

WebCore/inspector/InspectorController.cpp:1521
 +  void InspectorController::getProfileHeaders(long, RefPtr<InspectorArray>* headers)
remove first parameter completely here and in other places?

WebCore/inspector/InspectorController.cpp:1707
 +          *newCallFrames = currentCallFrames();
please add else branch *newCallFrames = InspectorValue::null();

WebCore/inspector/InspectorController.h:221
 +      void getDOMStorageEntries(long callId, long storageId, RefPtr<InspectorArray>* entries);
no need to pass callId

WebCore/inspector/InspectorDOMAgent.cpp:560
 +  {
 outNodeId should be removed

WebCore/inspector/InspectorDOMAgent.cpp:1140
 +      *success = false;
consider generating those initializers in InspectorBackendDispatcher
Comment 8 Ilya Tikhonovsky 2010-08-05 07:49:50 PDT
Created attachment 63591 [details]
[patch] second iteration

WebCore/ChangeLog:5
 +          WebInspector: In the current implementation of inspector agent's
we're calling
typo: agent's -> agents

done.



WebCore/inspector/CodeGeneratorInspector.pm:306
 +      push(@function, "        protocolError(callId, backendFunctionName,
formatWrongArgumentsCountMessage(args->length() - 1,
$expectedParametersCount));");
I think it should be retportProtocolError according to the style guide

done.



WebCore/inspector/InspectorBackend.cpp:129
 +      RefPtr<InspectorArray> result = InspectorArray::create();
this should be *names = InspectorArray::create()

default values are assigning in the generated code.



WebCore/inspector/Inspector.idl:131
 +          [handler=DOM, customResponse=didApplyDomChange] void
setTextNodeValue(in long callId, in long nodeId, in String value, out boolean
success);
Would be nice if we could avoid custom responses

will do that in another patch



WebCore/inspector/Inspector.idl:134
 +          [handler=DOM] void removeNode(in long callId, in long nodeId, out
long outNodeId);
I think we don't need out outNodeId in this method.

will do that in another patch



WebCore/inspector/InspectorController.cpp:1521
 +  void InspectorController::getProfileHeaders(long, RefPtr<InspectorArray>*
headers)
remove first parameter completely here and in other places?

will do that in another patch



WebCore/inspector/InspectorController.cpp:1707
 +          *newCallFrames = currentCallFrames();
please add else branch *newCallFrames = InspectorValue::null();

default values are assigning in the generated code.



WebCore/inspector/InspectorDOMAgent.cpp:560
 +  {
 outNodeId should be removed

will do that in another patch



WebCore/inspector/InspectorDOMAgent.cpp:1140
 +      *success = false;
consider generating those initializers in InspectorBackendDispatcher

done.
Comment 9 Ilya Tikhonovsky 2010-08-05 07:50:39 PDT
Created attachment 63592 [details]
sample: InspectorBackendDispatcher.cpp
Comment 10 WebKit Review Bot 2010-08-05 13:06:17 PDT
http://trac.webkit.org/changeset/64770 might have broken Leopard Intel Release (Tests)
Comment 11 Adam Barth 2010-08-10 23:05:41 PDT
This patch appears to have been landed.