[webkit-reviews] review granted: [Bug 191344] Web Inspector: Restrict domains at the target level instead of only at the window level : [Attachment 354142] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 191344: Web Inspector: Restrict domains at the target level instead of only
at the window level
https://bugs.webkit.org/show_bug.cgi?id=191344

Attachment 354142: [PATCH] Proposed Fix

https://bugs.webkit.org/attachment.cgi?id=354142&action=review




--- Comment #12 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 354142
  --> https://bugs.webkit.org/attachment.cgi?id=354142
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=354142&action=review

r=me, with a few comments/concerns

> Source/WebCore/inspector/WorkerInspectorController.cpp:82
> +	   commandLineAPIHost->init(nullptr,
m_instrumentingAgents->webConsoleAgent(), nullptr, nullptr, nullptr);

This means that we won't be able to ever `inspect` anything.  I'm guessing that
that's fine for now, as the only thing we are able to `inspect` inside a worker
is a "function" (no DOM nodes, databases, or DOM storages).

> Source/WebInspectorUI/ChangeLog:56
> +	   Regenerate protocol files now that workerSupportedDomains is
unnecessary
> +	   and explicit availability has been added to other domains.

Should we bother updating the availability of protocol files before iOS 10.3
(e.g. setting "Inspector" to "web" only), or is that not really an issue since
worker support was introduced in iOS 10.3?

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:179
> +	   return this._supportedDomainsForDebuggableType.get(type);

Maybe add an assertion here?

    console.assert(Object.values(WI.DebuggableType).includes(type), "Unknown
debuggable type", type);

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:46
> +	   this._agents = {};

NIT: I know that this isn't an issue right now, but the ordering of this (set
connection target, then set agents) scares me a bit, especially since `target`
is a setter.  I'd personally rather put the connection setter below the agents
initialization.

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:49
> +	       this._agents[domain] = connection._agents[domain];

Style: I try to use the member variable whenever possible, even in the
constructor, so that there is as little chance for side-effect changes as
possible.

    this._agents[domain] = this._connection._agents[domain];