Bug 181151 – [WTF] Align assumption in RunLoopWin to the other platform's RunLoop

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[WTF] Support feature-rich RunLoop in Windows

[Yusuke Suzuki]

Created attachment 330179 [details]
Patch

[Yusuke Suzuki]

Created attachment 330180 [details]
Patch

[Yusuke Suzuki]

Created attachment 330181 [details]
Patch

[Yusuke Suzuki]

Created attachment 330182 [details]
Patch

[EWS Watchlist]

Comment on attachment 330182 [details]
Patch

Attachment 330182 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5827210

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html
fast/mediastream/MediaStream-MediaElement-setObject-null.html
imported/w3c/web-platform-tests/service-workers/service-worker/ServiceWorkerGlobalScope/update.https.html

[EWS Watchlist]

Created attachment 330185 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Created attachment 331860 [details]
Patch

[EWS Watchlist]

Attachment 331860 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/RunLoop.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 331861 [details]
Patch

[EWS Watchlist]

Attachment 331861 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/RunLoop.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 331866 [details]
Patch

[EWS Watchlist]

Attachment 331866 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/RunLoop.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 331867 [details]
Patch

[EWS Watchlist]

Attachment 331867 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/RunLoop.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 331869 [details]
Patch

[EWS Watchlist]

Attachment 331869 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/RunLoop.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 331870 [details]
Patch

[EWS Watchlist]

Attachment 331870 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/RunLoop.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 331871 [details]
Patch

[EWS Watchlist]

Attachment 331871 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/RunLoop.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 331889 [details]
Patch

[EWS Watchlist]

Attachment 331889 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/RunLoop.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 331894 [details]
Patch

[EWS Watchlist]

Attachment 331894 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/RunLoop.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 331894 [details]
Patch

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

This seems like a good thing and it would allow better cross-platform code, but the Windows bots are red.

> Source/WTF/ChangeLog:3
> +        [WTF] Support feature-rich RunLoop in Windows

This could be a better description.  Exactly what functions are you trying to enable?

[Yusuke Suzuki]

Created attachment 338719 [details]
Patch

[Yusuke Suzuki]

Created attachment 338725 [details]
Patch

[Yusuke Suzuki]

Created attachment 338740 [details]
Patch

[Yusuke Suzuki]

Created attachment 338742 [details]
Patch

[Yusuke Suzuki]

Created attachment 338748 [details]
Patch

[Yusuke Suzuki]

Created attachment 338904 [details]
Patch

[Yusuke Suzuki]

Created attachment 338905 [details]
Patch

[Yusuke Suzuki]

Created attachment 340264 [details]
Patch

[Don Olmstead]

Created attachment 363711 [details]
Redone patch

Here's the previous patch redone on ToT.

Yusuke can you double check that I got everything from your original patch?

[EWS Watchlist]

Comment on attachment 363711 [details]
Redone patch

Attachment 363711 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11388338

New failing tests:
fast/scrolling/scroll-container-horizontally.html

[EWS Watchlist]

Created attachment 363721 [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.10.0-0.325-5-3-x86_64-64bit

[Fujii Hironori]

Comment on attachment 363711 [details]
Redone patch

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

> Source/WTF/wtf/win/RunLoopWin.cpp:80
> +    while (BOOL result = ::PeekMessage(&message, 0, 0, 0, PM_REMOVE)) {

Unused variable 'result'.

> Source/WTF/wtf/win/RunLoopWin.cpp:98
> +    std::call_once(onceKey, [] {

Theoretically, other thread can create a window before RegisterClass is finish. 
Should registerRunLoopMessageWindowClass be called from initializeMainThread?

> Source/WTF/wtf/win/RunLoopWin.cpp:101
> +        windowClass.cbWndExtra      = sizeof(RunLoop*);

cbWndExtra is for dialog box. You don't need to set.

[Fujii Hironori]

Comment on attachment 363711 [details]
Redone patch

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

>> Source/WTF/wtf/win/RunLoopWin.cpp:101
>> +        windowClass.cbWndExtra      = sizeof(RunLoop*);
> 
> cbWndExtra is for dialog box. You don't need to set.

Err. This is needed for GetWindowLongPtr.

[Fujii Hironori]

Comment on attachment 363711 [details]
Redone patch

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

> Source/WTF/wtf/win/RunLoopWin.cpp:139
> +            ::KillTimer(m_runLoop->m_runLoopMessageWindow, bitwise_cast<uintptr_t>(this));

Set m_isActive = false.

[Fujii Hironori]

Created attachment 363987 [details]
Patch

* Addressed my review feedbacks

[EWS Watchlist]

Comment on attachment 363987 [details]
Patch

Attachment 363987 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11424255

New failing tests:
accessibility/mac/selection-notification-focus-change.html

[EWS Watchlist]

Created attachment 363994 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Don Olmstead]

Comment on attachment 363987 [details]
Patch

r=me

Code looks fine. I'm worried about this getting a rollout with the AppleWin test failures. You analyze any of those?

[Fujii Hironori]

Thanks. They all seem false positive.

[Fujii Hironori]

Comment on attachment 363987 [details]
Patch

Clearing flags on attachment: 363987

Committed r242694: <https://trac.webkit.org/changeset/242694>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48756033>