Summary: | Web Inspector: Protocol: add type checking when commands are called via invoke | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bburg, commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Devin Rousso
2019-03-04 20:39:55 PST
Created attachment 363596 [details]
Patch
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.
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 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
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.
Created attachment 363678 [details]
Patch
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.
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). 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. Created attachment 363905 [details]
Patch
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.
Comment on attachment 363905 [details]
Patch
r=me
Comment on attachment 363905 [details] Patch Clearing flags on attachment: 363905 Committed r242606: <https://trac.webkit.org/changeset/242606> All reviewed patches have been landed. Closing bug. |