Summary: | Web Inspector: implement runOpenPanel callback for WebKit2-hosted inspector page | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brian Burg <burg> | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, graouts, joepeck, timothy | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.8 | ||||||||||
Attachments: |
|
Description
Brian Burg
2013-05-09 10:41:43 PDT
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. |