Bug 238236

<rdar://problem/59009154>

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

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

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

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

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

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

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

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

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

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

rs=me

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

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