To reproduce: open the legacy inspector, right click on the timelines panel to load timeline data. Nothing will happen. The WKPageUIClient defined in WebInspectorProxy::createInspectorPage() has no callback for runOpenPanel (triggered by file selector element). The Mac WK1 code for this can probably be reused with some adaptation. It is located in Source/WebKit/WebCoreSupport/WebInspectorClient.mm:687.
Created attachment 201295 [details] Patch v1
Comment on attachment 201295 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=201295&action=review Looking good! > Source/WebKit2/UIProcess/WebInspectorProxy.h:99 > + NSWindow* inspectorWindow() const { return m_inspectorWindow.get(); } This will only be non-nil when the Inspector is detached. I assume that is what you wanted, since having the open panel as a sheet on the Safari window when docked would be odd. > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:37 > +#import "WKRetainPtr.h" I don't think you use this? You should if you can. > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:244 > + NSURL *nsURL; > + for (nsURL in [openPanel URLs]) { You can put the type in the for(). > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:245 > + WKURLRef wkURL = WKURLCreateWithCFURL((CFURLRef)nsURL); Use static_cast<CFURLRef>(nsURL) instead.
(In reply to comment #2) > (From update of attachment 201295 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201295&action=review > > > Source/WebKit2/UIProcess/WebInspectorProxy.h:99 > > + NSWindow* inspectorWindow() const { return m_inspectorWindow.get(); } > > This will only be non-nil when the Inspector is detached. I assume that is what you wanted, since having the open panel as a sheet on the Safari window when docked would be odd. I added an assert and early exit in this unlikely case. > > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:37 > > +#import "WKRetainPtr.h" > > I don't think you use this? You should if you can. It's used to refcount the listener object. > > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:245 > > + WKURLRef wkURL = WKURLCreateWithCFURL((CFURLRef)nsURL); > > Use static_cast<CFURLRef>(nsURL) instead. I can't :-( I think this is related to __CFURL being an opaque type. Normally, for such toll-free bridged types I wouldn't expect a cast to be required. So, I changed it to reinterpret_cast. "Cannot cast from type 'NSURL *' to pointer type 'CFURLRef' (aka 'const __CFURL *')"
Created attachment 201310 [details] Patch v1b
Comment on attachment 201310 [details] Patch v1b View in context: https://bugs.webkit.org/attachment.cgi?id=201310&action=review > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:237 > + if (!inspectorWindow) Sorry, I didn't mean to cause confusion. What you had before was fine. If window is nil (which it is when the Inspector is docked) passing nil to NSOpenPanel will create a modal dialog instead of a sheet. Now with this patch Open does not work when docked. You can remove the ASSERT and early return. I just wanted you to understand and test the docked vs windowed case and make sure you get what you expect.
(In reply to comment #5) > (From update of attachment 201310 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201310&action=review > > > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:237 > > + if (!inspectorWindow) > > Sorry, I didn't mean to cause confusion. What you had before was fine. If window is nil (which it is when the Inspector is docked) passing nil to NSOpenPanel will create a modal dialog instead of a sheet. Now with this patch Open does not work when docked. You can remove the ASSERT and early return. > > I just wanted you to understand and test the docked vs windowed case and make sure you get what you expect. Confusion leads to understanding. :-) In this case, I wasn't aware that behavior would happen with a nil window argument. I couldn't find any documentation of what happens with nil window (seems I'm not the only one: http://stackoverflow.com/questions/671204/why-isnt-my-sheet-attached-to-the-window-its-run-for?rq=1). I'll revert that guard, add a comment to document the intended modal behaviors, and test on a bigger screen.
Created attachment 201358 [details] Patch v2
Comment on attachment 201358 [details] Patch v2 Looks good. Needs a WK2 owner to r+.
Comment on attachment 201358 [details] Patch v2 Clearing flags on attachment: 201358 Committed r149903: <http://trac.webkit.org/changeset/149903>
All reviewed patches have been landed. Closing bug.