RESOLVED FIXED 115865
Web Inspector: implement runOpenPanel callback for WebKit2-hosted inspector page
https://bugs.webkit.org/show_bug.cgi?id=115865
Summary Web Inspector: implement runOpenPanel callback for WebKit2-hosted inspector page
Brian Burg
Reported 2013-05-09 10:41:43 PDT
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.
Attachments
Patch v1 (4.04 KB, patch)
2013-05-09 15:52 PDT, Brian Burg
no flags
Patch v1b (4.17 KB, patch)
2013-05-09 19:00 PDT, Brian Burg
no flags
Patch v2 (4.16 KB, patch)
2013-05-10 10:18 PDT, Brian Burg
no flags
Brian Burg
Comment 1 2013-05-09 15:52:20 PDT
Created attachment 201295 [details] Patch v1
Timothy Hatcher
Comment 2 2013-05-09 16:36:38 PDT
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.
Brian Burg
Comment 3 2013-05-09 18:59:29 PDT
(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 *')"
Brian Burg
Comment 4 2013-05-09 19:00:41 PDT
Created attachment 201310 [details] Patch v1b
Timothy Hatcher
Comment 5 2013-05-10 04:36:08 PDT
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.
Brian Burg
Comment 6 2013-05-10 09:10:32 PDT
(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.
Brian Burg
Comment 7 2013-05-10 10:18:55 PDT
Created attachment 201358 [details] Patch v2
Timothy Hatcher
Comment 8 2013-05-10 10:30:02 PDT
Comment on attachment 201358 [details] Patch v2 Looks good. Needs a WK2 owner to r+.
WebKit Commit Bot
Comment 9 2013-05-10 13:34:24 PDT
Comment on attachment 201358 [details] Patch v2 Clearing flags on attachment: 201358 Committed r149903: <http://trac.webkit.org/changeset/149903>
WebKit Commit Bot
Comment 10 2013-05-10 13:34:26 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.