Bug 59134 - WebKit2: Support docked mode for Web Inspector
Summary: WebKit2: Support docked mode for Web Inspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brian Weinstein
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-04-21 13:41 PDT by Brian Weinstein
Modified: 2011-04-28 15:52 PDT (History)
13 users (show)

See Also:


Attachments
WIP Patch (14.18 KB, patch)
2011-04-21 13:43 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
[PATCH] Win fix (18.54 KB, patch)
2011-04-21 15:30 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
[PATCH] Win fix v2 (18.72 KB, patch)
2011-04-21 20:48 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
[PATCH] Win fix v3 (18.74 KB, patch)
2011-04-22 11:06 PDT, Brian Weinstein
aroben: review-
Details | Formatted Diff | Diff
[PATCH] Win Fix v4 (18.91 KB, patch)
2011-04-22 13:51 PDT, Brian Weinstein
aroben: review+
Details | Formatted Diff | Diff
[PATCH] Mac Fix v1 (17.04 KB, patch)
2011-04-27 17:48 PDT, Brian Weinstein
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Weinstein 2011-04-21 13:41:54 PDT
We need to support the docked mode for the Web Inspector in WebKit2.

<rdar://problem/8739005>
Comment 1 Brian Weinstein 2011-04-21 13:43:25 PDT
Created attachment 90590 [details]
WIP Patch
Comment 2 Adam Roben (:aroben) 2011-04-21 13:46:23 PDT
Comment on attachment 90590 [details]
WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90590&action=review

> Source/WebKit2/UIProcess/win/WebView.cpp:1513
> +HWND WebView::parentWindow()
> +{
> +    return ::GetParent(m_window);
> +}

I think you could just make people do ::GetParent(webView->nativeWindow()) and avoid adding this function.
Comment 3 Brian Weinstein 2011-04-21 13:49:11 PDT
Comment on attachment 90590 [details]
WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90590&action=review

>> Source/WebKit2/UIProcess/win/WebView.cpp:1513
>> +}
> 
> I think you could just make people do ::GetParent(webView->nativeWindow()) and avoid adding this function.

Okay. I can remove it.
Comment 4 Brian Weinstein 2011-04-21 15:30:21 PDT
Created attachment 90616 [details]
[PATCH] Win fix
Comment 5 WebKit Review Bot 2011-04-21 15:32:27 PDT
Attachment 90616 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:96:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Jeff Miller 2011-04-21 16:54:30 PDT
Comment on attachment 90616 [details]
[PATCH] Win fix

View in context: https://bugs.webkit.org/attachment.cgi?id=90616&action=review

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:152
> +    // FIXME: Implement this.

I assume you'll be implementing this next, but you might want to put a notImplemented() in here for now.

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:157
> +    // FIME: Implement this.

Ditto.

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:162
> +    // FIXME: Implement this.

Ditto.

> Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:177
> +    GetClientRect(inspectorWindow, &inspectorRect);

You should preface Win32 API calls with ::, i.e. ::GetClientRect()

> Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:182
> +    SetWindowPos(inspectorWindow, 0, windowPos->x, windowPos->y + windowPos->cy, windowPos->cx, inspectorHeight, SWP_NOZORDER);

Ditto, should be ::SetWindowPos().

> Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:295
> +    GetClientRect(inspectedWindow, &inspectedRect);

