Bug 115865 – Web Inspector: implement runOpenPanel callback for WebKit2-hosted inspector page

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Brian Burg]

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.

[Brian Burg]

Created attachment 201295 [details]
Patch v1

[Timothy Hatcher]

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]

(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]

Created attachment 201310 [details]
Patch v1b

[Timothy Hatcher]

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]

(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]

Created attachment 201358 [details]
Patch v2

[Timothy Hatcher]

Comment on attachment 201358 [details]
Patch v2

Looks good. Needs a WK2 owner to r+.

[WebKit Commit Bot]

Comment on attachment 201358 [details]
Patch v2

Clearing flags on attachment: 201358

Committed r149903: <http://trac.webkit.org/changeset/149903>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.