Bug 195310 – Web Inspector: Protocol: add type checking when commands are called via invoke

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

[Devin Rousso]

Right now we only do type checking for when it is called like a normal function.

[Radar WebKit Bug Importer]

<rdar://problem/48588679>

[Devin Rousso]

Created attachment 363596 [details]
Patch

[EWS Watchlist]

Attachment 363596 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:426:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:429:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:475:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 363596 [details]
Patch

Attachment 363596 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11375595

New failing tests:
fast/scrolling/ios/hit-testing-iframe-004.html

[EWS Watchlist]

Created attachment 363628 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Blaze Burg]

Comment on attachment 363596 [details]
Patch

Good change but.. it needs to be tested. Please update inspector-backend-invocation-return-value.html to exercise the invoke() code paths.

[Devin Rousso]

Created attachment 363678 [details]
Patch

[EWS Watchlist]

Attachment 363678 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:423:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:428:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:435:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:481:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 363678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363678&action=review

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:428
> +            if (typeof commandArguments !== "object") {
> +                if (!optional)
> +                    return deliverFailure(`Invalid number of arguments for command '${instance.qualifiedName}'.`);
> +                continue;
> +            }
> +
> +            if (!(name in commandArguments) && !optional)
> +                return deliverFailure(`Missing argument '${name}' for command '${instance.qualifiedName}'.`);

Could we just require the first argument is always an object to simplify this a bit:

    if (typeof commandArguments !== "object")
        return deliverFailure(`invoke expects an object for command arguments but it was a '${typeof commandArguments}'.`);

    let parameters = {};
    for (let {name, type, optional} of instance.callSignature) {
        if ((!name in commandArguments && !optional)
            return deliverFailure(`Missing argument '${name}' for command '${instance.qualifiedName}'.`);
        ...
    }

> LayoutTests/inspector/protocol/inspector-backend-invocation-return-value.html:122
> +            let result = RuntimeAgent.evaluate.invoke();

In other words I don't think we should allow calling invoke with no object (even if there are no required arguments).

[Devin Rousso]

Comment on attachment 363678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363678&action=review

>> LayoutTests/inspector/protocol/inspector-backend-invocation-return-value.html:122
>> +            let result = RuntimeAgent.evaluate.invoke();
> 
> In other words I don't think we should allow calling invoke with no object (even if there are no required arguments).

I'm fine with that.  I wasn't sure which approach we wanted to take, so I just matched the existing functionality.

[Devin Rousso]

Created attachment 363905 [details]
Patch

[EWS Watchlist]

Attachment 363905 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:420:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:425:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:432:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:478:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 363905 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 363905 [details]
Patch

Clearing flags on attachment: 363905

Committed r242606: <https://trac.webkit.org/changeset/242606>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.