Bug 14270 - Impossible to close web inspector when docked to webview
: Impossible to close web inspector when docked to webview
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 523.x (Safari 3)
: All All
: P2 Normal
Assigned To:
:
: InRadar
:
: 19477
  Show dependency treegraph
 
Reported: 2007-06-21 00:04 PST by
Modified: 2008-08-13 11:32 PST (History)


Attachments
patch (14.59 KB, patch)
2008-01-30 11:50 PST, Matt Lilek (pewtermoose)
aroben: review-
Review Patch | Details | Formatted Diff | Diff
revised patch (7.63 KB, patch)
2008-01-31 16:40 PST, Matt Lilek (pewtermoose)
no flags Review Patch | 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 PST, Timothy Hatcher
kmccullough: review+
Review Patch | Details | Formatted Diff | Diff
Picture (25.76 KB, image/png)
2008-08-12 23:50 PST, Timothy Hatcher
no flags Details
Proposed patch (part 2) (2.17 KB, patch)
2008-08-13 00:17 PST, Timothy Hatcher
kmccullough: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2007-06-21 00:04:49 PST
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 From 2008-01-29 10:55:28 PST -------
<rdar://problem/5712792>
------- Comment #2 From 2008-01-30 11:50:02 PST -------
Created an attachment (id=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 From 2008-01-30 12:06:10 PST -------
(From update of attachment 18795 [details])
+        { "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 From 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 From 2008-01-31 16:40:19 PST -------
Created an attachment (id=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 From 2008-01-31 16:41:59 PST -------
Created an attachment (id=18837) [details]
screenshot

screenshot - not exactly a whole lot to show
------- Comment #7 From 2008-02-08 15:38:50 PST -------
(From update of attachment 18836 [details])
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 From 2008-06-11 05:13:36 PST -------
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 From 2008-08-12 23:45:48 PST -------
Created an attachment (id=22768) [details]
Proposed patch
------- Comment #10 From 2008-08-12 23:50:00 PST -------
Created an attachment (id=22769) [details]
Picture
------- Comment #11 From 2008-08-13 00:17:34 PST -------
Created an attachment (id=22770) [details]
Proposed patch (part 2)
------- Comment #12 From 2008-08-13 00:41:11 PST -------
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 From 2008-08-13 00:54:36 PST -------
No, this just adds a close button. Please file a Safari bug for that, since it is a Safari key command.
------- Comment #14 From 2008-08-13 11:32:42 PST -------
Landed in r35720 and r35721.