[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

Brian Burg <bburg at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bburg at apple.com

--- Comment #13 from Brian Burg <bburg at apple.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

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.

-- 
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/f2c983f7/attachment.html>