Bug 201262 - Web Inspector: Local Overrides - Provide substitution content for resource loads (URL based)
Summary: Web Inspector: Local Overrides - Provide substitution content for resource lo...
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: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks: 201489 202375 204330
  Show dependency treegraph
 
Reported: 2019-08-28 17:47 PDT by Joseph Pecoraro
Modified: 2019-11-18 16:34 PST (History)
18 users (show)

See Also:


Attachments
[IMAGE] Light - Network (1.20 MB, image/png)
2019-08-28 17:47 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Light - Local Resource Override (1.09 MB, image/png)
2019-08-28 17:48 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Light - Overridden Resource (1.10 MB, image/png)
2019-08-28 17:48 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Light - Edit Override Popover (1.07 MB, image/png)
2019-08-28 17:48 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Light - Timelines (1.16 MB, image/png)
2019-08-28 17:49 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Dark - Network (1.26 MB, image/png)
2019-08-28 17:49 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Dark - Local Resource Override (1.11 MB, image/png)
2019-08-28 17:49 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Dark - Overridden Resource (1.12 MB, image/png)
2019-08-28 17:49 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Dark - Edit Override Popover (1.09 MB, image/png)
2019-08-28 17:50 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Dark - Timelines (1.25 MB, image/png)
2019-08-28 17:50 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Tool - CodeMirrorModes (335.41 KB, image/png)
2019-08-28 17:51 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (262.64 KB, patch)
2019-08-28 19:45 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (266.08 KB, patch)
2019-08-28 23:07 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (276.81 KB, patch)
2019-08-29 00:19 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (266.18 KB, patch)
2019-08-29 00:23 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (268.64 KB, patch)
2019-08-29 00:34 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (283.45 KB, patch)
2019-08-30 00:52 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (283.74 KB, patch)
2019-08-30 00:59 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (283.48 KB, patch)
2019-09-03 16:46 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (283.71 KB, patch)
2019-09-03 17:50 PDT, Joseph Pecoraro
drousso: review+
Details | Formatted Diff | Diff
[PATCH] For Bots (284.60 KB, patch)
2019-09-04 11:52 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Landing (287.27 KB, patch)
2019-09-04 13:52 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Landing (287.39 KB, patch)
2019-09-04 15:10 PDT, Joseph Pecoraro
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-08-28 17:47:01 PDT
Web Inspector: Local Overrides - Provide substitution content for resource loads (URL based)
Comment 1 Joseph Pecoraro 2019-08-28 17:47:12 PDT
<rdar://problem/13108764>
Comment 2 Joseph Pecoraro 2019-08-28 17:47:48 PDT
Created attachment 377521 [details]
[IMAGE] Light - Network
Comment 3 Joseph Pecoraro 2019-08-28 17:48:07 PDT
Created attachment 377522 [details]
[IMAGE] Light - Local Resource Override
Comment 4 Joseph Pecoraro 2019-08-28 17:48:21 PDT
Created attachment 377523 [details]
[IMAGE] Light - Overridden Resource
Comment 5 Joseph Pecoraro 2019-08-28 17:48:51 PDT
Created attachment 377524 [details]
[IMAGE] Light - Edit Override Popover
Comment 6 Joseph Pecoraro 2019-08-28 17:49:06 PDT
Created attachment 377525 [details]
[IMAGE] Light - Timelines
Comment 7 Joseph Pecoraro 2019-08-28 17:49:21 PDT
Created attachment 377526 [details]
[IMAGE] Dark - Network
Comment 8 Joseph Pecoraro 2019-08-28 17:49:36 PDT
Created attachment 377527 [details]
[IMAGE] Dark - Local Resource Override
Comment 9 Joseph Pecoraro 2019-08-28 17:49:50 PDT
Created attachment 377528 [details]
[IMAGE] Dark - Overridden Resource
Comment 10 Joseph Pecoraro 2019-08-28 17:50:01 PDT
Created attachment 377529 [details]
[IMAGE] Dark - Edit Override Popover
Comment 11 Joseph Pecoraro 2019-08-28 17:50:11 PDT
Created attachment 377530 [details]
[IMAGE] Dark - Timelines
Comment 12 Joseph Pecoraro 2019-08-28 17:51:44 PDT
Created attachment 377532 [details]
[IMAGE] Tool - CodeMirrorModes
Comment 13 Joseph Pecoraro 2019-08-28 19:45:41 PDT
Created attachment 377538 [details]
[PATCH] Proposed Fix

I'm missing a test for `Network.interceptContinue`. I'm trying to write a ProtocolTest for that and having issues in LayoutTests/http/tests/inspector/network. So I'll put this up now while I work that out.
Comment 14 Build Bot 2019-08-28 19:46:44 PDT Comment hidden (obsolete)
Comment 15 Joseph Pecoraro 2019-08-28 23:07:41 PDT
Created attachment 377547 [details]
[PATCH] Proposed Fix
Comment 16 Joseph Pecoraro 2019-08-28 23:09:01 PDT
(In reply to Joseph Pecoraro from comment #13)
> Created attachment 377538 [details]
> [PATCH] Proposed Fix
> 
> I'm missing a test for `Network.interceptContinue`. I'm trying to write a
> ProtocolTest for that and having issues in
> LayoutTests/http/tests/inspector/network. So I'll put this up now while I
> work that out.

I wasn't able to figure out how ProtocolTest's were failing in http/tests/. Seems like the dummy Web Inspector frontend just never loads resources even if  I had granted universal access. I rewrote the test to work in LayoutTests/inspector/network, which is a bit ugly but still small enough to be okay.
Comment 17 Joseph Pecoraro 2019-08-29 00:19:10 PDT Comment hidden (obsolete)
Comment 18 Joseph Pecoraro 2019-08-29 00:23:10 PDT Comment hidden (obsolete)
Comment 19 Joseph Pecoraro 2019-08-29 00:34:29 PDT Comment hidden (obsolete)
Comment 20 Devin Rousso 2019-08-29 14:39:55 PDT Comment hidden (obsolete)
Comment 21 Joseph Pecoraro 2019-08-29 22:55:19 PDT Comment hidden (obsolete)
Comment 22 Devin Rousso 2019-08-29 23:34:11 PDT
Comment on attachment 377555 [details]
[PATCH] Proposed Fix

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

>>> LayoutTests/http/tests/inspector/network/local-resource-override-basic-expected.txt:61
>>> +Content: PASS
>> 
>> I like the idea of using "PASS" as a way of representing a successful override.  Perhaps we can also use this as the value for overridden headers/content above?
> 
> The tests above are to show that we can provide arbitrary data. The other tests are just pass/fail for overrides happening.

Sorry, I should've been clearer.  I meant to prefix the content with the word "PASS", so instead of "OVERRIDDEN TEXT" we could use "PASS - OVERRIDDEN TEXT" or something like that, so it's much more immediately obvious that this is the expected/passing output.

>>> LayoutTests/http/tests/inspector/network/local-resource-override-basic.html:42
>>> +                // Create overrides.
>> 
>> Rather than make these comments, why not actually include them in the test output?  That way, if we encounter issues, we get more logging and can better see the progress of the test until it failed.
>> ```
>>     InspectorTest.log("Creating overrides...");
>> ```
> 
> Often it makes the output harder to read. The correct way to debug this is `InspectorTest.debug()` if anything goes wrong, in which case the protocol traffic will be clear.
> 
> In this case I like the idea of a log per-override created so tests that have a few overrides it will be nice to see both.

I agree that it adds extra (possibly unnecessary) logging, but I've found that `InspectorTest.debug()` only helps for local debugging, not debugging issues from the bots.  Adding this extra logging makes it _much_ easier to understand at what point something went wrong with the bots, especially if the test failure doesn't reproduce locally.

>>> LayoutTests/http/tests/inspector/network/local-resource-override-main-resource.html:36
>>> +<script>alert("ORIGINAL HTML CONTENT");</script>
>> 
>> NIT: should we `alert` or just `console.log`?  Using an `alert` makes me think almost like an "ERROR".
> 
> console.log output won't get carried over across page loads. The alert does.

Interesting!  I didn't know that =D

>>> LayoutTests/platform/mac-wk1/TestExpectations:502
>>> +http/tests/inspector/network/resource-response-inspector-override.html [ Failure ]
>> 
>> Why not just `Skip`?
> 
> Normally we run the code to ensure we don't introduce crashes on WK1.

Oooo good point!

>>> Source/WebInspectorUI/UserInterface/Base/HTTPUtilities.js:32
>>> +    case 100: return "Continue";
>> 
>> Should we explicitly mark these as `WI.unlocalizedString`?
> 
> Unlocalized string hasn't proven to be very useful =(

The one nice thing I find about it is that it makes it immediately clear when a string is meant to be used in the UI vs as an ident (e.g. CSS class name).

>>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:128
>>> +                        target.NetworkAgent.addInterception(localResourceOverride.url, "response");
>> 
>> Since the second parameter doesn't even do anything right now, why don't we just remove it entirely?
> 
> Local Overrides will always be the response. They won't override requests. So if we add a new network stage this will continue to work as expected.

Got it.  Rather than use the literal `"response"`, could we use `InspectorBackend.domains.Network.NetworkPhase.Response` (or whatever name you decide) instead?

>>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:248
>>> +        this.dispatchEventToListeners(WI.NetworkManager.Event.LocalResourceOverrideAdded, {localResourceOverride});
>> 
>> Style: drop the `WI.` since we're inside this class.
> 
> For events in particular it is easier to search for the entire string. I could go either way on this one.

I typically don't search for things with the `WI.` prefix (unless I'm looking for a creation, like `new WI.*`), but like you said I could go either way.  Your call.

