[WinCairo] Support display of webinspector ui on non-legacy minibrowser
Created attachment 344461 [details] Patch
Comment on attachment 344461 [details] Patch Attachment 344461 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8460693 New failing tests: media/media-fullscreen-return-to-inline.html
Created attachment 344471 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 344461 [details] Patch Attachment 344461 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8460640 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
Created attachment 344475 [details] Archive of layout-test-results from ews204 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 344483 [details] Initial WebInspector UI bits for non-legacy wincairo
https://trac.webkit.org/changeset/139003/ This is the revision that the original implementation has been removed.
Comment on attachment 344483 [details] Initial WebInspector UI bits for non-legacy wincairo View in context: https://bugs.webkit.org/attachment.cgi?id=344483&action=review > Source/WebKit/PlatformWin.cmake:53 > + UIProcess/win/WebInspectorMessageListener.cpp The original implementaion had handled WM_WINDOWPOSCHANGING in WebInspectorProxy. Why do you want to add a new file? > Source/WebKit/UIProcess/WebInspectorProxy.h:51 > +#include "win/WebInspectorMessageListener.h" This should be "WebInspectorMessageListener.h". Append the include path. > Source/WebKit/UIProcess/WebInspectorProxy.h:220 > + static LRESULT CALLBACK WndProc(HWND, UINT, WPARAM, LPARAM); Shoul be lower case 'wndProc'. > Source/WebKit/UIProcess/WebInspectorProxy.h:221 > + ATOM registerWindowClass(); The original implementaion had returned bool. Why do you want to return ATOM? > Source/WebKit/UIProcess/WebInspectorProxy.h:261 > + RetainPtr<WebKit::WebView> m_inspectorView; I think this should be RefPtr<WebView>. > Source/WebKit/UIProcess/win/WebInspectorMessageListener.cpp:31 > +namespace WebKit { I think you should put a blank line under "namespace WebKit" line. > Source/WebKit/UIProcess/win/WebInspectorMessageListener.cpp:44 > + auto windowPos = reinterpret_cast<WINDOWPOS*>(lParam); This function can be simplified by using *reinterpret_cast<WINDOWPOS*>(lParam). > Source/WebKit/UIProcess/win/WebInspectorMessageListener.cpp:59 > + unsigned inspectorHeight = inspectorRect.bottom - inspectorRect.top; The original implementaion had used InspectorFrontendClientLocal::constrainedAttachedWindowHeight. Do you need to do it? > Source/WebKit/UIProcess/win/WebInspectorMessageListener.h:39 > + virtual void windowReceivedMessage(HWND, UINT, WPARAM, LPARAM); Put 'override'. > Source/WebKit/UIProcess/win/WebInspectorMessageListener.h:40 > +private: I think you should put a blank line above private:. > Source/WebKit/UIProcess/win/WebInspectorProxyWin.cpp:44 > +#include <WebKit/WebKit2_C.h> I think <WebKit/WebKit2_C.h> is only for client. > Source/WebKit/UIProcess/win/WebInspectorProxyWin.cpp:48 > +static RECT getInitialInspectorRect() The original implementaion had used WebInspectorProxy::initialWindowWidth and initialWindowHeight. > Source/WebKit/UIProcess/win/WebInspectorProxyWin.cpp:50 > + return RECT { 0, 0, 1200, 600 }; You don't need RECT. return { 0, 0, 1200, 600 } > Source/WebKit/UIProcess/win/WebInspectorProxyWin.cpp:53 > +static const LPCTSTR WebInspectorProxyPointerProp = L"WebInspectorProxyPointer"; LPCTSTR -> LPCWSTR because you use L"". > Source/WebKit/UIProcess/win/WebInspectorProxyWin.cpp:73 > + return InspectedWindowInfo { rect.left, rect.top, rect.right - rect.left, rect.bottom - rect.top, parentRect.right - parentRect.left, parentRect.bottom - parentRect.top }; You don't need "InspectedWindowInfo". "return { rect.left, rect.top, rect.right - rect.left, rect.bottom - rect.top, parentRect.right - parentRect.left, parentRect.bottom - parentRect.top }" > Source/WebKit/UIProcess/win/WebInspectorProxyWin.cpp:81 > + case WM_SIZE: I think WebKit prefer '{' on case line. For example, https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/JSONValues.cpp?rev=225699#L355 > Source/WebKit/UIProcess/win/WebInspectorProxyWin.cpp:108 > + wcex.hInstance = 0; The original implementaion had use WebCore::instanceHandle(). > Source/WebKit/UIProcess/win/WebInspectorProxyWin.cpp:115 > + haveRegisteredWindowClass = true; The original implementaion had "haveRegisteredWindowClass = true" immediately after the check. I prefer that because relevant code is placed close. > Source/WebKit/UIProcess/win/WebInspectorProxyWin.cpp:165 > + }); Do you need this new line? I know you seemed to copy GTK port's code. > Source/WebKit/UIProcess/win/WebInspectorProxyWin.cpp:233 > + RetainPtr<CFURLRef> htmlURLRef = adoptCF(CFBundleCopyResourceURL(WebCore::webKitBundle(), CFSTR("Main"), CFSTR("html"), CFSTR("WebInspectorUI"))); Don is planning to remove CFLite dependency within WinCairo. > Source/WebKit/UIProcess/win/WebInspectorProxyWin.cpp:278 > + m_messageListener.reset(new WebInspectorMessageListener(m_inspectedViewWindow, m_inspectorViewWindow)); Use std::make_unique instead of new. > Source/WebKit/UIProcess/win/WebInspectorProxyWin.cpp:305 > + m_inspectorDetachWindow = ::CreateWindowEx(0, WebInspectorProxyClassName, 0, WS_OVERLAPPEDWINDOW, The original implementaion had use WebCore::instanceHandle(). > Source/WebKit/UIProcess/win/WebInspectorProxyWin.cpp:308 > + ::SetProp(m_inspectorDetachWindow, WebInspectorProxyPointerProp, reinterpret_cast<HANDLE>(this)); According the spec of SetProp, RemoveProp need to be called. https://msdn.microsoft.com/library/windows/desktop/ms633568(v=vs.85).aspx > Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:55 > + char* utf8Buffer = new char[utf8Length + 1]; This has been fixed in Bug 187167. > Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:90 > + // Set developer extras enabled on the main browser window! Why do you need this even though you does "preferences->setDeveloperExtrasEnabled(true)" for the inspector's WebView.
Comment on attachment 344483 [details] Initial WebInspector UI bits for non-legacy wincairo View in context: https://bugs.webkit.org/attachment.cgi?id=344483&action=review >> Source/WebKit/PlatformWin.cmake:53 >> + UIProcess/win/WebInspectorMessageListener.cpp > > The original implementaion had handled WM_WINDOWPOSCHANGING in WebInspectorProxy. > Why do you want to add a new file? I'd thought it might be harder to sell a platform specific inheritance change in WebInspectorProxy for Windows than to put it in its own class and using a forward declaration. Looking at https://trac.webkit.org/browser/webkit/trunk/Source/WebKit2/UIProcess/WebInspectorProxy.h?rev=134346 that seems to have been how it was done previously, so I'll move to that. >> Source/WebKit/UIProcess/WebInspectorProxy.h:221 >> + ATOM registerWindowClass(); > > The original implementaion had returned bool. Why do you want to return ATOM? I was just matching it to the actual return type for RegisterClassEx, but since we really only care about did it work or not, moving to bool should be okay.
Comment on attachment 344483 [details] Initial WebInspector UI bits for non-legacy wincairo View in context: https://bugs.webkit.org/attachment.cgi?id=344483&action=review >> Source/WebKit/UIProcess/win/WebInspectorMessageListener.cpp:44 >> + auto windowPos = reinterpret_cast<WINDOWPOS*>(lParam); > > This function can be simplified by using *reinterpret_cast<WINDOWPOS*>(lParam). I'm not sure what you mean here. We're actually modifying the WINDOWPOS pointed to by lParam in order to change the resulting size, so I don't think that using *reinterpret_cast<WINDOWPOS*>(lParam) will do what we want.
Comment on attachment 344483 [details] Initial WebInspector UI bits for non-legacy wincairo View in context: https://bugs.webkit.org/attachment.cgi?id=344483&action=review >> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:90 >> + // Set developer extras enabled on the main browser window! > > Why do you need this even though you does "preferences->setDeveloperExtrasEnabled(true)" for the inspector's WebView. What I'd been running into was that WebInspectorProxy sends Messages::WebInspector::Show() for the inspected page. Which will then do m_page->corePage()->inspectorController().show(), but InspectorController's show aborts early if !enabled() which seems to be based on m_page.settings().developerExtrasEnabled(). Also, the same InspectorController::enabled for the "inspected" page seems to be used to determine whether to allow "Inspect Element" in the context menu by ContextMenuController::showContextMenu, so without the controller being enabled, the menu entry seems to not be added.
Created attachment 344617 [details] Initial WebInspector UI bits for non-legacy wincairo - partial feedback response
Comment on attachment 344483 [details] Initial WebInspector UI bits for non-legacy wincairo View in context: https://bugs.webkit.org/attachment.cgi?id=344483&action=review Overall the patch looks good to me, but please address my and Fujii's comments and post a new patch for EWS. >> Source/WebKit/UIProcess/WebInspectorProxy.h:261 >> + RetainPtr<WebKit::WebView> m_inspectorView; > > I think this should be RefPtr<WebView>. Indeed, RetainPtr has no meaning outside of Cocoa/CoreFoundation code. > Source/WebKit/UIProcess/win/WebInspectorMessageListener.cpp:43 > + if (m_isAttached) { Please early exit in the case of !m_isAttached so we can outdent most of this block. > Source/WebKit/UIProcess/win/WebInspectorMessageListener.cpp:58 > + { Please put the opening brace on the same line as the 'case', and outdent by one level the contents of the switch case. > Source/WebKit/UIProcess/win/WebInspectorMessageListener.cpp:66 > + { Ditto > Source/WebKit/UIProcess/win/WebInspectorMessageListener.h:43 > + bool m_isAttached = false; Nit: use syntax { false } > Source/WebKit/UIProcess/win/WebInspectorMessageListener.h:44 > + AttachmentSide m_currentAttachment; This should have an initial value.
Created attachment 344627 [details] Initial WebInspector UI bits for non-legacy wincairo - feedback response I think this covers the main points besides those I responded to. A few issues that were in the message listener went away by merging in the windowReceivedMessage into the proxy instead of having a separate class so the code in question disappeared.
Comment on attachment 344627 [details] Initial WebInspector UI bits for non-legacy wincairo - feedback response View in context: https://bugs.webkit.org/attachment.cgi?id=344627&action=review r=me, please wait for EWS. > Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:90 > + // Set developer extras enabled on the main browser window! Nit: this comment is unnecessary.
Created attachment 344705 [details] Initial WebInspector UI bits for non-legacy wincairo - removed comment Version of the patch with the comment removed.
Comment on attachment 344705 [details] Initial WebInspector UI bits for non-legacy wincairo - removed comment Clearing flags on attachment: 344705 Committed r233691: <https://trac.webkit.org/changeset/233691>
All reviewed patches have been landed. Closing bug.
<rdar://problem/42032574>