Should be ::GetClientRect().
Comment 7 Brian Weinstein 2011-04-21 20:46:48 PDT
(In reply to comment #6)
> (From update of attachment 90616 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90616&action=review
> 
> > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:152
> > +    // FIXME: Implement this.
> 
> I assume you'll be implementing this next, but you might want to put a notImplemented() in here for now.
> 
> > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:157
> > +    // FIME: Implement this.
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:162
> > +    // FIXME: Implement this.
> 
> Ditto.

Fixed all three cases of this.

> 
> > Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:177
> > +    GetClientRect(inspectorWindow, &inspectorRect);
> 
> You should preface Win32 API calls with ::, i.e. ::GetClientRect()

Fixed.

> 
> > Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:182
> > +    SetWindowPos(inspectorWindow, 0, windowPos->x, windowPos->y + windowPos->cy, windowPos->cx, inspectorHeight, SWP_NOZORDER);
> 
> Ditto, should be ::SetWindowPos().

Ditto.

> 
> > Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:295
> > +    GetClientRect(inspectedWindow, &inspectedRect);
> 
> Should be ::GetClientRect().

And fixed.

Also fixed the style-bot grievance, as much as it pains me to do so.
Comment 8 Brian Weinstein 2011-04-21 20:48:04 PDT
Created attachment 90664 [details]
[PATCH] Win fix v2
Comment 9 Adam Roben (:aroben) 2011-04-22 06:24:46 PDT
Comment on attachment 90664 [details]
[PATCH] Win fix v2

View in context: https://bugs.webkit.org/attachment.cgi?id=90664&action=review

> Source/WebKit2/UIProcess/WebInspectorProxy.h:61
> +, public WebCore::WindowMessageListener

You should indent this.

> Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:220
>  void WebInspectorProxy::platformClose()
>  {
> +    if (m_isAttached) {
> +        // Detach here so we only need to have one code path that is responsible for cleaning up the inspector
> +        // state.
> +        detach();
> +        
> +        // When the docked inspector is closed, WebInspector::close is never called, so the InspectorController
> +        // doesn't know that the inspector has been closed, and never tries to re-create it when the user
> +        // tries to open the inspector for the same view. Call close here, which sends a message to call
> +        // WebInspector::close.
> +        close();
> +    }

It seems really strange for platformClose to call close. On the surface it sounds like that would cause infinite recursion, though I see that platformClose is in fact called from didClose (so maybe it should be renamed to platformDidClose?). It would be nice to find some other way to do this.

Why is this code Windows-only?
Comment 10 Brian Weinstein 2011-04-22 10:02:11 PDT
(In reply to comment #9)
> (From update of attachment 90664 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90664&action=review
> 
> > Source/WebKit2/UIProcess/WebInspectorProxy.h:61
> > +, public WebCore::WindowMessageListener
> 
> You should indent this.

Fixed.

> 
> > Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:220
> >  void WebInspectorProxy::platformClose()
> >  {
> > +    if (m_isAttached) {
> > +        // Detach here so we only need to have one code path that is responsible for cleaning up the inspector
> > +        // state.
> > +        detach();
> > +        
> > +        // When the docked inspector is closed, WebInspector::close is never called, so the InspectorController
> > +        // doesn't know that the inspector has been closed, and never tries to re-create it when the user
> > +        // tries to open the inspector for the same view. Call close here, which sends a message to call
> > +        // WebInspector::close.
> > +        close();
> > +    }
> 
> It seems really strange for platformClose to call close. On the surface it sounds like that would cause infinite recursion, though I see that platformClose is in fact called from didClose (so maybe it should be renamed to platformDidClose?). It would be nice to find some other way to do this.

I agree, it is really strange. It has been fixed, we were missing a call at the WebProcess level, and this was a hack. It is fixed now.

> 
> Why is this code Windows-only?

When the Mac side is implemented, it probably won't be, I was just putting it there because the Windows side is implemented. It will probably move to cross-platform code when the Mac side is done.
Comment 11 Brian Weinstein 2011-04-22 11:06:06 PDT
Created attachment 90728 [details]
[PATCH] Win fix v3
Comment 12 Adam Roben (:aroben) 2011-04-22 11:17:17 PDT
Comment on attachment 90728 [details]
[PATCH] Win fix v3

View in context: https://bugs.webkit.org/attachment.cgi?id=90728&action=review

> Source/WebKit2/ChangeLog:43
> +        * WebProcess/WebCoreSupport/WebInspectorFrontendClient.cpp:
> +        (WebKit::WebInspectorFrontendClient::attachWindow): Call WebInspector::attach.
> +        (WebKit::WebInspectorFrontendClient::detachWindow): Call WebInspector::detach.
> +        (WebKit::WebInspectorFrontendClient::setAttachedWindowHeight): Call WebInspector::setAttachedWindowHeight.

No comment on closeWindow!?

> Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:216
>  void WebInspectorProxy::platformClose()
>  {
> +    if (m_isAttached) {
> +        // Detach here so we only need to have one code path that is responsible for cleaning up the inspector
> +        // state.
> +        detach();
> +    }
> +
> +    ASSERT(!m_isAttached);

Sure seems like this code should be in the cross-platform file.

> Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:269
> +    // We only want to show the inspector window if we're visible, if not, we are in the process
> +    // of being destroyed, and we don't want this window to flash.
> +    if (m_isVisible)
> +        ::ShowWindow(m_inspectorWindow, SW_SHOW);

I'm not sure the comment is needed here. Not showing the window when we're not visible makes sense even if the "being destroyed" case isn't considered.

> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:551
>  {
>      ASSERT(!m_isPaintingSuspended);
>      ASSERT(!m_layerTreeHost || m_layerTreeHost->participatesInDisplay());
> -    ASSERT(!m_webPage->size().isEmpty());
> +
> +    if (m_webPage->size().isEmpty())
> +        return;

We should be bailing out at an earlier point than this.
Comment 13 Brian Weinstein 2011-04-22 11:22:11 PDT
Comment on attachment 90728 [details]
[PATCH] Win fix v3

View in context: https://bugs.webkit.org/attachment.cgi?id=90728&action=review

>> Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:216
>> +    ASSERT(!m_isAttached);
> 
> Sure seems like this code should be in the cross-platform file.

Yeah, especially since detach() is a no-op for platforms that haven't implemented platformDetach(), that is fine.

>> Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:269
>> +        ::ShowWindow(m_inspectorWindow, SW_SHOW);
> 
> I'm not sure the comment is needed here. Not showing the window when we're not visible makes sense even if the "being destroyed" case isn't considered.

Comment removed.

>> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:551
>> +        return;
> 
> We should be bailing out at an earlier point than this.

Okay.
Comment 14 Brian Weinstein 2011-04-22 13:51:12 PDT
Created attachment 90754 [details]
[PATCH] Win Fix v4
Comment 15 Adam Roben (:aroben) 2011-04-24 23:30:01 PDT
Comment on attachment 90754 [details]
[PATCH] Win Fix v4

View in context: https://bugs.webkit.org/attachment.cgi?id=90754&action=review

> Source/WebKit2/ChangeLog:44
> +        (WebKit::WebInspectorFrontendClient::closeWindow): Add a call to the InspectorController to disconnect the inspector frontend
> +            from the inspector backend. Without this line, when we close the attached inspector, it won't re-open, because the
> +            inspector controller still thinks there is a frontend.

Might be worth mentioning that this matches WebKit1. Maybe someday WebCore should do this automatically.

> Source/WebKit2/ChangeLog:48
> +        (WebKit::DrawingAreaImpl::sendUpdateBackingStoreState): Add an early return if the WebPageProxy's viewSize is empty.

It would be good to expand on this comment a little bit to explain why this is a good and OK thing to do.

> Source/WebKit2/UIProcess/WebPageProxy.h:496
> +    PageClient* pageClient() const { return m_pageClient; }
> +

It would be better to add a nativeWindow function that just calls through to the PageClient rather than exposing the client itself. (That would match WebPage::viewSize, for example.)
Comment 16 Brian Weinstein 2011-04-25 10:38:03 PDT
(In reply to comment #15)
> (From update of attachment 90754 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90754&action=review
> 
> > Source/WebKit2/ChangeLog:44
> > +        (WebKit::WebInspectorFrontendClient::closeWindow): Add a call to the InspectorController to disconnect the inspector frontend
> > +            from the inspector backend. Without this line, when we close the attached inspector, it won't re-open, because the
> > +            inspector controller still thinks there is a frontend.
> 
> Might be worth mentioning that this matches WebKit1. Maybe someday WebCore should do this automatically.

Added: This matches WebKit1's behavior, although it seems like this is
            something WebCore should handle.

> 
> > Source/WebKit2/ChangeLog:48
> > +        (WebKit::DrawingAreaImpl::sendUpdateBackingStoreState): Add an early return if the WebPageProxy's viewSize is empty.
> 
> It would be good to expand on this comment a little bit to explain why this is a good and OK thing to do.

Added: This can be needed if the inspector takes up the whole view, and it is useful to prevent a possible synchronous message
            from the UIProcess -> WebProcess to update a backing store that isn't visible in the first place.

> 
> > Source/WebKit2/UIProcess/WebPageProxy.h:496
> > +    PageClient* pageClient() const { return m_pageClient; }
> > +
> 
> It would be better to add a nativeWindow function that just calls through to the PageClient rather than exposing the client itself. (That would match WebPage::viewSize, for example.)

Fixed.
Comment 17 Brian Weinstein 2011-04-25 10:43:04 PDT
Landed Windows part of this in r84785.
Comment 18 Brian Weinstein 2011-04-27 17:48:42 PDT
Created attachment 91392 [details]
[PATCH] Mac Fix v1
Comment 19 Timothy Hatcher 2011-04-28 15:00:48 PDT
Comment on attachment 91392 [details]
[PATCH] Mac Fix v1

View in context: https://bugs.webkit.org/attachment.cgi?id=91392&action=review

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:92
> +- (NSInteger)tag
> +{
> +    return WKInspectorViewTag;
> +}

You don't need to do this as a subclass. You can call setTag: on the normal WKView.
Comment 20 Brian Weinstein 2011-04-28 15:03:45 PDT
Comment on attachment 91392 [details]
[PATCH] Mac Fix v1

View in context: https://bugs.webkit.org/attachment.cgi?id=91392&action=review

>> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:92
>> +}
> 
> You don't need to do this as a subclass. You can call setTag: on the normal WKView.

According to http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/ApplicationKit/Classes/NSView_Class/Reference/NSView.html - you can't call setTag on an NSView - you can set it in Interface Builder, but not in code. It looks like I need to do a subclass. I wish I could use setTag.
Comment 21 Brian Weinstein 2011-04-28 15:34:40 PDT
Mac side of this landed in r85245.
Comment 22 WebKit Review Bot 2011-04-28 15:52:55 PDT
http://trac.webkit.org/changeset/85245 might have broken WinCE Release (Build)