[Webkit-unassigned] [Bug 187415] [WinCairo] Support display of webinspector ui on non-legacy minibrowser

[bugzilla-daemon at webkit.org]

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>