Bug 203300 - Web Inspector: add ITML debuggable/target type
Summary: Web Inspector: add ITML debuggable/target type
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 200384 203197
Blocks: 201150 212497
  Show dependency treegraph
 
Reported: 2019-10-23 10:11 PDT by Devin Rousso
Modified: 2020-05-29 10:34 PDT (History)
11 users (show)

See Also:


Attachments
[Patch] WIP (84.57 KB, patch)
2019-10-23 13:08 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (669.68 KB, patch)
2019-11-09 01:03 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (686.94 KB, patch)
2020-01-15 21:15 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (692.72 KB, patch)
2020-01-15 21:22 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (691.92 KB, patch)
2020-01-15 23:02 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (555.61 KB, patch)
2020-05-21 21:46 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (530.91 KB, patch)
2020-05-28 11:48 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (530.34 KB, patch)
2020-05-28 15:07 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (530.36 KB, patch)
2020-05-28 15:37 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (531.15 KB, patch)
2020-05-28 17:41 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (544.04 KB, patch)
2020-05-28 21:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-10-23 10:11:29 PDT
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.
Comment 1 Radar WebKit Bug Importer 2019-10-23 11:07:33 PDT
<rdar://problem/56545896>
Comment 2 Devin Rousso 2019-10-23 13:08:27 PDT
Created attachment 381716 [details]
[Patch] WIP
Comment 3 EWS Watchlist 2019-10-23 13:09:06 PDT
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.
Comment 4 Devin Rousso 2019-11-09 01:03:19 PST
Created attachment 383208 [details]
Patch
Comment 5 Devin Rousso 2020-01-15 21:15:44 PST
Created attachment 387892 [details]
Patch

Rebase
Comment 6 Devin Rousso 2020-01-15 21:22:31 PST
Created attachment 387893 [details]
Patch
Comment 7 Joseph Pecoraro 2020-01-15 22:04:44 PST
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 8 Devin Rousso 2020-01-15 22:56:47 PST
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??!?!?
Comment 9 Devin Rousso 2020-01-15 23:02:29 PST
Created attachment 387896 [details]
Patch
Comment 10 Joseph Pecoraro 2020-01-16 13:30:07 PST
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");
Comment 11 Devin Rousso 2020-05-21 21:46:31 PDT
Created attachment 400025 [details]
Patch
Comment 12 BJ Burg 2020-05-22 15:21:43 PDT
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.
Comment 13 Devin Rousso 2020-05-28 11:48:31 PDT
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.
Comment 14 Devin Rousso 2020-05-28 15:07:49 PDT
Created attachment 400512 [details]
Patch
Comment 15 Devin Rousso 2020-05-28 15:37:00 PDT
Created attachment 400516 [details]
Patch

oops wrong method
Comment 16 Devin Rousso 2020-05-28 17:41:05 PDT
Created attachment 400527 [details]
Patch
Comment 17 Devin Rousso 2020-05-28 21:32:41 PDT
Created attachment 400545 [details]
Patch

add more protocol checks
Comment 18 EWS 2020-05-29 10:34:13 PDT
Committed r262302: <https://trac.webkit.org/changeset/262302>

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