Bug 59134 – WebKit2: Support docked mode for Web Inspector

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

[Brian Weinstein]

We need to support the docked mode for the Web Inspector in WebKit2.

<rdar://problem/8739005>

[Brian Weinstein]

Created attachment 90590 [details]
WIP Patch

[Adam Roben (:aroben)]

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.

[Brian Weinstein]

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.

[Brian Weinstein]

Created attachment 90616 [details]
[PATCH] Win fix

[WebKit Review Bot]

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.

[Jeff Miller]

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

[Brian Weinstein]

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

[Brian Weinstein]

Created attachment 90664 [details]
[PATCH] Win fix v2

[Adam Roben (:aroben)]

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?

[Brian Weinstein]

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

[Brian Weinstein]

Created attachment 90728 [details]
[PATCH] Win fix v3

[Adam Roben (:aroben)]

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.

[Brian Weinstein]

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.

[Brian Weinstein]

Created attachment 90754 [details]
[PATCH] Win Fix v4

[Adam Roben (:aroben)]

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

[Brian Weinstein]

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

[Brian Weinstein]

Landed Windows part of this in r84785.

[Brian Weinstein]

Created attachment 91392 [details]
[PATCH] Mac Fix v1

[Timothy Hatcher]

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.

[Brian Weinstein]

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.

[Brian Weinstein]

Mac side of this landed in r85245.

[WebKit Review Bot]

http://trac.webkit.org/changeset/85245 might have broken WinCE Release (Build)