>>> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:322
>>> +            treeElement.revealAndSelect(omitFocus, selectedByUser, suppressNotification);
>> 
>> This is far reaching and therefore scary.  Can you explain why this is needed?
> 
> The NavigationSidebar has a `TreeOutlineGroup` that tries to keep 1 row selected between all tree outlines. It does this on `WI.TreeOutline.Event.SelectionDidChange` events. When using `showRepresentedObject` to reveal and select a new LocalResourceOverride this event was not getting dispatched and the sidebar would end up with multiple selections (a selection in the local overrides tree outline and the other tree outline). I agree it is far reaching. I haven't seen any regressions from this yet, but I can test around a bit more.

That sounds reasonable.  I can try living with this for a bit as well.

>>> Source/WebInspectorUI/UserInterface/Views/ContextMenu.js:-252
>>> -            this._handlers[id].call(this);
>> 
>> What caused this?
> 
> I wrote a ContextMenu handler that was throwing an exception but it was getting swallowed and not shown anywhere.
> 
> This just made it throw up an uncaught exception dialog.

Ah.  I'm fine with this change btw.  Was just curious as to why it was included in this patch :)

>>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.css:79
>>> +}
>> 
>> Why so specific?
> 
> Monospace, assuming 12px per letter. These are all multiples of 12. Status code typically has 3 numbers so 36px.

Ah!  Got it.  That makes sense :)

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:56
> +            return false;

This return value doesn't match the others :(

