[Webkit-unassigned] [Bug 187415] [WinCairo] Support display of webinspector ui on non-legacy minibrowser
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jul 9 02:44:10 PDT 2018
https://bugs.webkit.org/show_bug.cgi?id=187415
--- Comment #8 from Fujii Hironori <Hironori.Fujii at sony.com> ---
Comment on attachment 344483
--> https://bugs.webkit.org/attachment.cgi?id=344483
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.
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180709/b8d41015/attachment.html>
More information about the webkit-unassigned
mailing list