[webkit-reviews] review canceled: [Bug 201262] Web Inspector: Local Overrides - Provide substitution content for resource loads (URL based) : [Attachment 377555] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has canceled Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 201262: Web Inspector: Local Overrides - Provide substitution content for
resource loads (URL based)
https://bugs.webkit.org/show_bug.cgi?id=201262

Attachment 377555: [PATCH] Proposed Fix

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




--- Comment #21 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 377555
  --> https://bugs.webkit.org/attachment.cgi?id=377555
[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.h
tml: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.LocalResourceOverrideAdde
d, {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:3
6
>> +	    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:4
2
>> +		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:6
4
>> +	    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:9
3
>> +	   
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:9
8
>> +		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.