Bug 188172 – [WinCairo] <select> elements do not popup options

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Stephan Szabo]

On WinCairo (non-legacy) <select> elements do not pop up the list of options. Selecting the element multiple times in a row can cause assertion failures because the menu is not up.

[Stephan Szabo]

Created attachment 346069 [details]
Patch

[Fujii Hironori]

https://github.com/WebKit/webkit/commit/43768040539a01ec2d52954f17fdb39763724623
This is the revision the original implementaion has been removed.

[Fujii Hironori]

Bug 48701 – Windows Popup widget support (<select>)
The original bug ticket.

[Fujii Hironori]

Comment on attachment 346069 [details]
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.

[Stephan Szabo]

Comment on attachment 346069 [details]
Patch

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

>> 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?

This was added in the similar place in the legacy path after the non-legacy code was removed.
"[Win] Resolve various static analyzer warnings in WebCore."
https://github.sie.sony.com/WebCore/neko/commit/ca06693684d7a9bf69d7db2deb1c21ca823d6384#diff-d88e6bcdaacad433c28da98715f946d3

[Stephan Szabo]

Sorry, public ref is:
https://github.com/WebKit/webkit/commit/ca06693684d7a9bf69d7db2deb1c21ca823d6384#diff-d88e6bcdaacad433c28da98715f946d3

[Stephan Szabo]

Created attachment 346198 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 346198 [details]
Patch

Clearing flags on attachment: 346198

Committed r234442: <https://trac.webkit.org/changeset/234442>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42840235>