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

• Component: New Bugs
• Status: RESOLVED FIXED

[Stephan Szabo]

[WinCairo] Support display of webinspector ui on non-legacy minibrowser

[Stephan Szabo]

Created attachment 344461 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Stephan Szabo]

Created attachment 344483 [details]
Initial WebInspector UI bits for non-legacy wincairo

[Fujii Hironori]

https://trac.webkit.org/changeset/139003/
This is the revision that the original implementation has been removed.

[Fujii Hironori]

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.

[Stephan Szabo]

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.

[Stephan Szabo]

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.

[Stephan Szabo]

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.

[Stephan Szabo]

Created attachment 344617 [details]
Initial WebInspector UI bits for non-legacy wincairo - partial feedback response

[Blaze Burg]

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.

[Stephan Szabo]

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.

[Blaze Burg]

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.

[Stephan Szabo]

Created attachment 344705 [details]
Initial WebInspector UI bits for non-legacy wincairo - removed comment

Version of the patch with the comment removed.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42032574>