Bug 201262 – Web Inspector: Local Overrides - Provide substitution content for resource loads (URL based)

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Web Inspector: Local Overrides - Provide substitution content for resource loads (URL based)

[Joseph Pecoraro]

<rdar://problem/13108764>

[Joseph Pecoraro]

Created attachment 377521 [details]
[IMAGE] Light - Network

[Joseph Pecoraro]

Created attachment 377522 [details]
[IMAGE] Light - Local Resource Override

[Joseph Pecoraro]

Created attachment 377523 [details]
[IMAGE] Light - Overridden Resource

[Joseph Pecoraro]

Created attachment 377524 [details]
[IMAGE] Light - Edit Override Popover

[Joseph Pecoraro]

Created attachment 377525 [details]
[IMAGE] Light - Timelines

[Joseph Pecoraro]

Created attachment 377526 [details]
[IMAGE] Dark - Network

[Joseph Pecoraro]

Created attachment 377527 [details]
[IMAGE] Dark - Local Resource Override

[Joseph Pecoraro]

Created attachment 377528 [details]
[IMAGE] Dark - Overridden Resource

[Joseph Pecoraro]

Created attachment 377529 [details]
[IMAGE] Dark - Edit Override Popover

[Joseph Pecoraro]

Created attachment 377530 [details]
[IMAGE] Dark - Timelines

[Joseph Pecoraro]

Created attachment 377532 [details]
[IMAGE] Tool - CodeMirrorModes

[Joseph Pecoraro]

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.

[EWS Watchlist]

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.

[Joseph Pecoraro]

Created attachment 377547 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Created attachment 377551 [details]
[PATCH] Proposed Fix

Addressed style warnings.

[Joseph Pecoraro]

Created attachment 377552 [details]
[PATCH] Proposed Fix

Oops, previous patch included partial image support!

[Joseph Pecoraro]

Created attachment 377555 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Comment on attachment 377555 [details]
[PATCH] Proposed Fix

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

Nice work!  I think this is great as is, but given the number of comments (and the lack of EWS results) I'm going to hold of on changing r?.

> LayoutTests/http/tests/inspector/network/local-resource-override-basic-expected.txt:6
> +URL: http://127.0.0.1:8000/inspector/network/resources/override.txt

Can we strip everything before the `inspector/network` in case the bots (now or in the future) use a different URL?

> LayoutTests/http/tests/inspector/network/local-resource-override-basic-expected.txt:20
> +Content: DEFAULT override.txt CONTENT

Maybe we could keep the original content lowercase and have the overridden content be uppercase, so it's more obvious.

> LayoutTests/http/tests/inspector/network/local-resource-override-basic-expected.txt:37
> +Status: 200 Super OK

LOL

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