>>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:269
>>> +        this._statusCodeCodeMirror.addKeyMap({
>> 
>> NIT: please move this after the `this._mimeTypeCodeMirror` event handlers to match the order in which the UI is created.  Alternatively, you could add these event handlers immediately after the `CodeMirror` is created above.
> 
> I put all the event handler code together at the bottom because it may need to deal with all elements after construction. In general there is no right or wrong way to build the UI code so I don't think we have to be as picky about it as long as it is reasonable.

I agree.  I usually just try to bundle/batch related things together as much as I can so you don't have to jump around when trying to see all the things that a particular part of the UI can do (class-defined event listeners are already hard enough to grok when the element is way up at the top).

>>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:334
>>> +        codeMirror.addKeyMap({
>> 
>> Can we have "Tab"/"Shift-Tab" select the next/previous editor?
> 
> That is what the `extraKeys: {Tab: false, ...}` above does!

Oh nice!

>>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:98
>>> +            WI.showRepresentedObject(newLocalResourceOverride, cookie, options);            
>> 
>> This means that if the user edits the URL, we'll automatically show the local override resource?  That seems a bit antagonistic, especially if the user is just trying to make a "quick fix" to a URL they typed incorrectly.
> 
> This is only if the current tree element `wasSelected` it selects the new tree element.

Doh!  My mistake.

>> Source/WebInspectorUI/UserInterface/Views/ResourceIcons.css:31
>> +    filter: invert();
> 
> <3

Oh, random thought, have you tried adding a `hue-rotate`?

You could use `var(--filter-invert)` instead =D

>>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:668
>>> +            InspectorFrontendHost.beep();
>> 
>> Should we beep in this case?  What about if the user "accidentally" creates a local override and wants to "cancel" it (e.g. escape, clicking outside, etc.)?
> 
> The popover doesn't currently distinguish between a "Cancel" and a "Commit". We could have a "Save" button or make "Esc" dismiss the popover with a different delegate, but normal system behavior is edits are committed in these cases (breakpoints popovers).

So if the user opens the popover and then immediately dismisses it without any changes, we get a beep (since the old `serializedData` will be "deeply" equal to the new `serializedData`, which causes the getter to return `null`)?  That seems unnecessary :(
Comment 23 Joseph Pecoraro 2019-08-30 00:17:42 PDT
> > The tests above are to show that we can provide arbitrary data. The other tests are just pass/fail for overrides happening.
> 
> Sorry, I should've been clearer.  I meant to prefix the content with the
> word "PASS", so instead of "OVERRIDDEN TEXT" we could use "PASS - OVERRIDDEN
> TEXT" or something like that, so it's much more immediately obvious that
> this is the expected/passing output.

Good idea!

> >>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:128
> >>> +                        target.NetworkAgent.addInterception(localResourceOverride.url, "response");
> >> 
> >> Since the second parameter doesn't even do anything right now, why don't we just remove it entirely?
> > 
> > Local Overrides will always be the response. They won't override requests. So if we add a new network stage this will continue to work as expected.
> 
> Got it.  Rather than use the literal `"response"`, could we use
> `InspectorBackend.domains.Network.NetworkPhase.Response` (or whatever name
> you decide) instead?

Good point! I totally overlooked that, will change.


> > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:56
> > +            return false;
> 
> This return value doesn't match the others :(

Oops! Good catch.


> >> Source/WebInspectorUI/UserInterface/Views/ResourceIcons.css:31
> >> +    filter: invert();
> > 
> > <3
> 
> Oh, random thought, have you tried adding a `hue-rotate`?
> 
> You could use `var(--filter-invert)` instead =D

I hadn't tried that! I actually really like the invert as is. The only one that looks a bit ugly so far has been images. When adding support for images I may hue rotate that. The other filter that I really liked was:

    filter: invert(100%) hue-rotate(260deg)

We can compare these together and pick a good one.


> >>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:668
> >>> +            InspectorFrontendHost.beep();
> >> 
> >> Should we beep in this case?  What about if the user "accidentally" creates a local override and wants to "cancel" it (e.g. escape, clicking outside, etc.)?
> > 
> > The popover doesn't currently distinguish between a "Cancel" and a "Commit". We could have a "Save" button or make "Esc" dismiss the popover with a different delegate, but normal system behavior is edits are committed in these cases (breakpoints popovers).
> 
> So if the user opens the popover and then immediately dismisses it without
> any changes, we get a beep (since the old `serializedData` will be "deeply"
> equal to the new `serializedData`, which causes the getter to return
> `null`)?  That seems unnecessary :(

This only beeps when creating a new override and dismissing the dialog (SourcesNavigationSidebarPanel).
This does not beep when editing an override and nothing is changed (LocalResourceOverrideTreeElement).

I beep in the new case because the popover shows and is dismissed but an override was not actually created. Note that this assumes that `http://example.com/path` 200, OK, javascript (the default popover parameters) is never intentional. We could allow that (if the popover was shown without a localResource then set the initial serialized data to null) but I think if someone hasn't made edits they probably wanted to cancel.

I don't really have a strong preference for the immediate Cancel / Commit case. I did consider and desire a "Save" button but I matched our existing popover's which just seem to perform actions on dismiss. I could change this more generally, but that refinement can be done in a follow-up.
Comment 24 Joseph Pecoraro 2019-08-30 00:52:38 PDT Comment hidden (obsolete)
Comment 25 Joseph Pecoraro 2019-08-30 00:59:09 PDT
Created attachment 377686 [details]
[PATCH] Proposed Fix
Comment 26 Alex Christensen 2019-08-30 08:55:59 PDT
Comment on attachment 377686 [details]
[PATCH] Proposed Fix

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

> Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:209
> +        Ref<WebResourceLoader> protectedThis(*this);
> +        m_interceptController.defer(m_coreLoader->identifier(), [this, protectedThis = WTFMove(protectedThis), networkLoadMetrics]() mutable {

You can use makeRef(*this) in the lambda capture to avoid the unnecessary local variable declaration and move.
Comment 27 Joseph Pecoraro 2019-09-03 16:46:51 PDT
Created attachment 377934 [details]
[PATCH] Proposed Fix
Comment 28 Joseph Pecoraro 2019-09-03 17:50:01 PDT
Created attachment 377943 [details]
[PATCH] Proposed Fix
Comment 29 Joseph Pecoraro 2019-09-04 11:52:14 PDT
Created attachment 377995 [details]
[PATCH] For Bots
Comment 30 Devin Rousso 2019-09-04 12:36:01 PDT
Comment on attachment 377943 [details]
[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.html: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.DisabledChanged, 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:56
> +        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:72
> +        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:135
> +    _updateStatusCheckbox()

Should we also override `updateStatus` to do nothing, so that the <input> never get's replaced?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideWarningView.js:45
> +        WI.networkManager.addEventListener(WI.NetworkManager.Event.LocalResourceOverrideAdded, 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:64
> +        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 :(
Comment 31 Joseph Pecoraro 2019-09-04 13:42:34 PDT
Comment on attachment 377943 [details]
[PATCH] Proposed Fix

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

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

The test doesn't finish naturally on its own. Not exactly sure why, but I like the simplicity of the test just ending here.

>> LayoutTests/http/tests/inspector/network/resource-response-inspector-override.html:21
>> +            test(resolve, reject) {
> 
> Make this an `async` test?

Same reason as before.

>> 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 :)

I thought of a reason to keep this NetworkStage if we ever have to do anything with pre-load / pre-connect stage of network requests. I'll update the description to be more generic.

>> 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.

Yeah, we will have to revisit this when we support intercepting requests.

I can totally see something say "pause every time this network request advances: dns, outgoing request, did receive response, did receive data, did finish loading, ...".

>> Source/WebInspectorUI/UserInterface/Base/URLUtilities.js:113
>> +    if (isWebKitInternalScript(url))
> 
> Do we have a test for this?

I'll add a few:

    testInvalid("__WebInspectorInternal__");
    testInvalid("__WebTest__");

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

If `url.hash` is the empty string (which is indistinguishable from a string without an empty string) then the if above would have fallen through to here:

Example:

    url = new URL("http://example.com/foo#");
    url.hash; // ""
    url.toString(); // "http://example.com/foo#"
    WI.urlWithoutFragment(url); // "http://example.com/foo"

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:244
>> +                if (target.NetworkAgent && target.NetworkAgent.addInterception)
> 
> This should have a compatibility comment.

I'm not sure a compatibility comment gains us much here. We'll always want to check for NetworkAgent support in loops where we are iterating over all agents, and `supports`. I'll add it though, just in case.

>> 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).

Okay, I'll remove this now.

>> 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).

These are used in one place and visible in the screenshots. If I made a variable it would be okay but it wouldn't be much more semantic than the selector itself other than knowing it is "yellow".

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:56
>> +        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).

In general this is not a problem because of the `this` part of `null, null, this`. For super-classes that is when you need to worry, though often we remove event listeners in a `dispose` like opportunity when it doesn't matter.

I've updated these cases.

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:72
>> +        return !this.status.contains(event.target);
> 
> Now that <https://webkit.org/b/17240> has landed, please also include a `super.canSelectOnMouseDown(event)` call too.

Good call!

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:135
>> +    _updateStatusCheckbox()
> 
> Should we also override `updateStatus` to do nothing, so that the <input> never get's replaced?

Sure, then we can include a comment to that effect.

>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1841
>> +        if (WI.NetworkManager.supportsLocalResourceOverrides()) {
> 
> Should we put this inside the "create resource" context menu instead?

My mind considers the (+) in the bottom left as only affecting the bottom sections below the mid-navigation bar (the resources) and the (+) in the top right as affecting the top sections above the navigation bar.

We can experiment here with future changes, especially if we receive feedback.

>> Source/WebKit/WebProcess/Network/WebResourceInterceptController.cpp:39
>> +    m_interceptedResponseQueue.set(identifier, Deque<Function<void()>>());
> 
> Please use `{ }` instead of the specific type declaration.

That didn't compile, so I left this as is.
Comment 32 Joseph Pecoraro 2019-09-04 13:52:50 PDT
Created attachment 378012 [details]
[PATCH] For Landing
Comment 33 Joseph Pecoraro 2019-09-04 15:10:49 PDT
Created attachment 378016 [details]
[PATCH] For Landing
Comment 34 WebKit Commit Bot 2019-09-04 16:27:46 PDT
Comment on attachment 378016 [details]
[PATCH] For Landing

Rejecting attachment 378016 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 378016, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
Builder.js
patching file Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js
patching file Source/WebInspectorUI/UserInterface/Images/NavigationItemNetworkOverride.svg
patching file Source/WebInspectorUI/UserInterface/Main.html
patching file Source/WebInspectorUI/UserInterface/Models/LocalResource.js
patching file Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js
patching file Source/WebInspectorUI/UserInterface/Models/Resource.js
patching file Source/WebInspectorUI/UserInterface/Protocol/NetworkObserver.js
patching file Source/WebInspectorUI/UserInterface/Test.html
patching file Source/WebInspectorUI/UserInterface/Views/BreakpointPopoverController.css
patching file Source/WebInspectorUI/UserInterface/Views/CodeMirrorLocalOverrideURLMode.css
patching file Source/WebInspectorUI/UserInterface/Views/CodeMirrorLocalOverrideURLMode.js
patching file Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.css
patching file Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/ContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/ContextMenu.js
patching file Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js
patching file Source/WebInspectorUI/UserInterface/Views/DataGrid.js
patching file Source/WebInspectorUI/UserInterface/Views/DataGridNode.js
patching file Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.css
patching file Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js
patching file Source/WebInspectorUI/UserInterface/Views/InputPopover.css
patching file Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideLabelView.css
patching file Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideLabelView.js
patching file Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.css
patching file Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js
patching file Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.css
patching file Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js
patching file Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideWarningView.css
patching file Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideWarningView.js
patching file Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js
patching file Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/ResourceIcons.css
patching file Source/WebInspectorUI/UserInterface/Views/ResourceSizesContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js
patching file Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js
patching file Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.css
patching file Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.css
patching file Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js
patching file Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js
patching file Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.css
patching file Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js
patching file Source/WebInspectorUI/UserInterface/Views/SourcesTabContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/TextEditor.css
patching file Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.css
patching file Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/TimelineDataGridNode.js
patching file Source/WebInspectorUI/UserInterface/Views/URLBreakpointPopover.css
patching file Source/WebInspectorUI/UserInterface/Views/URLBreakpointPopover.js
patching file Source/WebInspectorUI/UserInterface/Views/Variables.css
patching file Source/WebKit/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/Sources.txt
patching file Source/WebKit/WebKit.xcodeproj/project.pbxproj
patching file Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp
Hunk #1 succeeded at 757 (offset 8 lines).
patching file Source/WebKit/WebProcess/Network/WebResourceInterceptController.cpp
patching file Source/WebKit/WebProcess/Network/WebResourceInterceptController.h
patching file Source/WebKit/WebProcess/Network/WebResourceLoader.cpp
patching file Source/WebKit/WebProcess/Network/WebResourceLoader.h
Hunk #1 FAILED at 28.
Hunk #2 succeeded at 83 (offset 2 lines).
Hunk #3 succeeded at 95 (offset 2 lines).
1 out of 3 hunks FAILED -- saving rejects to file Source/WebKit/WebProcess/Network/WebResourceLoader.h.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/12997734
Comment 35 Build Bot 2019-09-04 17:25:06 PDT
Comment on attachment 378016 [details]
[PATCH] For Landing

Attachment 378016 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12997617

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases
Comment 36 Joseph Pecoraro 2019-09-06 01:16:51 PDT
https://trac.webkit.org/changeset/249504/webkit