Bug 238236 – Web Inspector: Sources: allow Response Local Overrides to map to a file on disk

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

this makes Response Local Overrides even more powerful by allowing developers to map the contents of the (Local Override) resource to a file on disk (e.g. a local copy of the file), meaning that they can use their preferred editor of choice (and all the tools that may come with it) to make changes instead of having to stay within Web Inspector :)

[Devin Rousso]

<rdar://problem/59009154>

[Devin Rousso]

Created attachment 455450 [details]
[fast-cq] Patch

[Devin Rousso]

Created attachment 455451 [details]
[Image] after Patch is applied

[Devin Rousso]

Created attachment 455454 [details]
[fast-cq] Patch

[Devin Rousso]

Created attachment 455516 [details]
[fast-cq] Patch

[Devin Rousso]

Created attachment 455525 [details]
[fast-cq] Patch

[Devin Rousso]

Created attachment 455554 [details]
[fast-cq] Patch

[Devin Rousso]

Created attachment 455704 [details]
[fast-cq] Patch

[Patrick Angle]

Comment on attachment 455704 [details]
[fast-cq] Patch

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

Awesome stuff!

> Source/WebCore/testing/Internals.cpp:4867
> +    auto path = FileSystem::openTemporaryFile("WebCoreTesting", file);

I wonder if we should require the test to provide a name for the temporary file so that if you needed two temp files the second one wouldn't clobber the first? Not a problem currently, but I could see the assumption being made that you can create multiple files when you really can't.

> Source/WebInspectorUI/UserInterface/Images/Disk.svg:5
> +    <path fill="none" stroke="currentColor" d="M5.240,3.215 L9.784,3.215 C10.594,3.215 11.325,3.705 11.633,4.455 L13.521,9.047 C13.941,10.069 13.454,11.238 12.432,11.658 C12.191,11.757 11.932,11.808 11.672,11.808 L3.337,11.808 C2.233,11.808 1.337,10.912 1.337,9.808 C1.337,9.545 1.389,9.285 1.490,9.042 L3.393,4.449 C3.702,3.702 4.432,3.215 5.240,3.215 Z"/>
> +    <rect fill="none" width="12.219" height="4.137" x="1.395" y="7.671" stroke="currentColor" rx="2"/>

I like this icon, but I have some concerns with it particularly @1x. I'll reach out "offline" to provide some images of what I'm seeing, but it would be nice if the lines that are visually horizontal in here were snapped to pixels at both @1x and @2x. Also the little indicator lights on the drive are a mess @1x.

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:259
> +        this._loadFromFileSystem(true).then(() => {

Nit:
```
const forceUpdate = true;
this._loadFromFileSystem(forceUpdate).then(() => {`
```

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:301
> +            if (forceUpdate || this._didWarnAboutFailureToLoadFromFileSystem)

Nit: `_didWarnAboutFailureToLoadFromFileSystem` isn't created in the constructor.
Also, is there a specific reason we don't notify the user of an error when `forceUpdate` is enabled? It seems like if while setting a new mapped file we encounter an error that is is probably worth telling the user (and probably related to file permissions).

> Source/WebKit/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:124
>  - (void)loadForDebuggable:(_WKInspectorDebuggableInfo *)debuggableInfo backendCommandsURL:(NSURL *)backendCommandsURL

Should this now be called something along the lines of `initializeForDebuggable` to avoid overloading the term `load` here (and to match the new `initialize` method)?

> LayoutTests/platform/mac-wk1/TestExpectations:928
> +http/tests/inspector/network/local-resource-override-mapped-to-file.html [ Skip ]

Is this actually necessary since the base expectations also mark this test as `[ Skip ]`?

[Devin Rousso]

Comment on attachment 455704 [details]
[fast-cq] Patch

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

>> Source/WebCore/testing/Internals.cpp:4867
>> +    auto path = FileSystem::openTemporaryFile("WebCoreTesting", file);
> 
> I wonder if we should require the test to provide a name for the temporary file so that if you needed two temp files the second one wouldn't clobber the first? Not a problem currently, but I could see the assumption being made that you can create multiple files when you really can't.

Hmm I coulda sworn that `FileSystem::openTemporaryFile` appended some random characters to the path, but I guess it does that for all platforms except cocoa.  Yeah asking the test to provide a name is a good idea :)

>> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:301
>> +            if (forceUpdate || this._didWarnAboutFailureToLoadFromFileSystem)
> 
> Nit: `_didWarnAboutFailureToLoadFromFileSystem` isn't created in the constructor.
> Also, is there a specific reason we don't notify the user of an error when `forceUpdate` is enabled? It seems like if while setting a new mapped file we encounter an error that is is probably worth telling the user (and probably related to file permissions).

The only time we provide `forceUpdate` is when we're calling `set mappedFilePath`, which is only called in response to an `<input type="file">`, meaning that the path we've been given cannot be bogus.  But yeah, good call in case there's something like permissions issues.

>> Source/WebKit/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:124
>>  - (void)loadForDebuggable:(_WKInspectorDebuggableInfo *)debuggableInfo backendCommandsURL:(NSURL *)backendCommandsURL
> 
> Should this now be called something along the lines of `initializeForDebuggable` to avoid overloading the term `load` here (and to match the new `initialize` method)?

This is used by other projects, so I'm trying not to affect them.

>> LayoutTests/platform/mac-wk1/TestExpectations:928
>> +http/tests/inspector/network/local-resource-override-mapped-to-file.html [ Skip ]
> 
> Is this actually necessary since the base expectations also mark this test as `[ Skip ]`?

No, but I did it to keep consistent with the other local override tests (also just in case other platforms adopt this feature in the future they won't have to worry about this).

[Patrick Angle]

Comment on attachment 455704 [details]
[fast-cq] Patch

rs=me

[Devin Rousso]

Created attachment 456081 [details]
[fast-cq] Patch

[EWS]

Committed r292084 (249011@main): <https://commits.webkit.org/249011@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456081 [details].