Currently the code generation for a method for calling a backend method is pretty messy and produces warnings with the static analyzer. For example: NOTE: unnecessary protocolErrors creation. void InspectorLayerTreeBackendDispatcher::disable(long callId, const InspectorObject&) { RefPtr<InspectorArray> protocolErrors = InspectorArray::create(); RefPtr<InspectorObject> result = InspectorObject::create(); ErrorString error; if (!protocolErrors->length()) { m_agent->disable(&error); } m_backendDispatcher->sendResponse(callId, result.release(), protocolErrors.release(), error); } NOTE: weird whitespace, things out of order. weird nesting for setting out values in the end. void InspectorLayerTreeBackendDispatcher::layersForNode(long callId, const InspectorObject& message) { RefPtr<InspectorArray> protocolErrors = InspectorArray::create(); RefPtr<TypeBuilder::Array<TypeBuilder::LayerTree::Layer> > out_layers; RefPtr<InspectorObject> paramsContainer = message.getObject("params"); InspectorObject* paramsContainerPtr = paramsContainer.get(); InspectorArray* protocolErrorsPtr = protocolErrors.get(); int in_nodeId = InspectorBackendDispatcher::getInt(paramsContainerPtr, ASCIILiteral("nodeId"), nullptr, protocolErrorsPtr); RefPtr<InspectorObject> result = InspectorObject::create(); ErrorString error; if (!protocolErrors->length()) { m_agent->layersForNode(&error, in_nodeId, out_layers); if (!error.length()) { result->setValue(ASCIILiteral("layers"), out_layers); } } m_backendDispatcher->sendResponse(callId, result.release(), protocolErrors.release(), error); } Suggested code would output only what is needed, and should be simpler to read at a glance: // Method with no in or out parmeters. InspectorFooBackendDispatcher::disable(long callId, const InspectorObject& message) { // NOTE: No in parameters, or out parameters, just run function. ErrorString error; RefPtr<InspectorObject> result = InspectorObject::create(); m_agent->disable(&error); m_backendDispatcher->sendResponse(callId, result.release(), "Foo.disable", error); } // Method with in and out parameters. InspectorFooBackendDispatcher::bar(long callId, const InspectorObject& message) { // NOTE: Parse Parameters may have errors. RefPtr<InspectorArray> protocolErrors = InspectorArray::create(); RefPtr<InspectorObject> paramsContainer = requestMessageObject->getObject("params"); InspectorObject* paramsContainerPtr = paramsContainer.get(); InspectorArray* protocolErrorsPtr = protocolErrors.get(); String in_paramName = getString(paramsContainerPtr, ASCIILiteral("paramName"), nullptr, protocolErrorsPtr); if (protocolErrors->length()) { m_backendDispatcher->reportProtocolErrors(protocolErrors); return; } // NOTE: Method dispatch, may fill error string. ErrorString error; RefPtr<InspectorObject> result = InspectorObject::create(); RefPtr<TypeBuilder::Page::FrameResourceTree> out_returnName; m_agent->getResourceTree(&error, out_returnName); // NOTE: Out parameters. if (!error.length()) result->setValue("returnName", out_returnName); // NOTE: Ending - Send response. m_backendDispatcher->sendResponse(callId, result.release(), error); } // Method "async" has a callback. Also shows an optional parameter. InspectorFooFrontendDispatcher::baz(long callId, const InspectorObject& message) { // NOTE: Parse parameters may have errors. RefPtr<InspectorArray> protocolErrors = InspectorArray::create(); RefPtr<InspectorObject> paramsContainer = message.getObject("params"); InspectorObject* paramsContainerPtr = paramsContainer.get(); InspectorArray* protocolErrorsPtr = protocolErrors.get(); String in_securityOrigin = InspectorBackendDispatcher::getString(paramsContainerPtr, ASCIILiteral("securityOrigin"), nullptr, protocolErrorsPtr); String in_databaseName = InspectorBackendDispatcher::getString(paramsContainerPtr, ASCIILiteral("databaseName"), nullptr, protocolErrorsPtr); bool objectStoreName_valueFound = false; String in_objectStoreName = InspectorBackendDispatcher::getString(paramsContainerPtr, ASCIILiteral("objectStoreName"), &objectStoreName_valueFound, protocolErrorsPtr); if (protocolErrors->length()) { String errorMessage = String::format("Some arguments of method '%s' can't be processed", "Foo.baz"); m_backendDispatcher->reportProtocolError(&callId, InvalidParams, errorMessage, protocolErrors.release()); return; } // NOTE: Method dispatch, with Callback. ErrorString error; RefPtr<InspectorObject> result = InspectorObject::create(); RefPtr<InspectorIndexedDBBackendDispatcherHandler::ClearObjectStoreCallback> callback = adoptRef(new InspectorIndexedDBBackendDispatcherHandler::ClearObjectStoreCallback(m_backendDispatcher, callId)); m_agent->clearObjectStore(&error, in_securityOrigin, in_databaseName, objectStoreName_valueFound ? &in_objectStoreName : nullptr, callback); // NOTE: Callback has out parameters in callback. if (error.length()) { callback->disable(); m_backendDispatcher->reportProtocolError(&callId, ServerError, invocationError); return; } // NOTE: Ending - Callback will be called... so nothing goes here. }
<rdar://problem/15466376>
Created attachment 216896 [details] [PATCH] Proposed Fix
Created attachment 216897 [details] [BEFORE] InspectorBackendDispatchers.cpp
Created attachment 216898 [details] [AFTER] InspectorBackendDispatchers.cpp
Attachment 216896 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/inspector/CodeGeneratorInspector.py', u'Source/WebCore/inspector/CodeGeneratorInspectorStrings.py', u'Source/WebCore/inspector/InspectorBackendDispatcher.cpp', u'Source/WebCore/inspector/InspectorBackendDispatcher.h']" exit_code: 1 Source/WebCore/inspector/CodeGeneratorInspector.py:2060: multiple statements on one line (semicolon) [pep8/E702] [5] Source/WebCore/inspector/CodeGeneratorInspector.py:2069: too many blank lines (2) [pep8/E303] [5] Source/WebCore/inspector/CodeGeneratorInspector.py:2092: trailing whitespace [pep8/W291] [5] Source/WebCore/inspector/CodeGeneratorInspector.py:2093: multiple statements on one line (semicolon) [pep8/E702] [5] Source/WebCore/inspector/CodeGeneratorInspector.py:2099: trailing whitespace [pep8/W291] [5] Source/WebCore/inspector/CodeGeneratorInspector.py:2153: trailing whitespace [pep8/W291] [5] Source/WebCore/inspector/CodeGeneratorInspector.py:2154: too many blank lines (2) [pep8/E303] [5] Total errors found: 7 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
The reasons the file is bigger now is because of this added code for each method with parameters: if (protocolErrors->length()) { String errorMessage = String::format("Some arguments of method '%s' can't be processed", "Foo.baz"); m_backendDispatcher->reportProtocolError(&callId, InvalidParams, errorMessage, protocolErrors.release()); return; } This is just us being more explicit. This used to happen inside of sendResponse(...) before, and thus sendResponse had some extra parameters and was confusing. I don't expect the actual linked code to be larger, I actually would expect it to be smaller in the end. (I wonder how I can measure that)
Created attachment 216899 [details] [PATCH] Proposed Fix Fixed python style.
Comment on attachment 216899 [details] [PATCH] Proposed Fix Clearing flags on attachment: 216899 Committed r159289: <http://trac.webkit.org/changeset/159289>
All reviewed patches have been landed. Closing bug.