Bug 43489 – Web Inspector: It would be better to make did* method calls automatically

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Ilya Tikhonovsky]

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.

[Ilya Tikhonovsky]

Created attachment 63453 [details]
[patch] preliminary version. Not for landing.

[Ilya Tikhonovsky]

Created attachment 63454 [details]
InspectorBackendDispatcher.cpp - sample of generated file.

[Joseph Pecoraro]

Is this missing the frontend's implementation of protocolError? I'm assuming
it just removes the wrapped Callback so it doesn't leak.

[Ilya Tikhonovsky]

Created attachment 63570 [details]
[patch] initial version.

[Ilya Tikhonovsky]

Created attachment 63571 [details]
InspectorBackendDispatcher.cpp - sample of generated file.

[Ilya Tikhonovsky]

Comment on attachment 63570 [details]
[patch] initial version.

Double entry in ChangeLog.

[Yury Semikhatsky]

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

[Ilya Tikhonovsky]

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.

[Ilya Tikhonovsky]

Created attachment 63592 [details]
sample: InspectorBackendDispatcher.cpp

[WebKit Review Bot]

http://trac.webkit.org/changeset/64770 might have broken Leopard Intel Release (Tests)

[Adam Barth]

This patch appears to have been landed.