Bug 124333 – Web Inspector: Cleaner Backend Method Calling Code Generation

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

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.
    }

[Radar WebKit Bug Importer]

<rdar://problem/15466376>

[Joseph Pecoraro]

Created attachment 216896 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 216897 [details]
[BEFORE] InspectorBackendDispatchers.cpp

[Joseph Pecoraro]

Created attachment 216898 [details]
[AFTER] InspectorBackendDispatchers.cpp

[WebKit Commit Bot]

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.

[Joseph Pecoraro]

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)

[Joseph Pecoraro]

Created attachment 216899 [details]
[PATCH] Proposed Fix

Fixed python style.

[WebKit Commit Bot]

Comment on attachment 216899 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 216899

Committed r159289: <http://trac.webkit.org/changeset/159289>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.