Bug 203300 – Web Inspector: add ITML debuggable/target type

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

This will allow the frontend to do distinguish between a regular `JSContext` and an ITML one, as well as knowing at launch what domains/commands/events will be supported by ITML, most of which are not normally supported by `JSContext`, which would also allow us to remove the "extra domains" concept.

[Radar WebKit Bug Importer]

<rdar://problem/56545896>

[Devin Rousso]

Created attachment 381716 [details]
[Patch] WIP

[EWS Watchlist]

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.

[Devin Rousso]

Created attachment 383208 [details]
Patch

[Devin Rousso]

Created attachment 387892 [details]
Patch

Rebase

[Devin Rousso]

Created attachment 387893 [details]
Patch

[Joseph Pecoraro]

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.

[Devin Rousso]

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??!?!?

[Devin Rousso]

Created attachment 387896 [details]
Patch

[Joseph Pecoraro]

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");

[Devin Rousso]

Created attachment 400025 [details]
Patch

[Blaze Burg]

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.

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 400512 [details]
Patch

[Devin Rousso]

Created attachment 400516 [details]
Patch

oops wrong method

[Devin Rousso]

Created attachment 400527 [details]
Patch

[Devin Rousso]

Created attachment 400545 [details]
Patch

add more protocol checks

[EWS]

Committed r262302: <https://trac.webkit.org/changeset/262302>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400545 [details].