> LayoutTests/http/tests/inspector/network/local-resource-override-basic.html:20
> +    async function dumpResource(resource) {

NIT: we normally use "log*" for these types of functions.

> 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...");
```

> LayoutTests/http/tests/inspector/network/local-resource-override-basic.html:51
> +                InspectorTest.evaluateInPage(expression);

I'd include this as part of the `Promise.all` (at the end, so it gets fired after the listeners are added) so if an error gets thrown, the test will fail with output.

> LayoutTests/http/tests/inspector/network/local-resource-override-basic.html:59
> +                let resource = resourceWasAddedEvent.data.resource;

You could inline this.

> LayoutTests/http/tests/inspector/network/local-resource-override-basic.html:70
> +        name: "LocalResourceOverride.NoOverride",

NIT: you could simplify these names to just "LocalResourceOverride.None" and "LocalResourceOverride.{Text,JavaScript,etc.}".

> LayoutTests/http/tests/inspector/network/local-resource-override-basic.html:80
> +        overrides: [{

I personally hate this syntax, especially when there's more than one item (like below).  It makes it MUCH harder to understand where the array starts/ends, and confuses types.  Please put the entire object on a separate indent (with a trailing comma)l

> LayoutTests/http/tests/inspector/network/local-resource-override-basic.html:91
> +            }
> +        }]

Style: these could have a trailing comma.

> LayoutTests/http/tests/inspector/network/local-resource-override-basic.html:118
> +            content: btoa("<data>"),

Sneaky :P

> LayoutTests/http/tests/inspector/network/local-resource-override-basic.html:201
> +        async setup() { WI.networkManager.interceptionEnabled = false; },
> +        async teardown() { WI.networkManager.interceptionEnabled = true; },

Style: please indent these so they're easier to read, and aren't confused with `() => ...` that implicitly return a value.

> LayoutTests/http/tests/inspector/network/local-resource-override-main-resource.html:25
> +            await InspectorTest.reloadPage({ignoreCache: false, revalidateAllResources: true});

If `ignoreCache` is `false`, can we remove it here?

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

> LayoutTests/http/tests/inspector/network/local-resource-override-main-resource.html:37
> +<script>TestPage.dispatchEventToFrontend("TestPageLoadComplete");</script>

So this will be ignored on the first load (`runTest` shouldn't have run yet), but will be picked up the second time?  This seems brittle.  Could we listen for various `WI.Frame.Event` or `WI.Resource.Event` instead?

> LayoutTests/http/tests/inspector/network/resource-response-inspector-override.html:21
> +            test(resolve, reject) {

`async`?

> LayoutTests/inspector/network/local-resource-override-continue-response.html:20
> +    ProtocolTest.debug();

Oops.

> LayoutTests/inspector/network/local-resource-override-continue-response.html:28
> +            InspectorProtocol.sendCommand("Network.enable", {});
> +            InspectorProtocol.sendCommand("Network.setInterceptionEnabled", {"enabled": true});

Please use `awaitCommand` instead, so the test fails with output if there's a problem.

> LayoutTests/inspector/network/local-resource-override-continue-response.html:32
> +            await ProtocolTest.evaluateInPage(`getURL()`);
> +            let event = await ProtocolTest.awaitEvent("TestURL");

Ditto (with `Promise.all`).

> LayoutTests/inspector/network/local-resource-override-continue-response.html:37
> +            ProtocolTest.evaluateInPage(`triggerOverrideLoad()`);
> +            let messageObject = await InspectorProtocol.awaitEvent({event: "Network.responseIntercepted"});

Ditto (with `Promise.all`).

> LayoutTests/inspector/network/local-resource-override-continue-response.html:42
> +            InspectorProtocol.sendCommand("Network.interceptContinue", {"requestId": messageObject.params.requestId});
> +            await InspectorProtocol.awaitEvent({event: "Network.responseReceived"});

Ditto (with `Promise.all`).

> LayoutTests/inspector/unit-tests/url-utilities-expected.txt:479
> +PASS: Removing fragment of 'http://example.com?#frag' should be 'http://example.com/?'.

Just for sanity, can we add a test where the fragment is empty?

http://example.com#
http://example.com/#
http://example.com/path#
http://example.com/path/#
...

> LayoutTests/inspector/unit-tests/url-utilities.html:565
> +            test("invalid", "invalid");

Should we test other protocols?  ftp, https, etc.

> LayoutTests/platform/mac-wk1/TestExpectations:502
> +http/tests/inspector/network/local-resource-override-basic.html [ Failure ]
> +http/tests/inspector/network/local-resource-override-main-resource.html [ Failure ]
> +http/tests/inspector/network/local-resource-override-script-tag.html [ Failure ]
> +http/tests/inspector/network/resource-response-inspector-override.html [ Failure ]

Why not just `Skip`?

> Source/JavaScriptCore/ChangeLog:9
> +        Network Interception - Local Resource Overrides

This doesn't really add anything.

> Source/JavaScriptCore/inspector/protocol/Network.json:154
> +            "id": "RequestStage",

Maybe this should be "InterceptStage" instead?  It's a bit "confusing" to me how one of the stages of a "request" is "response".  Perhaps "NetworkStage" would also work?

> Source/JavaScriptCore/inspector/protocol/Network.json:230
> +            "name": "setInterceptionEnabled",

I understand the motivation behind this command (being able to "keep" overrides but turn them all off, like breakpoints), but it seems like it adds a level of complexity that probably isn't needed (especially since there's no UI for turning this on/off).  I'd expect it to be a bit "simpler" in that if any urls have been added via `Network.addInterception`, we assume that interception is enabled.  I'd like to avoid sending so many messages to the backend unless we have a reason for needing it (e.g. some sort of UI).

> Source/JavaScriptCore/inspector/protocol/Network.json:233
> +                { "name": "enabled", "type": "boolean", "description": "Whether to enable resource interception." }

NIT: these types of descriptions I don't find useful and are often obvious from context.  I'd be fine removing it.

> Source/JavaScriptCore/inspector/protocol/Network.json:238
> +            "description": "Add an interception.",

Ditto (233).

> Source/JavaScriptCore/inspector/protocol/Network.json:246
> +            "description": "Remove an interception.",

Ditto (233).

> Source/JavaScriptCore/inspector/protocol/Network.json:265
> +                { "name": "content", "type": "string" },
> +                { "name": "base64Encoded", "type": "boolean", "description": "True, if content was sent as base64." },

This means that there's no way to have an override that ONLY modifies metadata (e.g. headers, mime type, etc.).  Do we want to support something like that?

> Source/JavaScriptCore/inspector/protocol/Network.json:347
> +            "name": "responseIntercepted",

As you alluded to in your ChangeLog, would we have a separate event for `requestIntercepted` (as well as an `interceptWithRequest` command)?

> Source/WebCore/inspector/InspectorInstrumentation.h:223
> +    static void interceptResponse(Frame&, const ResourceResponse&, unsigned long identifier, CompletionHandler<void(const ResourceResponse&, RefPtr<SharedBuffer>)>&&);

Do we need any other includes or forward declarations for these types?

> Source/WebCore/inspector/InspectorInstrumentation.h:1211
> +    if (!frame)
> +        return false;

This can be folded into the `instrumentingAgentsForFrame`, as we have a version that accepts (and checks) for a `Frame*`.

> Source/WebCore/inspector/InspectorInstrumentation.h:1212
> +    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForFrame(*frame))

`auto*`

> Source/WebCore/inspector/InspectorInstrumentation.h:1220
> +    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForFrame(frame))

Ditto (1212).

> Source/WebCore/inspector/InspectorInstrumentation.h:1228
> +    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForFrame(frame))

Ditto (1212).

> Source/WebCore/inspector/InspectorInstrumentation.h:1626
> +    InspectorInstrumentationPublic::s_frontendCounter++;

Style: pre-increment

> Source/WebCore/inspector/InspectorInstrumentation.h:1635
> +    InspectorInstrumentationPublic::s_frontendCounter--;

Style: pre-decrement

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:844
> +    for (auto& entry : m_pendingInterceptResponses)

```
    for (auto& pendingInterceptResponse : m_pendingInterceptResponses.values()
        pendingInterceptResponse->respondWithOriginalResponse();
```

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1000
> +    m_interceptionEnabled = enabled;

I'd return an error if we're already in the state described by `enabled`:
```
    if (m_interceptionEnabled == enabled) {
       errorString = m_interceptionEnabled ? "Interception already enabled"_s : "Interception already disabled"_s;
       return;
    }
```

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1009
> +    // FIXME: Support `request` RequestStage and don't just assume `response` always.
> +    UNUSED_PARAM(requestStage);

We could do some basic string parsing to make sure we're only getting an expected `requestStage` values.

```
    auto networkStage = Inspector::Protocol::InspectorHelpers::parseEnumValueFromString<Inspector::Protocol::Network::NetworkStage>(networkStageString);
    if (!networkStage) {
        errorString = makeString("Unknown networkStage: "_s, networkStageString);
        return;
    }
```

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1011
> +    m_interceptResponseURLs.add(url);

We should return an error if the `url` (and `networkStage`) have already been added.
```
    if (!m_interceptResponseURLs.add(url))
        errorString = "Intercept for given url already exists"_s;
```

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1017
> +    // FIXME: Support `request` RequestStage and don't just assume `response` always.
> +    UNUSED_PARAM(requestStage);

Ditto.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1019
> +    m_interceptResponseURLs.remove(url);

Ditto (1011).
```
    if (!m_interceptResponseURLs.remove(url))
        errorString = "Missing intercept for given url"_s;
```

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1024
> +    if (!m_enabled)

OK so this now begs the question of what does `enable`/`disable` signify.  Do we want to "limit" it to "this controls whether the agent can dispatch events to the frontend" or should it be broader to "this controls whether the agent can do anything"? 

As it stands right now, this check is unnecessary, as `m_enabled` is set at the same time as `m_instrumentingAgents.setInspectorNetworkAgent(...)`, so this cannot be called when `m_enabled` is false.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1031
> +    requestURL.removeFragmentIdentifier();

Wouldn't this modify the `URL`?  Should we `isolatedCopy` first?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1042
> +    if (!m_enabled)

Ditto (1024).

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1049
> +    responseURL.removeFragmentIdentifier();

Ditto (1031).

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1060
> +    if (!m_enabled) {

Ditto (1024).

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1065
> +    if (!m_interceptionEnabled) {

Should we instead `ASSERT` that we can't reach this?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1073
> +        handler(response, nullptr);

Should we `return` here?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1076
> +    auto pendingInterceptResponse = makeUnique<PendingInterceptResponse>(response, WTFMove(handler));

I'd just construct this inline and avoid the `WTFMove` below.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1079
> +    auto resourceResponse = buildObjectForResourceResponse(response, nullptr);

Ditto (1076).

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1080
> +    m_frontendDispatcher->responseIntercepted(requestId, WTFMove(resourceResponse));

Based on my comment on 1024, we should `ASSERT(m_enabled)` right before this.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1087
> +        errorString = "No pending response for requestId"_s;

"Missing pending intercept response for given requestId"

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1098
> +        errorString = "No pending response for requestId"_s;

Ditto (1087).

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1104
> +

Style: extra newline

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1113
> +        for (auto& entry : *headers) {

```
    for (auto [key, value] : *headers)
```

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1126
> +            errorString = "Unable to decode content"_s;

"Unable to decode given content"

> Source/WebCore/inspector/agents/InspectorNetworkAgent.h:170
> +        ResourceResponse originalResponse() { return m_originalResponse; }

I don't think we want to ever muck with the original response.
```
    const ResourceResponse& originalResponse() { return m_originalResponse; }
```

> Source/WebCore/inspector/agents/InspectorNetworkAgent.h:189
> +        ResourceResponse m_originalResponse;

Ditto (170).

> Source/WebInspectorUI/UserInterface/Base/HTTPUtilities.js:26
> +WI.httpStatusTextForStatusCode = function(code)

Rather than stick this onto `WI`, please make a "container object", like many of our other utilities files.
```
    WI.HTTPUtilities = class HTTPUtilities {
        static statusTextForStatusCode(statusCode)
        {
            ...
        }
    };
```

> Source/WebInspectorUI/UserInterface/Base/HTTPUtilities.js:28
> +    if (!code)

What about for `isNaN(code)`?

> Source/WebInspectorUI/UserInterface/Base/HTTPUtilities.js:32
> +    case 100: return "Continue";

Should we explicitly mark these as `WI.unlocalizedString`?

> Source/WebInspectorUI/UserInterface/Base/URLUtilities.js:299
> +        return urlString;

I'd move this outside the `catch` so it's more obvious that it's an "always" fallback.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:47
> +        this._harImportLocalResourceMap = new Map;

Is there any reason to keep this `Map`?  Is it purely to prevent GC?  In that case, we should just a `Set`, as two `WI.LocalResource` could have the same URL when importing a HAR.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:50
> +        this._pendingLocalResourceOverrideSaves = new Set;

We could also lazily initialize this when we create `this._saveLocalResourceOverridesDebouncer`.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:60
> +        WI.Resource.addEventListener(WI.SourceCode.Event.ContentDidChange, this._handleResourceContentDidChange, this);
> +        WI.LocalResourceOverride.addEventListener(WI.LocalResourceOverride.Event.DisabledChanged, this._handleResourceOverrideDisabledChanged, this);
> +
> +        WI.Target.registerInitializationPromise((async () => {

We should only do this `if (NetworkManager.supportsLocalResourceOverrides())`.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:100
> +        return window.NetworkAgent && InspectorBackend.domains.Network && InspectorBackend.domains.Network.setInterceptionEnabled;

This will have to be updated for <https://webkit.org/b/200384>.

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

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:173
> +            if (target.NetworkAgent && target.NetworkAgent.setInterceptionEnabled)

This should also have the `COMPATIBILITY (iOS 13)` statement.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:234
> +        if (!localResourceOverride)
> +            return;

I'd remove this, as we'd never provide a falsy `localResourceOverride` value.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:243
> +                if (target.NetworkAgent && target.NetworkAgent.addInterception)

Ditto (173).

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:248
> +        this.dispatchEventToListeners(WI.NetworkManager.Event.LocalResourceOverrideAdded, {localResourceOverride});

Style: drop the `WI.` since we're inside this class.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:255
> +        if (!localResourceOverride)
> +            return;

Ditto (233).

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:267
> +                if (target.NetworkAgent && target.NetworkAgent.removeInterception)

Ditto (173).

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:272
> +        this.dispatchEventToListeners(WI.NetworkManager.Event.LocalResourceOverrideRemoved, {localResourceOverride});

Ditto (248).

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:292
> +        if (!schemes.some((x) => resource.url.startsWith(x)))

Style: please use `scheme` instead of `x`.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:295
> +        let existingOverride = this.localResourceOverrideForURL(resource.url);

I'd inline this.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:308
> +            break;

This design means that if we add new resources, they're going to allow overrides by default.  I think we should take a whitelist approach, not a blacklist.
```
    switch (resource.type) {
        case WI.Resource.Type.Document:
        case WI.Resource.Type.StyleSheet:
        case WI.Resource.Type.Script:
        case WI.Resource.Type.XHR:
        case WI.Resource.Type.Fetch:
        case WI.Resource.Type.Image:
        case WI.Resource.Type.Font:
        case WI.Resource.Type.Other:
            return true;
    }

    return false;
```

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:314
> +            // Non-HTTP traffic.

I'd add a `console.assert()` not reached here, since this should be covered by the above `schemes` check.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:379
> -            this._localResourcesMap.set(localResource.url, localResource);
> +            this._harImportLocalResourceMap.set(localResource.url, localResource);

Ditto (47).

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:801
> +    responseIntercepted(target, requestId, response)

It seems like the only thing we actually care about from `response` is its `url`.  Perhaps we can simplify the protocol to just send back a `url`.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:813
> +        let {mimeType, statusCode, statusText, responseHeaders} = localResource;

I really don't like this pattern, as it's using destructuring on a non-POD object.  I'd far prefer you declare each of these as their own variable on a separate line, so it's clear that this isn't just a plain object.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:820
> +        if (isNaN(statusCode))
> +            statusCode = undefined;
> +        if (!statusText)
> +            statusText = undefined;
> +        if (!responseHeaders)
> +            responseHeaders = undefined;

In this situation, I'd actually recommend using `invoke` instead.
```
    let commandArguments = {
        requestId,
        content: localResource.localContent,
        base64Encoded: localResource.localContentIsBase64Encoded,
        mimeType: localResource.mimeType,
    };
    if (!isNaN(localResource.statusCode))
        commandArguments.statusCode = localResource.statusCode;
    if (localResource.statusText)
        commandArguments.statusText = localResource.statusText;
    if (localResource.responseHeaders)
        commandArguments.responseHeaders = localResource.responseHeaders;
    target.NetworkAgent.interceptWithResponse.invoke(commandArguments);
```

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:1240
> +        let content = localResource.currentRevision.content;

You should just be able to use `WI.SourceCode.prototype.get content`, which does this for you.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:1242
> +        let content = localResource.currentRevision.content;
> +        let base64Encoded = localResource.localContentIsBase64Encoded;
> +        let mimeType = localResource.mimeType;

I'd inline most of these.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:1257
> +        this._saveLocalResourceOverridesDebouncer.delayForTime(1000);

How about 500ms?  1s seems very long.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:1260
> +    _saveLocalResourceOverrides()

I'd inline this inside the `WI.Debouncer`.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:1270
> +        console.assert(WI.NetworkManager.supportsLocalResourceOverrides());

We can remove this given my comment on 57.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:1278
> +                    target.NetworkAgent.removeInterception(localResourceOverride.url, "response");

Ditto (128).

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:1280
> +                    target.NetworkAgent.addInterception(localResourceOverride.url, "response");

Ditto (128).

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:40
> +    constructor({request, response, metrics, timing, override})

This makes me think that `override` is a `WI.LocalResourceOverride`, not a boolean flag.  Perhaps it should be `isLocalResourceOverride`?

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:235
> +    get localContent() { return this._localContent; }

Shouldn't this go through `this._currentRevision.content` (which is `WI.SourceCode.prototype.get content`)?

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:236
> +    get localContentIsBase64Encoded() { return this._localContentIsBase64Encoded; }

Perhaps we should add this to `WI.SourceCodeRevision`, so that each change can reflect whether the content is base64Encoded or not?

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:238
> +    isLocalResourceOverride()

Make this into a getter?

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:263
> +    updateOverrideContent(content, base64Encoded, mimeType, options = {})

This could be made to be more general.  There's no particular reason why we have to "limit" this to override content only.

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:269
> +        if (content !== undefined && this._localContent !== content) {

So is `undefined` the way you'd say "don't update this"?

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:282
> +            this.dispatchEventToListeners(WI.Resource.Event.MIMETypeDidChange, {oldMIMEType});

Should this also be guarded by `options.suppressEvent`?

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:283
> +            contentChanged = true;

Does a change of MIME type actually mean that the content has changed?  I wouldn't think so.

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:287
> +            this.dispatchEventToListeners(WI.LocalResource.Event.OverrideContentChanged);

You can drop the `WI.` since it's inside the same class.

Also, does anyone actually listen for this?  Can we remove it?

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:302
> +    OverrideContentChanged: "local-resource-override-content-changed",

This event name _could_ conflict with a theoretical `WI.LocalResourceOverride.Event.ContentChanged` :\

> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:67
> +        let {disabled} = json;

It's odd to only destructure part of `json` and then do a property access on the next line.
```
    static fromJSON({localResource, json})
    {
        return new WI.LocalResourceOverride(WI.LocalResource.fromJSON(localResource), {disabled});
    }
```

> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:79
> +        if (key === WI.ObjectStore.toJSONSymbol)

This exists primarily to allow extra data to be stored in IndexedDB that wouldn't appear in a regular JSON export (the enabled/disabled state of Audits, for example).

> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:80
> +            json[WI.objectStores.localResourceOverrides.keyPath] = this._localResource.url;

You shouldn't need to do this to store inside a `WI.ObjectStore`.  When you `WI.objectStores.localResourceOverrides.associateObject`, it'll already add a `__id` property to the `localResource` object.

> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:100
> +        this._disabled = disabled || false;

I think we should start using `!!` more in cases where we want to do a boolean setter, so that objects can be used as a value and won't be strongly held.
```
    this._disabled = !!disabled;
```

> Source/WebInspectorUI/UserInterface/Models/Resource.js:1065
> +            content: content,

This can be simplified to just `content,`.

> Source/WebInspectorUI/UserInterface/Protocol/NetworkObserver.js:112
> +        WI.networkManager.responseIntercepted(this.target, requestId, response);

This will have to change with <https://webkit.org/b/200384>.

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:322
> +            const omitFocus = true;
> +            const selectedByUser = false;
> +            const suppressNotification = false;
> +            treeElement.revealAndSelect(omitFocus, selectedByUser, suppressNotification);

This is far reaching and therefore scary.  Can you explain why this is needed?

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:105
> +        if (representedObject instanceof WI.LocalResourceOverride)
> +            return WI.ContentView.createFromRepresentedObject(representedObject.localResource);

This should already be handled by `WI.ContentView.resolvedRepresentedObjectForRepresentedObject`.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:287
> +        if (representedObject instanceof WI.LocalResourceOverride)
> +            return true;

Ditto (104).

> Source/WebInspectorUI/UserInterface/Views/ContextMenu.js:-252
> -        if (this._handlers[id])
> -            this._handlers[id].call(this);

What caused this?

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:75
> +    if (WI.NetworkManager.supportsLocalResourceOverrides()) {
> +        if (sourceCode instanceof WI.Resource) {
> +            let resource = sourceCode;

All of these checks are already done by (or could be moved into) `WI.NetworkManager.prototype.canBeOverridden`.  I also think the `let resource = sourceCode;` doesn't add much and can be removed.

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:91
> +    if (sourceCode instanceof WI.Resource && !sourceCode.isLocalResourceOverride()) {

Should this also wrap the breakpoints and actions below?

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:535
>          WI.startEditing(element, this._startEditingConfig(element));

I think this might add some styles that muck with the border of the cell.

> Source/WebInspectorUI/UserInterface/Views/DataGridNode.js:36
> +        this._editable = true;

We could probably use this to replace `WI.EditableDataGridNode` :P

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideLabelView.css:50
> +    background-color: hsl(43.53, 82.76%, 48.68%);

Very specific :P

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideLabelView.css:60
> +        background-color: var(--yellow-highlight-background-color);

Why not have a `--warning-color` override in dark mode?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideLabelView.css:61
> +        background-color: var(--yellow-highlight-background-color);
> +        color: var(--yellow-highlight-text-color);

I'd swap the order of these (`color` is content, whereas `background-color` is background, which is "below" content).

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideLabelView.js:51
> +        let div = this.element.appendChild(document.createElement("div"));

NIT: I'd call this `container` or `wrapper`.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.css:27
> +    width: 420px;

Does this have to be hardcoded?  Can we have it as a `max-width` instead?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.css:28
> +    padding: 5px 10px;

Usually multiples of 4 end up looking/rendering better.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.css:42
> +    width: 1px; /* Shrink to fit. */

Can we use a different keyword, like `auto` or `min-content`?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.css:48
> +    color: hsl(0, 0%, 34%);

Is `var(--text-color-secondary)` not contrast-y enough in light mode?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.css:52
> +    padding-left: 5px;

Ditto (28).

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.css:58
> +    padding: 4px 0 2px 0;

You don't need the last `0`.
```
    padding: 4px 0 2px;
```

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.css:79
> +.popover .local-resource-override-popover-content .editor.url {
> +    width: 324px;
> +}
> +
> +.popover .local-resource-override-popover-content .editor.mime {
> +    width: 240px;
> +}
> +
> +.popover .local-resource-override-popover-content .editor.status {
> +    width: 36px;
> +}

Why so specific?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.css:83
> +    -webkit-margin-start: 12px;
> +    width: 168px;

Please flip the order of these (width > margin).

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:35
> +        this._urlCodeMirror = null;
> +        this._mimeTypeCodeMirror = null;
> +        this._statusCodeCodeMirror = null;
> +        this._statusTextCodeMirror = null;

I understand that you have a special mode for non-fragment and http/https URLs, but none of these _really_ need to use `CodeMirror`.  The status code could be an `<input type="number">` for example.  We could use `CanvasRenderingContext2d.prototype.measureText` to make the inputs the right width too!

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:40
> +        this.windowResizeHandler = this._presentOverTargetElement.bind(this);

You could just directly override `this._resizeHandler`.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:55
> +        if (!schemes.some((x) => url.startsWith(x)))

Please use `scheme` instead of `x`.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:59
> +        // NOTE: We can allow an empty mimeType / statusCode / statusText to pass
> +        // network values through, but lets require them for overrides.

This could use a "why" or "because" at the end to explain the reasoning for this choice.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:66
> +        if (!statusCode || statusCode < 0)

This should probably be `if (isNaN(statusCode) || statusCode < 0)`.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:81
> +            if (name.toLowerCase() === "content-type")
> +                continue;
> +            if (name.toLowerCase() === "set-cookie")
> +                continue;

Should we color these rows red if someone tries to add a `Set-Cookie` header?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:96
> +        let oldSerialized = JSON.stringify(this._serializedDataWhenShown);
> +        let newSerialized = JSON.stringify(data);
> +        if (oldSerialized === newSerialized)

Perhaps we should make an `Object.deepEquals`?  If not for `headers` (or if it was an array), you could use `Object.shallowEquals`.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:109
> +        let statusCode = localResource ? ("" + localResource.statusCode) : "";

`String(locaResource.statusCode)`

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:127
> +        let urlRow = table.appendChild(document.createElement("tr"));

I'd make a function out of this for URL, MIME Type, and Status Code/Text:
```
    let createRow = (label, id, text, mode) => {
        let rowElement = table.appendChild(document.createElement("tr"));

        let headerElement = rowElement.appendChild(document.createElement("th"));

        let labelElement = headerElement.appendChild(document.createElement("label"));
        labelElement.textContent = label;

        let dataElement = rowElement.appendChild(document.createElement("td"));

        let editorElement = dataElement.appendChild(document.createElement("div"));
        editorElement.classList.add("editor", id);

        let codeMirror = this._createEditor(editorElement, text, mode);

        let urlInputField = codeMirror.getInputField();
        urlInputField.id = `local-resource-override-popover-${id}-input-field`;
        labelElement.setAttribute("for", urlInputField.id);

        return {codeMirror, dataElement};
    };

    let urlRow = createRow(WI.UIString("URL"), "url", url, "text/x-local-override-url");
    this._urlCodeMirror = urlRow.codeMirror;

    let mimeTypeRow = createRow(WI.UIString("MIME Type"), "mime", mimeType);
    this._mimeTypeCodeMirror = mimeTypeRow.codeMirror;

    let statusCodeRow = createRow(WI.UIString("Status"), "status", statusCode);
    this._statusCodeCodeMirror = statusCodeRow.codeMirror;

    let statusTextEditorElement = statusCodeRow.dataElement.appendChild(document.createElement("div"));
    statusTextEditorElement.className = "editor status-text";
    this._statusTextCodeMirror = this._createEditor(statusTextEditorElement, statusText);
```

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:170
> +        let headersData = headersRow.appendChild(document.createElement("td"));

I'd move this closer to where it's actually used.  I initially got confused and expected `headersData` to be a child of `headersHeader` since it "cascades" in code like that.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:195
> +            }

Style: missing comma

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:235
> +            let x = parseInt(this._statusCodeCodeMirror.getValue());

Please use an actual variable name instead of `x`.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:236
> +            if (x >= 999)

What about if `isNaN(statusCode)`?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:243
> +                x = (x - (x % 100)) + 100;
> +                if (x >= 999)
> +                    x = 999;

`x = Math.min((x - (x % 100)) + 100, 999);`

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:245
> +                x += 1;

`++x;`

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:259
> +                x = (x - (x % 100));

`x -= x % 100;`

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:263
> +                x -= 1;

`--x;`

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

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:327
> +            mode: "text/plain",

Rather than override this above inside `show`, we could take this as an argument and have `mode || "text/plain"` as a fallback.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:334
> +        codeMirror.addKeyMap({

Can we have "Tab"/"Shift-Tab" select the next/previous editor?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:36
> +        this._localResourceOverride = representedObject;

I personally don't like it when we have a duplicate reference to the same object in a class.  I'd rather you just use `this.representedObject`.  Its type still be obvious enough given the name of the class/file.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:38
> +        this._overrideEnabledStatusElement = document.createElement("input");

This doesn't need to be saved to a `this._*`.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:42
> +            this._localResourceOverride.disabled = !this._overrideEnabledStatusElement.checked;

I'd also add a listener for `WI.LocalResourceOverride.Event.DisabledChanged` and update the checkbox accordingly, just in case the disabled state is modified by something else.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:64
> +        contextMenu.appendItem(WI.UIString("Remove Local Override"), () => {

We should also have a context menu item for "Disable Local Override"/"Enable Local Override" to match what we have for breakpoints.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:89
> +            content: this._localResourceOverride.localResource._localContent,
> +            base64Encoded: this._localResourceOverride.localResource._localContentIsBase64Encoded,

Use the public getters?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:93
> +        WI.networkManager.removeLocalResourceOverride(this._localResourceOverride);
> +        WI.networkManager.addLocalResourceOverride(newLocalResourceOverride);

So if you change the MIME Type or add a new header, we create an entire new tree element?

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

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideWarningView.js:38
> +        let div = this.element.appendChild(document.createElement("div"));

The name `div` isn't descriptive.  How about `labelElement` or `messageElement`?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideWarningView.js:41
> +        this._revealButton = div.insertBefore(document.createElement("button"), div.firstChild);

Rather than `insertBefore`, we could regularly `appendChild` (or `append`) this and do a `document.createTextNode` (or `append`) for the message text.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideWarningView.js:43
> +        this._revealButton.addEventListener("click", (event) => {

Can we create these inside an `initialLayout` instead?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideWarningView.js:47
> +            WI.showRepresentedObject(overrideResource, cookie, options);

Should we `console.assert(overrideResource);`?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideWarningView.js:74
> +        let localResourceOverrideExists = !!WI.networkManager.localResourceOverrideForURL(this._resource.url);

How about `hasLocalResourceOverride`?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideWarningView.js:81
> +    _handleLocalResourceOverrideChanged(event)

`*Changed` isn't really a good term for this.  I'd call it `*AddedOrRemoved`.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1039
> +                    transferSizeA = -3;

Should we consider this as a `-1`, since there is kinda a transfer size but not really?

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1049
> +                    transferSizeB = -3;

Ditto (1039).

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

<3

> Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js:33
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:197
> +                return false;

We don't allow formatting for local resource overrides?  Why not?

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1185
> +            // FIXME: Allow setting breakpoint in overrides.

I wouldn't put this as a FIXME, as I'm not sure we've decided whether we want to do this or not.  I'd rather avoid a situation where we have this in the source and then decide _not_ to do it.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.css:72
> +.sidebar > .panel.navigation.sources > .content > .local-overrides {

I think you should also create a rule for this inside the `@media` below that sets the `flex-grow` and `flex-shrink` to make sure the local overrides section doesn't shrink when you don't want it to.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:133
> +        this._localResourceOverridesTreeOutline = this.createContentTreeOutline({suppressFiltering: true});

We shouldn't show any of this unless `WI.NetworkManager.supportsLocalResourceOverrides()`.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:135
> +        this._localResourceOverridesTreeOutline.ondelete = (treeElement) => {

This should be a prototype method on `WI.LocalResourceOverrideTreeElement` instead of the overall `WI.TreeOutline`.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:294
> +        WI.networkManager.addEventListener(WI.NetworkManager.Event.LocalResourceOverrideAdded, this._handleLocalResourceOverrideAdded, this);

Ditto (133).

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:467
> +            return this._localResourceOverridesTreeOutline.findTreeElement(localResourceOverride);

Should we `console.assert(localResourceOverride);`?

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

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1243
> +        // FIXME: Only should local resources matching the origins on the page?

I think we should always show ALL local resource overrides, so I think this should be removed.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1246
> +        let resourceTreeElement = new WI.LocalResourceOverrideTreeElement(localResourceOverride.localResource, localResourceOverride, {hideOrigin: true});

I think we should only `hideOrigin` if the local resource override has the same origin as the main frame.

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.css:-31
> -    position: absolute;
> -    top: 0;
> -    left: 0;
> -    right: 0;
> -    bottom: 0;

This is very scary T.T

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:59
> +        this._createLocalResourceOverrideButtonNavigationItem = new WI.ButtonNavigationItem("create-local-resource-override", WI.UIString("Create Local Override"), "Images/NavigationItemNetworkOverride.svg", 13, 14);

13x14?  Yuck.

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:64
> +        this._removeLocalResourceOverrideButtonNavigationItem = new WI.ButtonNavigationItem("remove-local-resource-override", WI.UIString("Remove Local Override"), "Images/NavigationItemTrash.svg", 15, 15);

Don't we know whether the `resource` is a local override at this point?  We should only be creating one of these navigation items.

Also, all of this should be guarded by `if (WI.NetworkManager.supportsLocalResourceOverrides())`.

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:90
> +        WI.networkManager.addEventListener(WI.NetworkManager.Event.LocalResourceOverrideAdded, this._handleLocalResourceOverrideChanged, this);

Ditto (64).

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:109
> +        if (isResource && !this.resource.isLocalResourceOverride())
> +            items.push(this._prettyPrintButtonNavigationItem, this._showTypesButtonNavigationItem, this._codeCoverageButtonNavigationItem);

We can pretty print CSS style sheets.  We shouldn't guard based on `isResource`.  AFAIU, our usual style is to always show navigation items and just keep them disabled if it doesn't make sense.  I'm all for not showing them when it doesn't make sense (in that case, we shouldn't even be creating the objects), but I think what's here right now isn't correct.

> Source/WebKit/WebProcess/Network/WebResourceInterceptController.cpp:30
> +namespace WebKit {
> +using namespace WebCore;

Style: missing newline

> Source/WebKit/WebProcess/Network/WebResourceInterceptController.cpp:39
> +    m_interceptedResponseQueue.set(identifier, Deque<Function<void()>>());

Can you use `{ }` instead?

> Source/WebKit/WebProcess/Network/WebResourceInterceptController.cpp:44
> +    auto queue = m_interceptedResponseQueue.take(identifier);

I'd inline this.

> Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:155
> +                    policyDecisionCompletionHandler();
> +                if (!m_coreLoader || !m_coreLoader->identifier())

Style: missing newline

> Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:158
> +                RefPtr<WebCore::ResourceLoader> protector = m_coreLoader;

I think this is your build failure issue.  Perhaps it should be `coreLoader` or `protectedCoreLoader`?

> Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:160
> +                    m_interceptController.continueResponse(coreLoader->identifier());

Should we save the `m_coreLoader->identifier()` so that this (or the equivalent call in the `else` branch) gets called, ensuring that there are no extra items inside the `HashMap`?

[Joseph Pecoraro]

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:6
>> +URL: http://127.0.0.1:8000/inspector/network/resources/override.txt
> 
> Can we strip everything before the `inspector/network` in case the bots (now or in the future) use a different URL?

This is `http/tests` it will always be that.

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

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

>> LayoutTests/http/tests/inspector/network/local-resource-override-basic.html:201
>> +        async teardown() { WI.networkManager.interceptionEnabled = true; },
> 
> Style: please indent these so they're easier to read, and aren't confused with `() => ...` that implicitly return a value.

I found that harder to read. I think these are far more obvious with syntax highlighting.

>> LayoutTests/http/tests/inspector/network/local-resource-override-main-resource.html:25
>> +            await InspectorTest.reloadPage({ignoreCache: false, revalidateAllResources: true});
> 
> If `ignoreCache` is `false`, can we remove it here?

Our tests tend to be explicit about this, and I like that this is explicit here.

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

>> LayoutTests/http/tests/inspector/network/local-resource-override-main-resource.html:37
>> +<script>TestPage.dispatchEventToFrontend("TestPageLoadComplete");</script>
> 
> So this will be ignored on the first load (`runTest` shouldn't have run yet), but will be picked up the second time?  This seems brittle.  Could we listen for various `WI.Frame.Event` or `WI.Resource.Event` instead?

Oops, I will just delete this. I accidentally carried it over from the script tag test.

The `TestPage.completeTest()` in the override content is what ends the test, the await line doesn't even get reached and the test ends.

I'll simplify this test a bit and try to make it clear with content in the override page.

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

This matches all the other http/tests/inspector/network/resource-response-*.html tests. We should modernize them together. I agree it is rather ugly.

>> LayoutTests/inspector/unit-tests/url-utilities-expected.txt:479
>> +PASS: Removing fragment of 'http://example.com?#frag' should be 'http://example.com/?'.
> 
> Just for sanity, can we add a test where the fragment is empty?
> 
> http://example.com#
> http://example.com/#
> http://example.com/path#
> http://example.com/path/#
> ...

Great! `new URL` doesn't strip that, but the backend wasn't including it. So I've updated `WI.urlWithoutFragment` to strip it in those cases.

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

>> Source/JavaScriptCore/inspector/protocol/Network.json:265
>> +                { "name": "base64Encoded", "type": "boolean", "description": "True, if content was sent as base64." },
> 
> This means that there's no way to have an override that ONLY modifies metadata (e.g. headers, mime type, etc.).  Do we want to support something like that?

Good point. If we want to support that in the future it should be easy to make those changes. I like the idea of requiring something if you are overriding.

>> Source/JavaScriptCore/inspector/protocol/Network.json:347
>> +            "name": "responseIntercepted",
> 
> As you alluded to in your ChangeLog, would we have a separate event for `requestIntercepted` (as well as an `interceptWithRequest` command)?

Yes, if/when we implement it.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1024
>> +    if (!m_enabled)
> 
> OK so this now begs the question of what does `enable`/`disable` signify.  Do we want to "limit" it to "this controls whether the agent can dispatch events to the frontend" or should it be broader to "this controls whether the agent can do anything"? 
> 
> As it stands right now, this check is unnecessary, as `m_enabled` is set at the same time as `m_instrumentingAgents.setInspectorNetworkAgent(...)`, so this cannot be called when `m_enabled` is false.

I typically consider `m_enabled` to mean can this agent send messages to the frontend, not wether or not it was in instrumenting agents.

But you are right. I'll remove these early returns.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1031
>> +    requestURL.removeFragmentIdentifier();
> 
> Wouldn't this modify the `URL`?  Should we `isolatedCopy` first?

The `URL requestURL = request.url()` should copy:

    // xcrun -sdk macosx.internal clang++ -std=gnu++17 -stdlib=libc++ -DNDEBUG -Os -framework Foundation -framework JavaScriptCore -g url.mm
    #import <Foundation/Foundation.h>
    #import <wtf/URL.h>

    class Foo {
    public:
        Foo(const String& str) : m_url({ }, str) { };
        const URL& url() const { return m_url; }
        URL m_url;
    };

    int main() {
        Foo f("http://example.com/path#foo");
        URL url = f.url();
        printf("foo.url: %s\n", f.url().string().utf8().data()); // => http://example.com/path#foo
        printf("url:     %s\n", url.string().utf8().data());     // => http://example.com/path#foo
        url.removeFragmentIdentifier();
        printf("foo.url: %s\n", f.url().string().utf8().data()); // => http://example.com/path#foo
        printf("url:     %s\n", url.string().utf8().data());     // => http://example.com/path
        return 0;
    }

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1065
>> +    if (!m_interceptionEnabled) {
> 
> Should we instead `ASSERT` that we can't reach this?

Agreed!

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.h:189
>> +        ResourceResponse m_originalResponse;
> 
> Ditto (170).

Hmm, I actually needed to copy this to avoid issues. I will investigate that.

>> Source/WebInspectorUI/UserInterface/Base/HTTPUtilities.js:28
>> +    if (!code)
> 
> What about for `isNaN(code)`?

I can avoid this early return and instead handle 0 as "OK".

>> 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 =(

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:47
>> +        this._harImportLocalResourceMap = new Map;
> 
> Is there any reason to keep this `Map`?  Is it purely to prevent GC?  In that case, we should just a `Set`, as two `WI.LocalResource` could have the same URL when importing a HAR.

No reason. I like moving this to a Set.

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

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

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:308
>> +            break;
> 
> This design means that if we add new resources, they're going to allow overrides by default.  I think we should take a whitelist approach, not a blacklist.
> ```
>     switch (resource.type) {
>         case WI.Resource.Type.Document:
>         case WI.Resource.Type.StyleSheet:
>         case WI.Resource.Type.Script:
>         case WI.Resource.Type.XHR:
>         case WI.Resource.Type.Fetch:
>         case WI.Resource.Type.Image:
>         case WI.Resource.Type.Font:
>         case WI.Resource.Type.Other:
>             return true;
>     }
> 
>     return false;
> ```

The intent of this method is to include a bunch of reasons why not to allow an override. So I don't want to provide any `return true` until the end.

I could break the sub-types out into ifs:

    // Responses aren't expected for Ping/Beacon.
    if (type === WI.Resource.Type.Ping || type === WI.Resource.Type.Beacon)
        return false;

    // Non-HTTP traffic.
    if (type === WI.Resource.Type.WebSocket)
        return false;

But I liked the switch so that if we add a new resource type we are more likely to get here and not miss it. Especially considering the most recent resource types have been cases where we return false.

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:801
>> +    responseIntercepted(target, requestId, response)
> 
> It seems like the only thing we actually care about from `response` is its `url`.  Perhaps we can simplify the protocol to just send back a `url`.

If we implement real URL breakpoints using this interception, then it would be useful to see the current response and modify it, before returning it. I want that capability from the beginning.

That said, we intercept a little too early (before we have the data itself).

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:1257
>> +        this._saveLocalResourceOverridesDebouncer.delayForTime(1000);
> 
> How about 500ms?  1s seems very long.

Agreed.

>> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:235
>> +    get localContent() { return this._localContent; }
> 
> Shouldn't this go through `this._currentRevision.content` (which is `WI.SourceCode.prototype.get content`)?

I'm going to rethink this system for the image content.

>> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:236
>> +    get localContentIsBase64Encoded() { return this._localContentIsBase64Encoded; }
> 
> Perhaps we should add this to `WI.SourceCodeRevision`, so that each change can reflect whether the content is base64Encoded or not?

Yep, using revisions would allow us to undo/redo for images.

>> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:238
>> +    isLocalResourceOverride()
> 
> Make this into a getter?

In general we don't make `isFoo` names getters. Maybe we should.

>> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:287
>> +            this.dispatchEventToListeners(WI.LocalResource.Event.OverrideContentChanged);
> 
> You can drop the `WI.` since it's inside the same class.
> 
> Also, does anyone actually listen for this?  Can we remove it?

This was only needed for Image support. I'm going to potentially change this around for that.

>> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:302
>> +    OverrideContentChanged: "local-resource-override-content-changed",
> 
> This event name _could_ conflict with a theoretical `WI.LocalResourceOverride.Event.ContentChanged` :\

Yes, that is why I used [Override]ContentChanged, even though LocalResource.Event.ContentChanged would have been unique.

This event entirely is subject to change if I switch to sitting on top of the SourceCodeRevision system which is a bit weird right now (only really works for text resources).

>> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:80
>> +            json[WI.objectStores.localResourceOverrides.keyPath] = this._localResource.url;
> 
> You shouldn't need to do this to store inside a `WI.ObjectStore`.  When you `WI.objectStores.localResourceOverrides.associateObject`, it'll already add a `__id` property to the `localResource` object.

Hmm, this is used by other persistent object store objects. Without it I get an exception:

    Error: Failed to store record in an IDBObjectStore:​ Evaluating the object store's key path did not yield a value. (at ObjectStore.js:​208:​35)​

>> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:100
>> +        this._disabled = disabled || false;
> 
> I think we should start using `!!` more in cases where we want to do a boolean setter, so that objects can be used as a value and won't be strongly held.
> ```
>     this._disabled = !!disabled;
> ```

Sounds good

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

>> Source/WebInspectorUI/UserInterface/Views/ContentView.js:105
>> +            return WI.ContentView.createFromRepresentedObject(representedObject.localResource);
> 
> This should already be handled by `WI.ContentView.resolvedRepresentedObjectForRepresentedObject`.

Depends on the API entry point. See the example of WI.Breakpoint above which also re-runs what would be the resolve code.

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

>> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:75
>> +            let resource = sourceCode;
> 
> All of these checks are already done by (or could be moved into) `WI.NetworkManager.prototype.canBeOverridden`.  I also think the `let resource = sourceCode;` doesn't add much and can be removed.

I use lines like:

    let resource = sourceCode;

To clarify and "reclassify" the type.

I treat this like:

    if (is<Foo>(*blah)) {
        Foo& foo = downcast<Foo>(*blah);
        ...
    }

For example if I see something like:

    sourceCode.createLocalResourceOverride()

It raises red flags. There isn't `SourceCode.prototype.createLocalResourceOverride`. Likewise for `sourceCode.url` there is `SourceCode.prototype.get url()` but there probably shouldn't be. In each case we can make the leap that this is a sourceCode subclass, but then we should hope that this was type checked, but I find making the code explicit helps prevent such mistakes.

This also has benefits in code searching. By using consistent names it is not surprising to look for /Override.localResource/ and find instances of "localResourceOverride.localResource" or "existingOverride.localResource". Likewise for /resource.url/. But it would be weird to search for /sourceCode.url/ and thus it makes something like this easy to be missed.

Finally, the assignments should be negligible for performance. I would expect any JIT'd code will optimize these out in by doing its own register allocation.

>> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:91
>> +    if (sourceCode instanceof WI.Resource && !sourceCode.isLocalResourceOverride()) {
> 
> Should this also wrap the breakpoints and actions below?

Hmm, that is only on a SourceCodeLocation, which is likely to be a real loaded resource. But I added the check.

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

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:40
>> +        this.windowResizeHandler = this._presentOverTargetElement.bind(this);
> 
> You could just directly override `this._resizeHandler`.

I believe we generally avoid accessing private members even when we override. Hopefully the cases we do it are rare.

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:66
>> +        if (!statusCode || statusCode < 0)
> 
> This should probably be `if (isNaN(statusCode) || statusCode < 0)`.

Nice!

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:81
>> +                continue;
> 
> Should we color these rows red if someone tries to add a `Set-Cookie` header?

Ultimately we should support Set-Cookie. We may need CFNetwork / lower level networking support to get this working.

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:127
>> +        let urlRow = table.appendChild(document.createElement("tr"));
> 
> I'd make a function out of this for URL, MIME Type, and Status Code/Text:
> ```
>     let createRow = (label, id, text, mode) => {
>         let rowElement = table.appendChild(document.createElement("tr"));
> 
>         let headerElement = rowElement.appendChild(document.createElement("th"));
> 
>         let labelElement = headerElement.appendChild(document.createElement("label"));
>         labelElement.textContent = label;
> 
>         let dataElement = rowElement.appendChild(document.createElement("td"));
> 
>         let editorElement = dataElement.appendChild(document.createElement("div"));
>         editorElement.classList.add("editor", id);
> 
>         let codeMirror = this._createEditor(editorElement, text, mode);
> 
>         let urlInputField = codeMirror.getInputField();
>         urlInputField.id = `local-resource-override-popover-${id}-input-field`;
>         labelElement.setAttribute("for", urlInputField.id);
> 
>         return {codeMirror, dataElement};
>     };
> 
>     let urlRow = createRow(WI.UIString("URL"), "url", url, "text/x-local-override-url");
>     this._urlCodeMirror = urlRow.codeMirror;
> 
>     let mimeTypeRow = createRow(WI.UIString("MIME Type"), "mime", mimeType);
>     this._mimeTypeCodeMirror = mimeTypeRow.codeMirror;
> 
>     let statusCodeRow = createRow(WI.UIString("Status"), "status", statusCode);
>     this._statusCodeCodeMirror = statusCodeRow.codeMirror;
> 
>     let statusTextEditorElement = statusCodeRow.dataElement.appendChild(document.createElement("div"));
>     statusTextEditorElement.className = "editor status-text";
>     this._statusTextCodeMirror = this._createEditor(statusTextEditorElement, statusText);
> ```

Done.

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:263
>> +                x -= 1;
> 
> `--x;`

I like all of these as written, because they show the logical steps clearer. They don't need to be particularly clever.

Up:
• Normal = +1
• Shift = next hundred barrier
• Max = 999

Down:
• Normal = -1
• Shift = previous hundred barrier
• Min = 0

In fact I moved the bounds check outside just to make it clearer.

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

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

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:36
>> +        this._localResourceOverride = representedObject;
> 
> I personally don't like it when we have a duplicate reference to the same object in a class.  I'd rather you just use `this.representedObject`.  Its type still be obvious enough given the name of the class/file.

This is the same as the `let resource = sourceCode;` kind of clarification.

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:42
>> +            this._localResourceOverride.disabled = !this._overrideEnabledStatusElement.checked;
> 
> I'd also add a listener for `WI.LocalResourceOverride.Event.DisabledChanged` and update the checkbox accordingly, just in case the disabled state is modified by something else.

Good idea!

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:64
>> +        contextMenu.appendItem(WI.UIString("Remove Local Override"), () => {
> 
> We should also have a context menu item for "Disable Local Override"/"Enable Local Override" to match what we have for breakpoints.

Good idea!

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:93
>> +        WI.networkManager.addLocalResourceOverride(newLocalResourceOverride);
> 
> So if you change the MIME Type or add a new header, we create an entire new tree element?

Yep. This tries to be simple to avoid complexity.

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

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1039
>> +                    transferSizeA = -3;
> 
> Should we consider this as a `-1`, since there is kinda a transfer size but not really?

Its just a sort preference of the -20, -10. -5. -3. Doesn't need any particular value. Just rating it something so they are sorted together.

>> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:197
>> +                return false;
> 
> We don't allow formatting for local resource overrides?  Why not?

I enabled this. I previously hadn't tested formatting with editable resources and it just turns into a "re-format" / "undo" button which is great!

>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.css:72
>> +.sidebar > .panel.navigation.sources > .content > .local-overrides {
> 
> I think you should also create a rule for this inside the `@media` below that sets the `flex-grow` and `flex-shrink` to make sure the local overrides section doesn't shrink when you don't want it to.

Good point!

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

>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1246
>> +        let resourceTreeElement = new WI.LocalResourceOverrideTreeElement(localResourceOverride.localResource, localResourceOverride, {hideOrigin: true});
> 
> I think we should only `hideOrigin` if the local resource override has the same origin as the main frame.

Good point.

>> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:109
>> +            items.push(this._prettyPrintButtonNavigationItem, this._showTypesButtonNavigationItem, this._codeCoverageButtonNavigationItem);
> 
> We can pretty print CSS style sheets.  We shouldn't guard based on `isResource`.  AFAIU, our usual style is to always show navigation items and just keep them disabled if it doesn't make sense.  I'm all for not showing them when it doesn't make sense (in that case, we shouldn't even be creating the objects), but I think what's here right now isn't correct.

Yes, I reworked this when allowing local resource overrides to be formatted.

>> Source/WebKit/WebProcess/Network/WebResourceInterceptController.cpp:30
>> +using namespace WebCore;
> 
> Style: missing newline

This is actually the norm in WebKit.

>> Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:160
>> +                    m_interceptController.continueResponse(coreLoader->identifier());
> 
> Should we save the `m_coreLoader->identifier()` so that this (or the equivalent call in the `else` branch) gets called, ensuring that there are no extra items inside the `HashMap`?

I like this. In case the identifier changes, and also to ensure we stop intercepting on the early returns.

[Devin Rousso]

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

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Created attachment 377685 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 377686 [details]
[PATCH] Proposed Fix

[Alex Christensen]

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.

[Joseph Pecoraro]

Created attachment 377934 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 377943 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 377995 [details]
[PATCH] For Bots

[Devin Rousso]

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

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 378012 [details]
[PATCH] For Landing

[Joseph Pecoraro]

Created attachment 378016 [details]
[PATCH] For Landing

[WebKit Commit Bot]

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

[EWS Watchlist]

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

[Joseph Pecoraro]

https://trac.webkit.org/changeset/249504/webkit