Bug 179904 – [Cocoa] Web Inspector: load inspector resources using a custom scheme handler

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

[Blaze Burg]

.

[Blaze Burg]

<rdar://problem/10887211>

[Blaze Burg]

Created attachment 327643 [details]
WIP patch

[EWS Watchlist]

Attachment 327643 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/WebPage/mac/WebInspectorUIMac.mm:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/UIProcess/mac/WKInspectorViewController.mm:47:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 327643 [details]
WIP patch

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

> Source/WebKit/UIProcess/mac/WKInspectorViewController.mm:186
> +    [configuration setURLSchemeHandler:[WKInspectorURLSchemeHandler new] forURLScheme:WKInspectorURLSchemeProtocol];

This is a memory leak.

[Blaze Burg]

Created attachment 423388 [details]
Patch v1.0

[Blaze Burg]

In a separate patch, I will remove the "assume access to the base URL" code that is no longer needed after this change. (It requires cross-port changes that are trickier to get right than this Cocoa only change.)

[Geoffrey Garen]

Comment on attachment 423388 [details]
Patch v1.0

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

r=me

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:89
> +    [[NSOperationQueue mainQueue] addOperation:_fileLoadOperation];

Let's do this file access off the main thread to keep the app responsive.

[Devin Rousso]

Comment on attachment 423388 [details]
Patch v1.0

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

r=me as well

I don't think there'd be any issues but please make sure that this works with the various `Worker` that the frontend creates (e.g. formatter, heap snapshot, etc.)

> Source/WebKit/ChangeLog:15
> +        This also has the effect of resetting Web Inspector settings since
> +        they are no longer associated with the file:/// protocol.

This is scary/unfortunate.  Any way to avoid it?  Maybe check to see if `inspector-resource://` exists and if not duplicate the `file://` data?

> Source/WebKit/ChangeLog:17
> +        Covered by existing inspector tests which exercise this code path.

I don't think this is accurate actually 😅

IIRC, both `WI.Setting` and `WI.ObjectStore` have checks for `!window.InspectorTest` before they attempt to actually write to `localStorage` and `indexedDB`.

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.h:31
> +
> +

Style: double whitespace

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:41
> +// MARK - WKURLSchemeHandler Protocol

NIT: Is it preferred to use `#pragma mark - ` or `// MARK - `?

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:76
> +        if (!mimeType)
> +            mimeType = @"application/octet-stream";

o_0

[Joseph Pecoraro]

Comment on attachment 423388 [details]
Patch v1.0

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

>> Source/WebKit/ChangeLog:17
>> +        Covered by existing inspector tests which exercise this code path.
> 
> I don't think this is accurate actually 😅
> 
> IIRC, both `WI.Setting` and `WI.ObjectStore` have checks for `!window.InspectorTest` before they attempt to actually write to `localStorage` and `indexedDB`.

+1!

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:37
> +    NSOperation *_fileLoadOperation;

It seems like there can be a set of ongoing file load operations at the same time, especially if this is being dispatched to another queue. So it seems like this should probably be a map (URL -> Operation).

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:46
> +        _cachedBundle = [NSBundle bundleWithIdentifier:@"com.apple.WebInspectorUI"];

Is this ARC? If not this should probably be a RetainPtr. It is an implementation detail, that could go away in the future, that NSBundles are never actually released.

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:78
> +        NSHTTPURLResponse *urlResponse = [[NSHTTPURLResponse alloc] initWithURL:urlSchemeTask.request.URL statusCode:200 HTTPVersion:nil headerFields:@{

Is this ARC? If not this should be a RetainPtr or released.

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm:32
> +#import "Logging.h"

Needed?

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm:52
> +#import <WebCore/MimeTypeRegistry.h>
>  #import <WebCore/VersionChecks.h>
> +#import <wtf/Assertions.h>

Seems a few of these imports are stale from earlier development.

[Blaze Burg]

Comment on attachment 423388 [details]
Patch v1.0

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

>> Source/WebKit/ChangeLog:15
>> +        they are no longer associated with the file:/// protocol.
> 
> This is scary/unfortunate.  Any way to avoid it?  Maybe check to see if `inspector-resource://` exists and if not duplicate the `file://` data?

I've tried every year for the past few years and couldn't shove it through. I don't think it's important enough to block moving forward. There are non-user facing reasons to make this change.

>>> Source/WebKit/ChangeLog:17
>>> +        Covered by existing inspector tests which exercise this code path.
>> 
>> I don't think this is accurate actually 😅
>> 
>> IIRC, both `WI.Setting` and `WI.ObjectStore` have checks for `!window.InspectorTest` before they attempt to actually write to `localStorage` and `indexedDB`.
> 
> +1!

I meant in the file loading sense. Please explain what extra tests would be meaningful.

>> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:37
>> +    NSOperation *_fileLoadOperation;
> 
> It seems like there can be a set of ongoing file load operations at the same time, especially if this is being dispatched to another queue. So it seems like this should probably be a map (URL -> Operation).

I'm not sure how this could happen given the current architecture of one big page with sync CSS and script tags. (I originally implemented it that way but found it unnecessary.)

>> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:41
>> +// MARK - WKURLSchemeHandler Protocol
> 
> NIT: Is it preferred to use `#pragma mark - ` or `// MARK - `?

MARK is newer and preferred

>> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:46
>> +        _cachedBundle = [NSBundle bundleWithIdentifier:@"com.apple.WebInspectorUI"];
> 
> Is this ARC? If not this should probably be a RetainPtr. It is an implementation detail, that could go away in the future, that NSBundles are never actually released.

OK

>> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:78
>> +        NSHTTPURLResponse *urlResponse = [[NSHTTPURLResponse alloc] initWithURL:urlSchemeTask.request.URL statusCode:200 HTTPVersion:nil headerFields:@{
> 
> Is this ARC? If not this should be a RetainPtr or released.

OK

[Blaze Burg]

Created attachment 423651 [details]
Patch v1.1 (for EWS)

[Blaze Burg]

Created attachment 423656 [details]
Patch v1.2 (for EWS)

[Blaze Burg]

Created attachment 423668 [details]
Patch v1.3 - use concurrent global queue

[EWS]

Committed r274697: <https://commits.webkit.org/r274697>

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