WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug