Summary: | WebKit2: Support docked mode for Web Inspector | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brian Weinstein <bweinstein> | ||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Brian Weinstein <bweinstein> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, apavlov, bweinstein, eric, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Brian Weinstein
2011-04-21 13:41:54 PDT
Created attachment 90590 [details]
WIP Patch
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 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. Created attachment 90616 [details]
[PATCH] Win fix
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 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(). (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. Created attachment 90664 [details]
[PATCH] Win fix v2
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? (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. Created attachment 90728 [details]
[PATCH] Win fix v3
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 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. Created attachment 90754 [details]
[PATCH] Win Fix v4
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.) (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. Landed Windows part of this in r84785. Created attachment 91392 [details]
[PATCH] Mac Fix v1
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 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. Mac side of this landed in r85245. http://trac.webkit.org/changeset/85245 might have broken WinCE Release (Build) |