[webkit-reviews] review denied: [Bug 200384] Web Inspector: rework frontend agent construction to allow commands/events to be controlled by the related target's type : [Attachment 377553] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 14 17:25:49 PDT 2019
Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 200384: Web Inspector: rework frontend agent construction to allow
commands/events to be controlled by the related target's type
https://bugs.webkit.org/show_bug.cgi?id=200384
Attachment 377553: Patch
https://bugs.webkit.org/attachment.cgi?id=377553&action=review
--- Comment #36 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 377553
--> https://bugs.webkit.org/attachment.cgi?id=377553
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=377553&action=review
Awesome!
I think this patch does a little too much at once (ITML, assumingMainTarget
sometimes, node highlighter changes, node query changes, and the core protocol
changes). But now that it has been reviewed I'm still fine with keeping most of
these in one patch. However, please extract the ITMLKit specific changes
(APIs), as I didn't review that in detail and it should really be separate.
I'm ready to r+, but I want to see an updated and rebaselined patch without the
ITMLKit SPIs.
I'd probably also want to play with it just a bit to sanity check some things
(like JSContext inspection and older iOS device inspection).
> Source/JavaScriptCore/ChangeLog:74
> + * inspector/scripts/tests/*:
> + Update test results, as well as added new tests for
`debuggableTypes` and `targetTypes`.
You could enumerate the "Added" tests and * the rest.
> Source/WebInspectorUI/ChangeLog:9
> + `InspectorBackend.domains.${domain}` isn't a truly valid way to
feature check, as it
This ChangeLog is great!
There should also be a section listing out the debuggable types and target
types akin to:
https://bugs.webkit.org/show_bug.cgi?id=200384#c10
That way when this lands it will forever document the state of the world at
this point.
>>> Source/WebInspectorUI/ChangeLog:19
>>> + depending on the debuggable type. Furthermore, each target
underneath that debuggable needs
>>
>> Looks like each target will always have more accurate information about
supported domains/commands. Is it possible to get rid of the global
InspectorBackend.domains completely and instead always consult with
corresponding target and have some meaningful defaults when there is no target
yet?
>
> The target will know what it supports, but we can be simultaneously connected
to multiple targets. See above comment.
Yeah, I think this was discussed offline but our goal is to move to
`target.FooAgent.command` feature checks as much as possible, as that will be
the most accurate for that target. However there are still cases where we want
`InspectorBackend.domains.FooAgent.command` checks to know if some UI should
even be shown given what we are connected to, as that will indicate whether
some UI feature is even relevant or not.
> Source/JavaScriptCore/API/JSContextPrivate.h:114
> +- (void)_setITMLDebuggableType JSC_API_AVAILABLE(macos(JSC_MAC_TBA),
ios(JSC_IOS_TBA));
Have we tried ITML with this? Can we keep the ITMLKit parts of the patch
separate to reduce the large scope of this?
NOTE: I wrote some of these comments a long time ago but forgot to submit the
early review. I agree with Yury that smaller aspects, like this, can be
extracted out of this patch and done separately.
> Source/JavaScriptCore/inspector/protocol/Audit.json:3
> "description": "",
We should have given this a description back in the day!
> Source/JavaScriptCore/inspector/protocol/Audit.json:4
> + "targetTypes": ["itml", "jscontext", "page", "service-worker",
"worker"],
Do Audits work in a Worker today?
I know they work in a jscontext / service-worker though I still think that is
probably non-ideal. Unless we can produce `unsupported` results based on the
target.
> Source/JavaScriptCore/inspector/protocol/Security.json:5
> + "targetTypes": ["itml", "service-worker", "page"],
Nit: You normally do these in alphabetical order. "page" < "service-worker".
> Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:381
> + case RemoteInspectionTarget::Type::ITML:
More ITML code that could happen separately.
> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp:-144
> - else if (target.type() == RemoteInspectionTarget::Type::ServiceWorker)
> - targetListing->setString("type"_s, "service-worker"_s);
It looks like they previously supported service-worker, so we should probably
keep that case.
> Source/JavaScriptCore/inspector/scripts/codegen/models.py:453
> + allowed_strings = set(['itml', 'jscontext', 'service-worker',
'page', 'worker'])
Given this is used in 3 places maybe we should put
"allowed_target_type_strings" somewhere more global in this file.
> Source/JavaScriptCore/inspector/scripts/codegen/models.py:457
> +
We should probably also validate that this is a subset of the Domain's
target_types.
> Source/JavaScriptCore/inspector/scripts/codegen/models.py:460
> + raise ParseException("Malformed domain specification: call
parameter list for command %s has duplicate parameter names" % json['name'])
Bad error string. Should say "target types list for command %s has duplicate
target names"
> Source/JavaScriptCore/inspector/scripts/codegen/models.py:499
> +
We should probably also validate that this is a subset of the Domain's
target_types.
> Source/JavaScriptCore/inspector/scripts/codegen/models.py:502
> + raise ParseException("Malformed domain specification: call
parameter list for command %s has duplicate parameter names" % json['name'])
Bad error string. Should say "target types list for event %s has duplicate
target names"
>
Source/JavaScriptCore/inspector/scripts/tests/generic/domain-targetTypes.json:7
> +{
> + "domain": "Domain",
> + "targetTypes": ["page"],
> + "commands": [{"name": "Command", "targetTypes": ["page"]}],
> + "events": [{"name": "Event", "targetTypes": ["page"]}]
> +}
Would be nice to see tests when Event/Command targetsTypes do not match the
Domain's targetTypes:
Valid:
{
"domain": "Domain",
"debuggableTypes": ["itml", "page", "web-page"],
"targetTypes": ["itml", "page"],
"commands": [
{"name": "Command1"},
{"name": "Command2", "targetTypes": ["itml"]},
],
"events": [
{"name": "Event1"},
{"name": "Event2", "targetTypes": ["itml"]},
]
}
Invalid Command target:
{
"domain": "Domain",
"targetTypes": ["page"],
"commands": [
{"name": "ValidCommand"},
{"name": "InvalidCommand", "targetTypes": ["itml"]},
],
}
Invalid Event target:
{
"domain": "Domain",
"targetTypes": ["page"],
"events": [
{"name": "ValidEvent"},
{"name": "InvalidEvent", "targetTypes": ["itml"]},
],
}
>>>>>> Source/WebCore/inspector/InspectorFrontendClient.h:63
>>>>>> + virtual String debuggableType() const = 0;
>>>>>
>>>>> Should it return an enum value so that it's clear what are the all
supported debuggable types?
>>>>
>>>> Ideally, yes, but then it would complicate the logic of
`RemoteWebInspectorProxy` and `InspectorFrontendHost.debuggableType()`. We
"fall back" to being a JavaScript debuggable if an unknown/invalid type is
supplied, which is OK since all other types support JavaScript at the very
least. It would also make it ever-so-slightly more difficult for forks of
WebKit (or other clients) to add support for Web Inspector debugging as they'd
have to add an enum value.
>>>
>>> Can you elaborate on how it'd complicate it? There is already
debuggableTypeFromHost
>>> which translates from the constants used in c++ code to the ones used in
the frontend.
>>>
>>> If the forks cannot add support for their own type to the front-end they
>>> will face the same problem. So it seems that they'd have to either fall
back
>>> to basic JavaScript inspector, pick from the existing types or modify
WebKit to
>>> add new type anyway.
>>
>> Thinking about this again (it's been a while), I think the only real "issue"
with using an enum is handling the case where there is no value set (see
`InspectorFrontendHost::debuggableType` for an example of how this may happen).
I think we can work around this though, so we probably should make this into
an enum.
>
> Yeah, when there is no frontend client debuggableType should have some
meaningful fall back anyway, current return String() just defers that decision.
Yeah this could be an enum. Could do this separately, given this is
pre-existing.
> Source/WebCore/testing/Internals.cpp:331
> + String debuggableType() const final { return "protocol"_s; };
Would this trip the frontend up at all? Could be "page".
> Source/WebInspectorUI/.eslintrc:-37
> "globals": {
> - // Agents
Nice! Then ESLint can just tell use where they are!
> Source/WebInspectorUI/UserInterface/Base/Main.js:172
> + if (InspectorBackend.domains.Target.hasCommand("exists")) {
This deserves a compatibility comment. Backends before a certain time had
"exists" but now it can be feature checked immediately since a Target domain
only exists for a WebPage debuggable (and not a Page debuggable). This can
eventually go away!
> Source/WebInspectorUI/UserInterface/Base/Main.js:2759
> + let target = WI.assumingMainTarget();
> +
target.NetworkAgent.setResourceCachingDisabled(WI.settings.resourceCachingDisab
led.value);
Whoa! NetworkAgent should work across multiple targets. So this is probably an
existing bug and should just iterate all targets. (this could be pulled out
into its own bug fix)
for (let target of WI.targets) {
if (target.NetworkAgent)
target.NetworkAgent.setResourceCachingDisabled(WI.settings.resourceCachingDisab
led.value);
}
> Source/WebInspectorUI/UserInterface/Base/Main.js:3043
> + let target = WI.assumingMainTarget();
> + target.DOMAgent.undo();
Heh, this may be difficult across multiple targets. We might end up with a
stack in the frontend of targets to call undo/redo on.
>>>> Source/WebInspectorUI/UserInterface/Controllers/AppController.js:32
>>>> + switch (InspectorFrontendHost.debuggableType()) {
>>>
>>> Why does debuggable type have to come from InspectorFrontendHost? Can't it
be derived from the inspected target type?
>>
>> At this point, we haven't connected to any targets yet, so we don't know
anything about it. Theoretically, we can determine the `debuggableType`
information from the first target we connect to, but we need to know about the
`debuggableType` _before_ that happens too (see above for more in-depth
explanation).
>
> The UI could start assuming minimal feature set (i.e. JSContext) and extend
it later
> depending on the main target type. But as you mentioned it may lead to
undesirable
> UI transformations. To avoid that you want to learn event before the
connection is
> established what kind of target you are going to inspect. Using a native
binding helps
> achieve this but seems like a side channel. Imagine e.g. connection to the
remote
> target over a web socket from a frontend loaded as a regular page (no
RemoteInspectorUI
> bindings at all). Native InspectorFrontendHost would not be available but the
same
> functionality could be achieved via e.g. passing arguments to the front-end
url.
> Anyway, it's not directly related to this change.
"it may lead to undesirable UI transformations" - Precisely!
"... functionality could be achieved via e.g. passing arguments to the
front-end url" - Correct, in such a frontend configuration the state would need
to be passed in another way, like you're suggesting.
> Source/WebInspectorUI/UserInterface/Controllers/AppController.js:52
> + case "web": // COMPATIBILITY (iOS 13): replaced by "page" (WK1) and
"web-page" (WK2).
> + case "web-page":
> + this._debuggableType = WI.DebuggableType.WebPage;
> + break;
Maybe instead of (WK1) and (WK2) you could say (WebCore::Page) and
(WebKit::WebPage).
> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:319
> + let target = WI.assumingMainTarget();
Can these all be `frame.target` or are we waiting to eliminate
assumingMainTarget's in the future?
> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:339
> - RuntimeAgent.evaluate.invoke({expression, objectGroup: "",
includeCommandLineAPI: false, doNotPauseOnExceptionsAndMuteConsole: true,
contextId, returnByValue: false, generatePreview: false}, documentAvailable);
> + target.RuntimeAgent.evaluate.invoke({expression, objectGroup: "",
includeCommandLineAPI: false, doNotPauseOnExceptionsAndMuteConsole: true,
contextId, returnByValue: false, generatePreview: false}, documentAvailable);
The reason I suggested that is because we should have implemented RuntimeAgent
to be multi-target aware. I see this was not done because it is assuming the
page / frame target.
> Source/WebInspectorUI/UserInterface/Controllers/ConsoleManager.js:49
> + return
!!InspectorBackend.domains.Console.hasCommand("getLoggingChannels");
We can remove the `!!` from these cases.
> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:458
> - highlightDOMNode(nodeId, mode)
> + highlightDOMNode(node, mode)
A change like this could be split out into its own patch.
> Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:50
> + return
!!InspectorBackend.domains.Runtime.hasCommand("awaitPromise");
Nit: You can drop the `!!`.
> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:561
> + let target = WI.assumingMainTarget();
Nit: Why not `this.target` for this one?
> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:581
> + let target = WI.assumingMainTarget();
Nit: Why not `this.target` for this one?
> Source/WebInspectorUI/UserInterface/Protocol/ConsoleObserver.js:38
> - WI.consoleManager.messageWasAdded(this.target, message.source,
message.level, message.text, message.type, message.url, message.line,
message.column || 0, message.repeatCount, message.parameters,
message.stackTrace, message.networkRequestId);
> + WI.consoleManager.messageWasAdded(this._target, message.source,
message.level, message.text, message.type, message.url, message.line,
message.column || 0, message.repeatCount, message.parameters,
message.stackTrace, message.networkRequestId);
Seems okay to switch to `this._target` for Dispatcher subclasses, but
`this.target` would be our normal style.
> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:167
> + domain.addCommand(targetTypes, new
InspectorBackend.Command(qualifiedName, callSignature, replySignature));
This could pass the `commandName` here to avoid another `split` in the Command
constructor.
> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:174
> + domain.addEvent(targetTypes, new
InspectorBackend.Event(qualifiedName, signature));
This could pass the `eventName` here to avoid another `split` in the Event
constructor.
> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:273
> + targetTypes = targetTypes || Object.values(WI.TargetType);
Shouldn't the targetTypes be limited to the domain's target types? If a domain
is limited to "service-worker" I'd expect the commands to only be limited to
"service-worker" as well, not all targets types.
It seems like this is the common case, so currently this produces a lot of
duplication (a map per-service type).
>>>>>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:295
>>>>>> + console.assert(this._commandParameters.has(commandName) ===
this._commandReturns.has(commandName));
>>>>>
>>>>> It was impossible to have such problem in the previous code where each
command was represented by an object instead of several multimaps that have to
be kept in sync. Also
>>>>> if (InspectorBackend.domains.Timeline.setAutoCaptureEnabled)
>>>>> is a shorter than
>>>>> if
(InspectorBackend.domains.Timeline.hasCommand("setAutoCaptureEnabled"))
>>>>> and it would be possible to validate former with a js compiler should you
decide to use one.
>>>>> I wonder what is the motivation for changing this to string keyed
multimaps?
>>>>
>>>> I can't remember all the reasons why I decided to take this approach, but
yes it does make global feature checking slightly more verbose.
>>>>
>>>> I personally prefer a more verbose/explicit approach, as it more clearly
distinguishes callsites from feature-checks, and there is some precedent with
`hasEvent`. I would rather us have a very clear distinction between those two
use cases.
>>>>
>>>> FWIW, `hasCommandReturn` also isn't used right now, so we could remove the
code related to that in this patch, but it does make a more complete API
surface.
>>>
>>> I agree that hasCommand/hasParameter etc better reflect the intent of the
call, I'm just not a fan of using raw constants especially when we can generate
them and utilize some automatic checks, otherwise the code relies solely on the
tests and reviewers. If you are changing the signatures of feature checkers
anyway, would it make sense to make the calls less verbose by supporting
something like this:
>>>
>>> Backend.hasCommand("Target.exists");
>>>
>>> instead of
>>>
>>> InspectorBackend.domains.Target &&
InspectorBackend.domains.Target.hasCommand("exists")
>>>
>>> and similar methods for checking parameters and return values?
>>
>> My only hesitance to your suggestion is that it would then likely involve a
`String.prototype.split`, which is more work than just a Map/Object lookup.
I'd prefer to avoid doing work where we can, but other than that, I'm fine with
it.
>
> I wouldn't worry about performance here, this code runs very rarely (mostly
once for some small number of commands?) and 'split' call itself is very cheap.
I'd be very surprised if any of these methods appeared in a CPU profile. Unless
there is profile data indicating performance bottleneck I personally would
prefer more readable code to premature optimizations.
I also thought while reading this that
`InspectorBackend.hasCommand("Target.exists")` might work out well. It
certainly reads a bit nicer than the long combo, and there is likely to be
human error forgetting the domain check. Both approaches have the possibility
of human error mistyping the last piece (aside from a JS compiler / static
check approach Yury mentions). I agree that performance shouldn't be a concern
if we did this (every protocol message is doing this).
>>>>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:301
>>>>> + return this._commandParameters.has(commandName,
commandParameterName);
>>>>
>>>> What is the value of creating _commandParameters and _commandReturns
multimaps over storing a map name->InspectorBackend.Command and then
>>>> writing something like
>>>>
>>>> const command = this._command[commandName];
>>>> return command && command.hasParameter(commandParameterName);
>>>>
>>>> This is way you'd have only one copy of the command metadata and would use
that for both feature detection and agent construction.
>>>
>>> Ah I remember why I did this! So one reason for this is that we could
theoretically have multiple versions of the same command/event so long as
there's no overlap in the `targetTypes`, meaning that the same command/event
name could map to multiple `InspectorBackend.Command`/`InspectorBackend.Event`.
This would be a very useful thing to have for compatibility reasons for ITML
(WebKit can change without breaking ITML).
>>
>> How is that possible? Can a backend talk several different versions of the
protocol?
>>
>> If it's possible to have different versions of the same domain on different
target types should registerVersion take that into account and version
individual commands / sets of commands?
>
> There's nothing preventing this from happening. Fundamentally, the protocol
is just an organizing "framework" around JSON-RPC-like payloads being sent over
a connection. Each backend would likely only actually have one "version" of
each command/event, but the protocol JSON files are effectively a combination
of the capabilities all of the various "platforms" that are inspectable. As an
example, `DOM.getDataBindingsForNode` and `DOM.getAssociatedDataForNode` are
both ITML-specific features that we would likely not want to show for non-ITML
targets, so we'd only want those to be created if `debuggableType === "ITML"`.
The primary reason we'd want to do this, however, is for situations where
WebKit changes any of the parameters/returns for a command/event that ITML
doesn't want, so we'd be able to copy the current command/event and change the
`targetTypes` to just be "ITML", and then edit the original command/event to
have `targetTypes` for everything else.
>
> Theoretically, C++ supports function overloading, and the various
`*BackendDispatcherHandler` objects are generated from the JSON files anyways,
so as long as two commands with the same name don't have the same signature
(e.g. the types of the parameters/returns are the same) we could support
switching based on the types of the arguments. The intention for now isn't to
support this, as that needs other support in C++ to do that switching, but it
is a possibility that we may want to have in the future.
I do think that the multi maps are overkill. Even with the
`DOM.getDataBindingsForNode` case which is restricted to "ITML", then if the
frontend were to do a straight forward feature check:
InspectorBackend.domains.DOM.hasCommand("getDataBindingsForNode")
... it seems to me that this would always appear appear to always be supported
regardless of an ITML target or not. So we'd still end up doing an extra check
at feature detection points (assuming we don't have a Target), right?
> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:332
> + let command = Array.from(commands).find((command) =>
command.commandName === commandName);
> + console.assert(command);
I think we can avoid allocations from `Array.from(commands)` and iterate the
Set ourselves. The temporary array seems wasteful.
> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:427
> + "use strict";
This `"use strict"` no longer makes sense. We are inside of a ClassMethod so it
is strict.
>
Source/WebInspectorUI/UserInterface/Protocol/Legacy/13.0/InspectorBackendComman
ds.js:160
> +InspectorBackend.registerCommand("DOM.getDataBindingsForNode", ["itml"],
[{"name": "nodeId", "type": "number"}], ["dataBindings"]);
> +InspectorBackend.registerCommand("DOM.getAssociatedDataForNode", ["itml"],
[{"name": "nodeId", "type": "number"}], ["associatedData"]);
Are these the only commands with a specific target type?
> Source/WebInspectorUI/UserInterface/Test/Test.js:207
> +// FIXME: <https://webkit.org/b/201149> Web Inspector: replace all uses of
`window.*Agent` with a target-specific call
> +(function() {
> + function makeAgentGetter(domainName) {
Nice!
> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:239
> - let domNode = this.domNode;
> + let target = WI.assumingMainTarget();
Another case where it seems like we should do:
let target = domNode.target;
Assuming you want to replace many of these `assumingMainTarget` in this patch
now. It can be done later.
> Source/WebInspectorUI/UserInterface/Views/QuickConsole.js:117
> - if (!WI.mainTarget || !WI.mainTarget.executionContext)
> + if (!WI.mainTarget || WI.mainTarget instanceof
WI.MultiplexingBackendTarget)
> return;
Seems fine. I wonder if we should have done something smarter here. In general
I recall this was a particularly ugly corner of UI initialization =(
>>>>>> Source/WebInspectorUI/Versions/Inspector-iOS-9.3.json:3267
>>>>>> + "debuggableTypes": ["page", "web-page"],
>>>>>
>>>>> Looking at this code it's quite hard to guess how web-page is different
from page. Maybe legacy-page like Greg suggests or wk1-page/wk2-page ?
>>>>
>>>> It's not WK1-page vs WK2-page, because a sub-target of "web-page" is
"page" (so my bad for suggesting that). It really just mirrors the type of
object that we're directly connecting to (`WebCore::Page` vs
`WebKit::WebPage`). The same is true of the other types too (e.g.
`JSC::JSContext`).
>>>
>>> The reason I referred to them as WK1 and WK2 is simply because that's where
they're used (e.g. you won't see a "web-page" when trying to inspect something
using WK1).
>>
>> I had an impression that WebPage is WK2 concept used to communicate with its
proxy in UIProcess and it doesn't exist in WK1.
>>
>> Is there any significant difference between page and web-page except for the
Target domain?
>
> As far as the protocol, I don't think there are any other differences, but
there could be, especially since we support a "featureGuard" concept.
>
> There can be implicit differences, however, like WK2 having the concept of
user interaction (different from user gesture) <https://webkit.org/b/197269>.
Correct, today there are no differences.
A "web-page" with a Target domain today just holds a top level Page target
(which is a "page" target) and transitions between pages.
One step forward would be extending that Target domain to include another top
level object related to the page, e.g. a Service Worker. In this scenario both
the Page and Service Worker are just normal "page" and "service-worker"
targets:
Target
|
/ \
/ \
Page Service Worker
Another step forward, would be when we have a process per-frame, there will
likely still be the concept of a Page and a new concept of a Frame target:
Target
|---------------------------
/ \ \
/ \ |
Page Page (provisional) Service Worker
|
/ \
/ \
Frame Frame
| |
| |
Frame Worker
We haven't yet mapped this out in complete detail yet. For example in this
scenario if the Page with out-of-process frames will need to be a different
Page type or not. I don't think we'll know that until that implementation comes
closer to reality.
More information about the webkit-reviews
mailing list