Summary: | Web Inspector: add ITML debuggable/target type | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||||||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | bburg, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||
Bug Depends on: | 200384, 203197 | ||||||||||||||||||||||||||
Bug Blocks: | 201150, 212497 | ||||||||||||||||||||||||||
Attachments: |
|
Description
Devin Rousso
2019-10-23 10:11:29 PDT
Created attachment 381716 [details]
[Patch] WIP
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`) This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features. Created attachment 383208 [details]
Patch
Created attachment 387892 [details]
Patch
Rebase
Created attachment 387893 [details]
Patch
Comment on attachment 387893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387893&action=review I did a quick once over, this looks good! > Source/WebInspectorUI/UserInterface/Base/Main.js:3155 > - if (!WI.sharedApp.isWebDebuggable()) > + if (!InspectorBackend.hasCommand("Page.archive")) Nice =) > Source/WebInspectorUI/UserInterface/Protocol/Legacy/10.3/InspectorBackendCommands.js:176 > -InspectorBackend.activateDomain("Database", ["page"]); > +InspectorBackend.activateDomain("Database", ["itml", "page"]); We don't want "itml" for the "Database" domain. These probably need to be regenerated. > Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:361 > _handleTableKeyDown(event) > { > - if (event.keyCode === WI.KeyboardShortcut.Key.Backspace.keyCode || event.keyCode === WI.KeyboardShortcut.Key.Delete.keyCode) > - this._table.removeSelectedRows(); > + if (event.keyCode === WI.KeyboardShortcut.Key.Backspace.keyCode || event.keyCode === WI.KeyboardShortcut.Key.Delete.keyCode) { > + if (InspectorBackend.hasCommand("Page.deleteCookie")) > + this._table.removeSelectedRows(); > + } > } Do this beep appropriately if there is a delete and nothing gets deleted? This code is weird (probably an existing issue if there is an issue). > WebKit.xcworkspace/xcshareddata/xcschemes/All Source.xcscheme:60 > - BuildableName = "libANGLE-shared.dylib" > + BuildableName = "libANGLE.a" This seems wrong. Comment on attachment 387893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387893&action=review >> Source/WebInspectorUI/UserInterface/Protocol/Legacy/10.3/InspectorBackendCommands.js:176 >> +InspectorBackend.activateDomain("Database", ["itml", "page"]); > > We don't want "itml" for the "Database" domain. These probably need to be regenerated. I think I just messed up this one. It should be `InspectorBackend.activateDomain("DOMStorage", ["itml", "page"]);` instead :( >> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:361 >> } > > Do this beep appropriately if there is a delete and nothing gets deleted? This code is weird (probably an existing issue if there is an issue). I doubt it. >> WebKit.xcworkspace/xcshareddata/xcschemes/All Source.xcscheme:60 >> + BuildableName = "libANGLE.a" > > This seems wrong. How did this get added??!?!? Created attachment 387896 [details]
Patch
Comment on attachment 387896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387896&action=review r=me! > Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:203 > + for (let [targetType, command] of domain._supportedCommandsForTargetType) { > + if (!supportedTargetTypes.has(targetType)) > + continue; I think we should be able to write a test for this (namely the last line in an LayoutTests/inspector test): InspectorTest.expectTrue(InspectorBackend.hasDomain("DOM"), "Web Debuggable has DOM Domain"); InspectorTest.expectTrue(InspectorBackend.hasCommand("DOM.requestNode"), "DOM.requestNode is available"); InspectorTest.expectFalse(InspectorBackend.hasCommand("DOM.getDataBindingsForNode"), "DOM.getDataBindingsForNode is not available"); Created attachment 400025 [details]
Patch
Comment on attachment 400025 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400025&action=review r=me too > Source/JavaScriptCore/API/JSContextPrivate.h:112 > + @discussion ITMLKit. Please expand or remove this line. Created attachment 400488 [details]
Patch
i undid the removal of the "extra agents" stuff, as that may still be desired if an older client links against this new WebKit. instead, we just don't dispatch `Inspector.activateExtraDomains` unless we're a basic `JavaScript` debuggable.
Created attachment 400512 [details]
Patch
Created attachment 400516 [details]
Patch
oops wrong method
Created attachment 400527 [details]
Patch
Created attachment 400545 [details]
Patch
add more protocol checks
Committed r262302: <https://trac.webkit.org/changeset/262302> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400545 [details]. |