.
<rdar://problem/70355910>
Created attachment 411726 [details] Patch Still need to finish the API test for this, but it's mostly done.
Created attachment 411933 [details] Patch v2.0
Comment on attachment 411933 [details] Patch v2.0 View in context: https://bugs.webkit.org/attachment.cgi?id=411933&action=review > Source/WebKit/ChangeLog:11 > + WebView to load resources from from ... ? > Source/WebKit/Shared/API/APIObject.h:387 > +#if ENABLE(INSPECTOR_EXTENSIONS) > + API::Object::Type::InspectorExtension, > +#endif this doesn't exist yet, so please either add a blocking bug that adds this or remove it from this patch > Source/WebKit/UIProcess/API/APIInspectorConfiguration.h:38 > +using URLSchemeHandlerPair = std::pair<Ref<WebKit::WebURLSchemeHandler>, WTF::String>; NIT: I'd expect the `String` to be first, as the `WebURLSchemeHandler` wouldn't know what to do without first knowing the scheme it's handling (i.e. `std::pair<[the scheme], [the handler for that scheme]>`) We also might consider renaming this to have `Inspector` in it so that it doesn't have the potential to clash elsewhere. > Source/WebKit/UIProcess/API/Cocoa/_WKInspectorConfiguration.mm:62 > + auto& handler = static_cast<WebKit::WebURLSchemeHandlerCocoa&>(pair.first.get()); Is the `static_cast` necessary? Why not just ``` for (auto& [handler, scheme] : _configuration->urlSchemeHandlers()) [configuration setURLSchemeHandler:handler->apiHandler() forURLScheme:scheme]; ``` > Source/WebKit/UIProcess/API/Cocoa/_WKInspectorConfiguration.mm:72 > + auto& handler = static_cast<WebKit::WebURLSchemeHandlerCocoa&>(pair.first.get()); ditto (:62) > Source/WebKit/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:175 > +- (_WKInspectorConfiguration *)configurationForDebuggable:(_WKInspectorDebuggableInfo *)debuggableInfo Do we really need to base all of this on `_WKInspectorDebuggableInfo`? It's not a very specific object (i.e. multiple unique debuggables can have the same `_WKInspectorDebuggableInfo` data (different instance same values)). Seeing as this patch doesn't even do anything with it, can should we just drop it and have it be `-inspectorConfiguration` instead? > Source/WebKit/UIProcess/Inspector/Cocoa/InspectorDelegate.h:61 > + void browserDomainEnabled(WebInspectorProxy&) override; NIT: this class is `final` so you can drop the `override` > Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:1 > +/* I'm guessing this wasn't intended to be included in this patch 😅 > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorDelegate.mm:248 > + // we can used the creteTab API to load the custom URL scheme resource in an iframe. "createTab"
Comment on attachment 411933 [details] Patch v2.0 View in context: https://bugs.webkit.org/attachment.cgi?id=411933&action=review >> Source/WebKit/ChangeLog:11 >> + WebView to load resources from > > from ... ? Oops >> Source/WebKit/Shared/API/APIObject.h:387 >> +#endif > > this doesn't exist yet, so please either add a blocking bug that adds this or remove it from this patch I'll rebase w/o it. >> Source/WebKit/UIProcess/API/APIInspectorConfiguration.h:38 >> +using URLSchemeHandlerPair = std::pair<Ref<WebKit::WebURLSchemeHandler>, WTF::String>; > > NIT: I'd expect the `String` to be first, as the `WebURLSchemeHandler` wouldn't know what to do without first knowing the scheme it's handling (i.e. `std::pair<[the scheme], [the handler for that scheme]>`) > > We also might consider renaming this to have `Inspector` in it so that it doesn't have the potential to clash elsewhere. I would expect that order too, but all the API calls and other uses of scheme handlers put the handler first. So I did it this way to be consistent with similar code. >> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorConfiguration.mm:62 >> + auto& handler = static_cast<WebKit::WebURLSchemeHandlerCocoa&>(pair.first.get()); > > Is the `static_cast` necessary? Why not just > ``` > for (auto& [handler, scheme] : _configuration->urlSchemeHandlers()) > [configuration setURLSchemeHandler:handler->apiHandler() forURLScheme:scheme]; > ``` Yes, it's a downcast. apiHandler(), which is used below, is defined on the Cocoa subclass not on the base class. >> Source/WebKit/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:175 >> +- (_WKInspectorConfiguration *)configurationForDebuggable:(_WKInspectorDebuggableInfo *)debuggableInfo > > Do we really need to base all of this on `_WKInspectorDebuggableInfo`? It's not a very specific object (i.e. multiple unique debuggables can have the same `_WKInspectorDebuggableInfo` data (different instance same values)). Seeing as this patch doesn't even do anything with it, can should we just drop it and have it be `-inspectorConfiguration` instead? It made the most sense to me as something to provide as a context object (the thing that's about to be inspected). WebKit clients could avoid setting URL scheme handlers when they are not needed, based on debuggable version and platform. >> Source/WebKit/UIProcess/Inspector/Cocoa/InspectorDelegate.h:61 >> + void browserDomainEnabled(WebInspectorProxy&) override; > > NIT: this class is `final` so you can drop the `override` Oops >> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:1 >> +/* > > I'm guessing this wasn't intended to be included in this patch 😅 ugh, git >> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorDelegate.mm:248 >> + // we can used the creteTab API to load the custom URL scheme resource in an iframe. > > "createTab" LOL
Created attachment 412010 [details] Patch v2.1 I'm going to try moving the configurationForDebuggable thing to WKUIDelegatePrivate. As implemented, I don't think the client would be able to supply a configuration for a secondary WebView if it were immediately shown (aka no chance to set inspector.delegate before the delegate is asked for a configuration.
Created attachment 412036 [details] Patch v3.0 New patch moves the delegate method to WKUIDelegatePrivate.
Created attachment 412037 [details] Patch v3.1 Fix GTK/WPE/WinCairo
Created attachment 412042 [details] Patch v3.2 Fix GTK/WPE/WinCairo harder
Comment on attachment 412042 [details] Patch v3.2 View in context: https://bugs.webkit.org/attachment.cgi?id=412042&action=review r=me, nice work! I think this is a much better design than using `_WKDebuggableInfo` =D > Source/WebKit/UIProcess/API/APIInspectorConfiguration.h:38 > +using URLSchemeHandlerPair = std::pair<Ref<WebKit::WebURLSchemeHandler>, WTF::String>; I'd recommend renaming this to have `Inspector` in it so that it doesn't have the potential to clash elsewhere. > Source/WebKit/UIProcess/API/Cocoa/_WKInspectorConfiguration.mm:62 > + auto& handler = static_cast<WebKit::WebURLSchemeHandlerCocoa&>(pair.first.get()); Is there any way we can check/assert that `pair.first` is in fact a `WebURLSchemeHandlerCocoa`, or is this a common pattern in API code? > Source/WebKit/UIProcess/API/Cocoa/_WKInspectorConfiguration.mm:72 > + auto& handler = static_cast<WebKit::WebURLSchemeHandlerCocoa&>(pair.first.get()); ditto (:62) > Source/WebKit/UIProcess/Inspector/mac/RemoteWebInspectorProxyMac.mm:107 > + Ref<API::InspectorConfiguration> configuration = m_client->configurationForRemoteInspector(*this); `auto`? > Source/WebKit/UIProcess/Inspector/mac/RemoteWebInspectorProxyMac.mm:108 > + m_inspectorView = adoptNS([[WKInspectorViewController alloc] initWithConfiguration: WebKit::wrapper(configuration) inspectedPage:nullptr]); Style: extra space > Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm:65 > // The (local) inspected page is nil if the controller is hosting a Remote Web Inspector view. Style: I'd add a newline before this > Source/WebKit/UIProcess/Inspector/mac/WebInspectorProxyMac.mm:299 > + Ref<API::InspectorConfiguration> configuration = inspectedPage()->uiClient().configurationForLocalInspector(*inspectedPage(), *this); `auto`? > Source/WebKit/UIProcess/Inspector/mac/WebInspectorProxyMac.mm:300 > + m_inspectorViewController = adoptNS([[WKInspectorViewController alloc] initWithConfiguration: WebKit::wrapper(configuration.get()) inspectedPage:inspectedPage()]); Style: extra space > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorDelegate.mm:251 > + configurationForLocalInspectorCalled = false; > + didAttachLocalInspectorCalled = false; Do we still need these here now that there's a `resetGlobalState()` above? > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorDelegate.mm:261 > + startURLSchemeTaskCalled = false; ditto (:250)
Comment on attachment 412042 [details] Patch v3.2 View in context: https://bugs.webkit.org/attachment.cgi?id=412042&action=review >> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorConfiguration.mm:62 >> + auto& handler = static_cast<WebKit::WebURLSchemeHandlerCocoa&>(pair.first.get()); > > Is there any way we can check/assert that `pair.first` is in fact a `WebURLSchemeHandlerCocoa`, or is this a common pattern in API code? We know it's that type, because the above line is the only place where we append to the pair vector. _configuration->addURLSchemeHandler(WebKit::WebURLSchemeHandlerCocoa::create(urlSchemeHandler), urlScheme); >> Source/WebKit/UIProcess/Inspector/mac/RemoteWebInspectorProxyMac.mm:108 >> + m_inspectorView = adoptNS([[WKInspectorViewController alloc] initWithConfiguration: WebKit::wrapper(configuration) inspectedPage:nullptr]); > > Style: extra space The extra space is necessary for this to parse, unfortunately. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorDelegate.mm:251 >> + didAttachLocalInspectorCalled = false; > > Do we still need these here now that there's a `resetGlobalState()` above? I don't think so.
Created attachment 412128 [details] Patch v3.3 [FOR LANDING]
Created attachment 412130 [details] Patch v3.3 [FOR LANDING]
Committed r269068: <https://trac.webkit.org/changeset/269068> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412130 [details].
Comment on attachment 412130 [details] Patch v3.3 [FOR LANDING] View in context: https://bugs.webkit.org/attachment.cgi?id=412130&action=review > Source/WebKit/UIProcess/API/Cocoa/_WKInspectorConfiguration.mm:36 > + if (!(self = [super init])) > + return nil; If [super init] returned nil then presumably the underlying class called dealloc, which will crash since _configuration is nullptr. I know we are just following a traditional pattern, and maybe the "return nil" here is dead code that is just being included for no clear reason, but I think this is not good without a null check in dealloc. > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorDelegate.mm:88 > + RetainPtr<NSURLResponse> response = adoptNS([[NSURLResponse alloc] initWithURL:task.request.URL MIMEType:@"text/html" expectedContentLength:1 textEncodingName:nil]); I suggest auto for code like this.