Bug 14270 - Impossible to close web inspector when docked to webview
: Impossible to close web inspector when docked to webview
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated)
: 523.x (Safari 3)
: All All
: P2 Normal
Assigned To: Timothy Hatcher
: InRadar
Depends on:
Blocks: 19477
  Show dependency treegraph
 
Reported: 2007-06-21 00:04 PDT by Matt Lilek (pewtermoose)
Modified: 2008-08-13 11:32 PDT (History)
5 users (show)

See Also:


Attachments
patch (14.59 KB, patch)
2008-01-30 11:50 PST, Matt Lilek (pewtermoose)
aroben: review-
Details | Formatted Diff | Diff
revised patch (7.63 KB, patch)
2008-01-31 16:40 PST, Matt Lilek (pewtermoose)
no flags Details | Formatted Diff | Diff
screenshot (11.45 KB, image/png)
2008-01-31 16:41 PST, Matt Lilek (pewtermoose)
no flags Details
Proposed patch (14.25 KB, patch)
2008-08-12 23:45 PDT, Timothy Hatcher
kmccullough: review+
Details | Formatted Diff | Diff
Picture (25.76 KB, image/png)
2008-08-12 23:50 PDT, Timothy Hatcher
no flags Details
Proposed patch (part 2) (2.17 KB, patch)
2008-08-13 00:17 PDT, Timothy Hatcher
kmccullough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Lilek (pewtermoose) 2007-06-21 00:04:49 PDT
There should be a way to close the web inspector when it's docked to a webview without having to undock it first.
Comment 1 Adam Roben (:aroben) 2008-01-29 10:55:28 PST
<rdar://problem/5712792>
Comment 2 Matt Lilek (pewtermoose) 2008-01-30 11:50:02 PST
Created attachment 18795 [details]
patch

Add a close button to the toolbar when the inspector is docked.  After the inspector is closed this way, it reopens docked. I think that makes the most sense but I'd be more than happy to listen to reasons for why it should be another way.

The images used for the close button are from Drosera, renamed and converted to pngs to match the rest of the inspector's images.

Build fix stubs of the new client method for non-mac/win platforms are not included in this patch, but I have them locally.
Comment 3 Adam Roben (:aroben) 2008-01-30 12:06:10 PST
Comment on attachment 18795 [details]
patch

+        { "attachedClose", attachedClose, kJSPropertyAttributeNone },

Maybe "hide" would be a better name for this method?

I think the Windows code could be better factored to share more code between closeWindow and closeAttachedWindow. It also seems strange to have to call ShowWindow(SW_HIDE) in closeAttachedWindow() -- won't th window already be hidden?

Is there any flashing when m_attachOnShow is true? I'd expect to see the detached window appear, then disappear given the way the code is written.

Any chance we could see some screenshots of the new close button in action?
Comment 4 Adam Roben (:aroben) 2008-01-30 12:08:28 PST
(In reply to comment #3)
> (From update of attachment 18795 [details] [edit])
> +        { "attachedClose", attachedClose, kJSPropertyAttributeNone },
> 
> Maybe "hide" would be a better name for this method?

In fact, maybe the button should even be a sort of "minimize" button, since presumably the effect is that the Inspector animates to 0 height and disappears.
Comment 5 Matt Lilek (pewtermoose) 2008-01-31 16:40:19 PST
Created attachment 18836 [details]
revised patch

This is a revised patch based on Adam's comments, but I'm not putting it up for official review.  It's mainly to see whether my Windows code is horribly wrong or not.

(In reply to comment #3)
> (From update of attachment 18795 [details] [edit])
> +        { "attachedClose", attachedClose, kJSPropertyAttributeNone },
> 
> Maybe "hide" would be a better name for this method?
> 
> I think the Windows code could be better factored to share more code between
> closeWindow and closeAttachedWindow. It also seems strange to have to call
> ShowWindow(SW_HIDE) in closeAttachedWindow() -- won't th window already be
> hidden?
> 

Thats what you'd think, but I'm not 100% sure - I don't really know enough about Windows' windowing system.  If my revised code is still wrong, I'd love pointers to figuring out what I'm screwing up.

> Is there any flashing when m_attachOnShow is true? I'd expect to see the
> detached window appear, then disappear given the way the code is written.
> 

Yes, I overlooked that in my initial patch.  It's taken care of in this revision.

> Any chance we could see some screenshots of the new close button in action?
> 

I'll upload a screenshot of it in a moment. If the general style is okay, I'll photoshop it to be a - instead of an X.
Comment 6 Matt Lilek (pewtermoose) 2008-01-31 16:41:59 PST
Created attachment 18837 [details]
screenshot

screenshot - not exactly a whole lot to show
Comment 7 Adam Roben (:aroben) 2008-02-08 15:38:50 PST
Comment on attachment 18836 [details]
revised patch

I don't think you need the m_shouldAttachOnShow member. m_attached should already have the value you want.

I think to make the code clearer, we should separate these two concepts:

1. Moving the Inspector's WebView into/out of the inspected WebView's window
2. Moving the Inspector's WebView into/out of the Inspector's own window

So we could have private methods like:

insertIntoInspectedWindow()
removeFromInspectedWindow()
insertIntoOwnWindow()
removeFromOwnWindow()

If we do that, then I think we'll have these nice simple (pseudocode) implementations of the public methods:

WebInspectorClient::attachWindow() {
    removeFromOwnWindow();
    ShowWindow(m_hwnd, SW_HIDE);
    insertIntoInspectedWindow();
}

WebInspectorClient::detachWindow() {
    removeFromInspectedWindow();
    insertIntoOwnWindow();
    ShowWindow(m_hwnd, SW_SHOW);
}

WebInspectorClient::hideWindow() {
    removeFromInspectedWindow();
}

Depending on how the methods are coded you might need to keep track of some state to know whether you're actually in a window at all (since you won't be after hideWindow() is called). But I think this should make things much cleaner.
Comment 8 Adam Strzelecki 2008-06-11 05:13:36 PDT
Any news to this bug? With latest WebKit it is still impossible to close docked inspector. Also it isn't possible to resize docked inspector.

Would be nice if I dock the inspector Opt+Cmd+I hides inspector, Opt+Cmd+I shows it again docked.
Comment 9 Timothy Hatcher 2008-08-12 23:45:48 PDT
Created attachment 22768 [details]
Proposed patch
Comment 10 Timothy Hatcher 2008-08-12 23:50:00 PDT
Created attachment 22769 [details]
Picture
Comment 11 Timothy Hatcher 2008-08-13 00:17:34 PDT
Created attachment 22770 [details]
Proposed patch (part 2)
Comment 12 Adam Strzelecki 2008-08-13 00:41:11 PDT
Thanks for the patch Timothy, looks great. However does it close the inspector on Opt+Cmd+I as well? (So on docked mode Opt+Cmd+I is a key to toggle the inspector, rather just to open it).
Comment 13 Timothy Hatcher 2008-08-13 00:54:36 PDT
No, this just adds a close button. Please file a Safari bug for that, since it is a Safari key command.
Comment 14 Timothy Hatcher 2008-08-13 11:32:42 PDT
Landed in r35720 and r35721.