[webkit-reviews] review denied: [Bug 188172] [WinCairo] <select> elements do not popup options : [Attachment 346069] Patch

[bugzilla-daemon at webkit.org]

Fujii Hironori <Hironori.Fujii at sony.com> has denied Stephan Szabo
<stephan.szabo at sony.com>'s request for review:
Bug 188172: [WinCairo] <select> elements do not popup options
https://bugs.webkit.org/show_bug.cgi?id=188172

Attachment 346069: Patch

https://bugs.webkit.org/attachment.cgi?id=346069&action=review




--- Comment #4 from Fujii Hironori <Hironori.Fujii at sony.com> ---
Comment on attachment 346069
  --> https://bugs.webkit.org/attachment.cgi?id=346069
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346069&action=review

> Source/WebKit/ChangeLog:21
> +	   removed wincairo implementation plus some fixes/api changes

Not "wincairo implementation" IIRC, but Windows implementation or AppleWin
implementation.

> Source/WebKit/ChangeLog:65
> +	   removed wincairo implementation plus some fixes/api changes

Ditto.

> Source/WebKit/Shared/PlatformPopupMenuData.cpp:40
> +    , m_itemHeight(0)

WebKit prefers member initializer.

> Source/WebKit/Shared/PlatformPopupMenuData.h:58
> +    int m_itemHeight;

I think WebKit prefers member initializer.
int m_clientPaddingLeft { 0 };
int m_clientPaddingRight { 0 };
int m_clientInsetLeft { 0 };
int m_clientInsetRight { 0 };
int m_popupWidth { 0 };
int m_itemHeight { 0 };

> Source/WebKit/UIProcess/win/WebPopupMenuProxyWin.cpp:36
> +#include <WebCore/BString.h>

Is BString used?

> Source/WebKit/UIProcess/win/WebPopupMenuProxyWin.cpp:47
> +using namespace std;

Don't "using namespace std;"
https://webkit.org/code-style-guidelines/#using-in-cpp

> Source/WebKit/UIProcess/win/WebPopupMenuProxyWin.cpp:221
> +	   shouldAnimate = FALSE;

You added this line. But, It seems for me that this line not needed. Does
SystemParametersInfo change shouldAnimate if it fails?

> Source/WebKit/UIProcess/win/WebPopupMenuProxyWin.h:38
> +typedef struct HBITMAP__* HBITMAP;

<windows.h> is included in WebCorePrefix.h. I think you can remove these
typedefs.