Bug 14270 – Impossible to close web inspector when docked to webview

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

[Matt Lilek]

There should be a way to close the web inspector when it's docked to a webview without having to undock it first.

[Adam Roben (:aroben)]

<rdar://problem/5712792>

[Matt Lilek]

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.

[Adam Roben (:aroben)]

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?

[Adam Roben (:aroben)]

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

[Matt Lilek]

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.

[Matt Lilek]

Created attachment 18837 [details]
screenshot

screenshot - not exactly a whole lot to show

[Adam Roben (:aroben)]

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.

[Adam Strzelecki]

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.

[Timothy Hatcher]

Created attachment 22768 [details]
Proposed patch

[Timothy Hatcher]

Created attachment 22769 [details]
Picture

[Timothy Hatcher]

Created attachment 22770 [details]
Proposed patch (part 2)

[Adam Strzelecki]

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).

[Timothy Hatcher]

No, this just adds a close button. Please file a Safari bug for that, since it is a Safari key command.

[Timothy Hatcher]

Landed in r35720 and r35721.