Bug 188172

Summary: [WinCairo] <select> elements do not popup options
Product: WebKit Reporter: Stephan Szabo <stephan.szabo>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, Hironori.Fujii, koivisto, lforschler, rniwa, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Stephan Szabo 2018-07-30 10:23:08 PDT
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.
Comment 1 Stephan Szabo 2018-07-30 10:49:35 PDT
Created attachment 346069 [details]
Patch
Comment 2 Fujii Hironori 2018-07-30 19:29:48 PDT
https://github.com/WebKit/webkit/commit/43768040539a01ec2d52954f17fdb39763724623
This is the revision the original implementaion has been removed.
Comment 3 Fujii Hironori 2018-07-30 19:32:39 PDT
Bug 48701 – Windows Popup widget support (<select>)
The original bug ticket.
Comment 4 Fujii Hironori 2018-07-30 19:40:29 PDT
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.
Comment 5 Stephan Szabo 2018-07-31 09:51:23 PDT
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
Comment 7 Stephan Szabo 2018-07-31 13:30:48 PDT
Created attachment 346198 [details]
Patch
Comment 8 WebKit Commit Bot 2018-07-31 18:40:47 PDT
Comment on attachment 346198 [details]
Patch

Clearing flags on attachment: 346198

Committed r234442: <https://trac.webkit.org/changeset/234442>
Comment 9 WebKit Commit Bot 2018-07-31 18:40:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2018-08-01 20:24:25 PDT
<rdar://problem/42840235>