[webkit-reviews] review granted: [Bug 201262] Web Inspector: Local Overrides - Provide substitution content for resource loads (URL based) : [Attachment 377943] [PATCH] Proposed Fix
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Sep 4 12:36:01 PDT 2019
Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 201262: Web Inspector: Local Overrides - Provide substitution content for
resource loads (URL based)
https://bugs.webkit.org/show_bug.cgi?id=201262
Attachment 377943: [PATCH] Proposed Fix
https://bugs.webkit.org/attachment.cgi?id=377943&action=review
--- Comment #30 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 377943
--> https://bugs.webkit.org/attachment.cgi?id=377943
[PATCH] Proposed Fix
View in context: https://bugs.webkit.org/attachment.cgi?id=377943&action=review
r=me, this is gonna be so awesome :D
Just to make sure, does all of this play nicely with inspector stylesheet
"resources"?
>
LayoutTests/http/tests/inspector/network/local-resource-override-main-resource.
html:18
> + content: `<!DOCTYPE html><html><head><script
src="../resources/inspector-test.js"></`+`script></head><body><p>Overridden
page content</p><script>alert("REPLACED HTML CONTENT");
TestPage.completeTest();</`+`script></body></html>`,
Do we actually need the `TestPage.completeTest()`, since you've already called
`suite.runTestCasesAndFinish()`? Shouldn't this finish "naturally" once the
`await InspectorTest.reloadPage(...);` returns? Should we follow the same
`TestPage.dispatchEventToFrontend` that some of the other tests do?
>
LayoutTests/http/tests/inspector/network/resource-response-inspector-override.h
tml:21
> + test(resolve, reject) {
Make this an `async` test?
> Source/JavaScriptCore/inspector/protocol/Network.json:156
> + "description": "Stage of the network request to intercept.",
This description suggests that `NetworkStage` is meant to be used for
interception only. In that case, how about we call it `InterceptStage`?
Either is fine by me though :)
> Source/JavaScriptCore/inspector/protocol/Network.json:241
> + { "name": "stage", "$ref": "NetworkStage", "optional": true,
"description": "If not present this applies to all Request Stages" }
I kinda feel like we shouldn't allow this to be optional. I understand that
we're treating not providing a `stage` as if it was a "response" value (given
that request interception is a FIXME right now), but my initial reaction to
having this be optional would be that not providing it means _every_ stage
would get intercepted, not just the "response". I don't think there's a
situation where we'd want to add an interception without knowing _when_ we'd
want that interception to "trigger".
Style: missing period at end of description.
> Source/JavaScriptCore/inspector/protocol/Network.json:249
> + { "name": "stage", "$ref": "NetworkStage", "optional": true,
"description": "If not present this applies to all Request Stages" }
Ditto (241).
> Source/JavaScriptCore/inspector/protocol/Network.json:350
> + { "name": "requestId", "$ref": "RequestId", "description":
"Identifier for this intercepted network. Corresponds with an earlier
`Network.requestWillBeSent`." },
"Identifier for the intercepted network request. Corresponds with an earlier
<code>Network.requestWillBeSent<code>."
> Source/JavaScriptCore/inspector/protocol/Network.json:351
> + { "name": "response", "$ref": "Response", "description":
"Response content that would proceed if this is continued." }
"Original response content that will be used if this intercept is continued
(<code>Network.interceptContinue</code>)."
> Source/WebCore/inspector/InspectorInstrumentation.h:1627
>
Style: unnecessary newline (you could inline the `++` in the `if` as well).
> Source/WebCore/inspector/InspectorInstrumentation.h:1636
>
Style: unnecessary newline (you could inline the `--` in the `if` as well).
> Source/WebCore/inspector/InspectorInstrumentationWebKit.h:52
> +
Style: unnecessary newline.
> Source/WebCore/inspector/InspectorInstrumentationWebKit.h:53
> + return shouldInterceptResponseInternal(*const_cast<Frame*>(frame),
response);
I think we can avoid the `const_cast`. It seems like all we use the `Frame`
for is to get the `InstrumentingAgents`, but all of the "getters" that we use
are all marked `const`, so we could use a `const` value instead.
> Source/WebCore/inspector/InspectorInstrumentationWebKit.h:59
> + interceptResponseInternal(*const_cast<Frame*>(frame), response,
identifier, WTFMove(handler));
Ditto (53).
> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1113
> +
Style: extra newline.
> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1124
> + String headerValue;
> + if (value->asString(headerValue))
I really wish we had versions of these that returned an `Optional` instead :(
> Source/WebInspectorUI/UserInterface/Base/HTTPUtilities.js:29
> + switch (code) {
We should probably have a `parseInt` (or at least a `console.assert` that it's
an integer).
> Source/WebInspectorUI/UserInterface/Base/URLUtilities.js:113
> + if (isWebKitInternalScript(url))
Do we have a test for this?
> Source/WebInspectorUI/UserInterface/Base/URLUtilities.js:301
> + // URL.toString with an empty hash leaves the hash symbol, so we
strip it.
When can this happen? Shouldn't this be handled by the above `if`?
> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:58
> + WI.Resource.addEventListener(WI.SourceCode.Event.ContentDidChange,
this._handleResourceContentDidChange, this);
> +
WI.LocalResourceOverride.addEventListener(WI.LocalResourceOverride.Event.Disabl
edChanged, this._handleResourceOverrideDisabledChanged, this);
These event listeners should also be inside the `if
(NetworkManager.supportsLocalResourceOverrides()) {` as they don't really do
anything otherwise.
> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:237
> + this._localResourceOverrideMap.set(localResourceOverride.url,
localResourceOverride);
We should also assert that we don't already have a local resource override for
the url.
> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:244
> + if (target.NetworkAgent &&
target.NetworkAgent.addInterception)
This should have a compatibility comment.
> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:257
> + return;
I'd add an "assert not reached" here, as we shouldn't ever remove a local
resource override that wasn't previously known.
> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:267
> + if (target.NetworkAgent &&
target.NetworkAgent.removeInterception)
Ditto (244).
> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:815
> +
Style: unnecessary newline.
> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:38
> WI.LocalResource = class LocalResource extends WI.Resource
Now that <https://webkit.org/b/17240> has landed, you'll also want to implement
a `get supportsScriptBlackboxing`, which would just `return false;`.
> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:287
> +
this.dispatchEventToListeners(WI.LocalResource.Event.OverrideContentChanged);
Was this going to be changed (you mentioned image support)? If this is coming
later, I'd rather it be added then, just so we don't accidentally "forget"
about it (plus it keeps things more contained/clean).
> Source/WebInspectorUI/UserInterface/Models/Resource.js:433
> + isLocalResourceOverride()
I think we should just make this a getter. <https://webkit.org/b/17240>
already added an `get isScript` and `get supportsScriptBlackboxing`, so there's
some "precedent".
> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:392
> + startEditingNode(node)
In your screenshots, it looked like this added a weird border/outline to the
outside of the cell when it's being edited. We should avoid that.
>
Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideLabelView.css:51
> + background-color: hsl(44, 83%, 49%);
Perhaps we should use a `--yellow-highlight-selected-background-color` or
`--yellow-highlight-glyph-color-active`? I'm not a fan of adding variables all
over the place, but given how specific this color is, having some sort of
"name" or "identifier" would make it easier to reason about how it would look
just by reading the code (I had to load a page and preview this color to see
where it fit into the UI).
>
Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideLabelView.css:61
> + background-color: hsl(41, 74%, 38%);
Ditto (51).
>
Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:5
6
> + this._localResourceOverride.removeEventListener(null, null, this);
I'm starting to feel like this "convenience" is actually not as good as it may
seem. It could potentially destroy listeners added by any super-class, which
may not be expected. Also, if we change our event listener system to go back
to an array of objects (<https://webkit.org/b/196956>), this won't be as
performant.
I think we should move in the direction of not using `null, null, this` unless
there's a large number of event listeners on the same object (e.g. like for a
manager).
>
Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:7
2
> + return !this.status.contains(event.target);
Now that <https://webkit.org/b/17240> has landed, please also include a
`super.canSelectOnMouseDown(event)` call too.
>
Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:1
35
> + _updateStatusCheckbox()
Should we also override `updateStatus` to do nothing, so that the <input> never
get's replaced?
>
Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideWarningView.js:4
5
> +
WI.networkManager.addEventListener(WI.NetworkManager.Event.LocalResourceOverrid
eAdded, this._handleLocalResourceOverrideAddedOrRemoved, this);
I just wanna say, this is how `WI.View` should be used, and it's awesome :D
>
Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideWarningView.js:6
4
> + this._revealButton =
container.insertBefore(document.createElement("button"), container.firstChild);
Rather than use `insertBefore`, you could use `append`.
```
this._revealButton = document.createElement("button");
...
container.append(this._revealButton, WI.UIString("This Resource came from a
Local Resource Override"));
```
> Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js:176
> + let mainFrameHost = WI.networkManager.mainFrame &&
WI.networkManager.mainFrame.mainResource ?
WI.networkManager.mainFrame.mainResource.urlComponents.host : null;
NIT: I'd wrap the condition in `(...)` so it's clearly distinct from the
ternary.
> Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js:181
> + let subtitle = parentResourceHost !== urlComponents.host ||
frame && frame.isMainFrame() && isMainResource ?
WI.displayNameForHost(urlComponents.host) : null;
Ditto (176).
>
Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1841
> + if (WI.NetworkManager.supportsLocalResourceOverrides()) {
Should we put this inside the "create resource" context menu instead?
> Source/WebKit/WebProcess/Network/WebResourceInterceptController.cpp:39
> + m_interceptedResponseQueue.set(identifier, Deque<Function<void()>>());
Please use `{ }` instead of the specific type declaration.
> Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:42
> +#include <WebCore/InspectorInstrumentationWebKit.h>
This seems to be failing on GTK and WPE :(
More information about the webkit-reviews
mailing list