.
<rdar://problem/10887211>
Created attachment 327643 [details] WIP patch
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.
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.
Created attachment 423388 [details] Patch v1.0
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.)
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.
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
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.
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
Created attachment 423651 [details] Patch v1.1 (for EWS)
Created attachment 423656 [details] Patch v1.2 (for EWS)
Created attachment 423668 [details] Patch v1.3 - use concurrent global queue
Committed r274697: <https://commits.webkit.org/r274697> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423668 [details].