Summary: | [Win] Remove RunLoopWin and WorkQueueWin | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||
Component: | Web Template Framework | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||
Status: | RESOLVED INVALID | ||||||||||
Severity: | Normal | CC: | achristensen, andersca, Basuke.Suzuki, bfulgham, don.olmstead, ews-watchlist, Hironori.Fujii, mcatanzaro, youennf | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=210785 | ||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2018-06-08 23:00:47 PDT
For WinCairo, we don't have any reason to keep using the current Windows implementation. We still need to test with generic implementation before moving forward, but we agreed with the direction. We need to hear the voice of AppleWin guys. > This patch removes RunLoopWin, and WorkQueueWin. Windows start using RunLoopGeneric and WorkQueueGeneric.
Can you attach you patch?
One issue we found is that very Windows specific feature was added to WorkQueueWin, which are: (WTF::WorkQueue::handleCallback): (WTF::WorkQueue::registerHandle): (WTF::WorkQueue::unregisterAndCloseHandle): and they are used in IPC/ConnectionWin.cpp. I don't see the reason why these features are in WorkQueue because they are not in other port. We can separate those features and move them into IPC/Connection. I'd have to see the patch in order to comment. In general, if nothing breaks we can do it. Created attachment 342536 [details]
Patch
(In reply to Basuke Suzuki from comment #3) > One issue we found is that very Windows specific feature was added to > WorkQueueWin, which are: > (WTF::WorkQueue::handleCallback): > (WTF::WorkQueue::registerHandle): > (WTF::WorkQueue::unregisterAndCloseHandle): > and they are used in IPC/ConnectionWin.cpp. > > I don't see the reason why these features are in WorkQueue because they are > not in other port. We can separate those features and move them into > IPC/Connection. Yes. WorkItemContext is not related to WorkQueue. Since it is only used in IPC::Connection, we can just move this there. Comment on attachment 342536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342536&action=review > Source/WTF/wtf/win/WorkQueueWin.cpp:-231 > - ::QueueUserWorkItem(unregisterWaitAndDestroyItemCallback, workItem.ptr(), WT_EXECUTEDEFAULT); I'm not sure who keeps this WorkItemContext's liveness here. I guess this was a bug. Comment on attachment 342536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342536&action=review > Source/WebKit/Platform/IPC/win/ConnectionWin.cpp:284 > + result = ::RegisterWaitForSingleObject( > + &m_readerWaitHandle, > + m_readState.hEvent, > + [] ((void* data, BOOLEAN timerOrWaitFired) { > + ASSERT_UNUSED(timerOrWaitFired, !timerOrWaitFired); > + // This is safe since we call ::UnregisterWaitEx directly when closing. > + RefPtr<Connection> protectedThis(static_cast<Connection*>(data)); > + protectedThis->m_connectionQueue->dispatch([protectedThis = WTFMove(protectedThis)] { > + protectedThis->readEventHandler(); > + }); > + }, this, INFINITE, WT_EXECUTEDEFAULT)); > + ASSERT(result); > + > + result = ::RegisterWaitForSingleObject( > + &m_writerWaitHandle, > + m_writeState.hEvent, > + [] ((void* data, BOOLEAN timerOrWaitFired) { > + ASSERT_UNUSED(timerOrWaitFired, !timerOrWaitFired); > + // This is safe since we call ::UnregisterWaitEx directly when closing. > + RefPtr<Connection> protectedThis(static_cast<Connection*>(data)); > + protectedThis->m_connectionQueue->dispatch([protectedThis = WTFMove(protectedThis)] { > + protectedThis->writeEventHandler(); > + }); > + }, this, INFINITE, WT_EXECUTEDEFAULT)); > + ASSERT(result); This doesn't match the WebKit coding style guidelines. Comment on attachment 342536 [details]
Patch
Ah, no. Unfortunately, we cannot do this. We have bunch of GetMessage / PostMessage / DispatchMessage / TranslateMessage calls in our tree, and at that time, these messages are implicitly transferred to the pump in the current thread: If RunLoop is working, it receives these message. So I think removing this is a bit difficult.
Comment on attachment 342536 [details] Patch Attachment 342536 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/8148872 Number of test failures exceeded the failure limit. Created attachment 342569 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 342536 [details] Patch Attachment 342536 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/8150164 New failing tests: js/dom/JSON-stringify.html Created attachment 342588 [details]
Archive of layout-test-results from ews117 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
(In reply to Yusuke Suzuki from comment #9) > Comment on attachment 342536 [details] > Patch > > Ah, no. Unfortunately, we cannot do this. We have bunch of GetMessage / > PostMessage / DispatchMessage / TranslateMessage calls in our tree, and at > that time, these messages are implicitly transferred to the pump in the > current thread: If RunLoop is working, it receives these message. So I think > removing this is a bit difficult. That's so bad. But at least we can move Windows only features of WorkQueue into IPC::Connection and make a bug fix with confident. I'll open a new bug for that. |