Bug 115865 - Web Inspector: implement runOpenPanel callback for WebKit2-hosted inspector page
Summary: Web Inspector: implement runOpenPanel callback for WebKit2-hosted inspector page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.8
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-09 10:41 PDT by Brian Burg
Modified: 2013-05-10 13:34 PDT (History)
4 users (show)

See Also:


Attachments
Patch v1 (4.04 KB, patch)
2013-05-09 15:52 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Patch v1b (4.17 KB, patch)
2013-05-09 19:00 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Patch v2 (4.16 KB, patch)
2013-05-10 10:18 PDT, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 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.
Comment 1 Brian Burg 2013-05-09 15:52:20 PDT
Created attachment 201295 [details]
Patch v1
Comment 2 Timothy Hatcher 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.
Comment 3 Brian Burg 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 *')"
Comment 4 Brian Burg 2013-05-09 19:00:41 PDT
Created attachment 201310 [details]
Patch v1b
Comment 5 Timothy Hatcher 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.
Comment 6 Brian Burg 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.
Comment 7 Brian Burg 2013-05-10 10:18:55 PDT
Created attachment 201358 [details]
Patch v2
Comment 8 Timothy Hatcher 2013-05-10 10:30:02 PDT
Comment on attachment 201358 [details]
Patch v2

Looks good. Needs a WK2 owner to r+.
Comment 9 WebKit Commit Bot 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>
Comment 10 WebKit Commit Bot 2013-05-10 13:34:26 PDT
All reviewed patches have been landed.  Closing bug.