Created attachment 322481 [details] [PATCH] Add WTF files for WinCairo WebKit

Comment on attachment 322481 [details] [PATCH] Add WTF files for WinCairo WebKit View in context: https://bugs.webkit.org/attachment.cgi?id=322481&action=review > Source/WTF/wtf/WorkQueue.h:28 > -#ifndef WorkQueue_h > -#define WorkQueue_h > +#pragma once Unfortunately we need to keep the c-style guard because of inconsistent including in the cocoa ports for now. > Source/WTF/wtf/threads/BinarySemaphore.h:51 > +#if PLATFORM(WIN) > + HANDLE m_event; I think it's a bad idea to have such a platform-dependent difference in design of something like BinarySemaphore. What's wrong with Mutex and ThreadCondition? > Source/WTF/wtf/threads/win/BinarySemaphoreWin.cpp:39 > +BinarySemaphore::~BinarySemaphore() > +{ > + ::CloseHandle(m_event); The Win32 API has quite a bit of overhead for things like this. We have our custom WTF types to make them thin and lightweight. c++11 added many things we can use instead.

Created attachment 322784 [details] [PATCH] Add WTF files for WinCairo WebKit Thank you for your review. (In reply to Alex Christensen from comment #2) > > Source/WTF/wtf/WorkQueue.h:28 > > -#ifndef WorkQueue_h > > -#define WorkQueue_h > > +#pragma once > > Unfortunately we need to keep the c-style guard because of inconsistent including in the cocoa ports for now. Should only be WorkQueue.h considered for cocoa ports to keep c-style guard? I'm interested in which file should use the guard to avoid the inconsistency. > > Source/WTF/wtf/threads/BinarySemaphore.h:51 > > +#if PLATFORM(WIN) > > + HANDLE m_event; > > I think it's a bad idea to have such a platform-dependent difference in > design of something like BinarySemaphore. What's wrong with Mutex and > ThreadCondition? > > > Source/WTF/wtf/threads/win/BinarySemaphoreWin.cpp:39 > > +BinarySemaphore::~BinarySemaphore() > > +{ > > + ::CloseHandle(m_event); > > The Win32 API has quite a bit of overhead for things like this. We have our > custom WTF types to make them thin and lightweight. c++11 added many things > we can use instead. I fixed this point, WTF/wtf/BinarySemaphore.cpp is used in this patch.

Created attachment 323012 [details] [PATCH] Add WTF files for WinCairo WebKit

Attachment 323012 [details] did not pass style-queue: ERROR: Source/WTF/wtf/win/WorkItemWin.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WTF/wtf/win/WorkItemWin.cpp:29: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to Build Bot from comment #5) > Attachment 323012 [details] did not pass style-queue: > > > ERROR: Source/WTF/wtf/win/WorkItemWin.cpp:28: Found other header before a > header this file implements. Should be: config.h, primary header, blank > line, and then alphabetically sorted. [build/include_order] [4] > ERROR: Source/WTF/wtf/win/WorkItemWin.cpp:29: Alphabetical sorting problem. > [build/include_order] [4] > Total errors found: 2 in 6 files It is to avoid a redeclaration error by referring WorkItemWin.h where exists in the forwarding header directory and the WTF directory. The method just mimics "Source/WTF/wtf/CrossThreadCopier.cpp". If this workaround is acceptable, I'd like to file a bug that these errors are false positives.

Comment on attachment 323012 [details] [PATCH] Add WTF files for WinCairo WebKit View in context: https://bugs.webkit.org/attachment.cgi?id=323012&action=review > Source/WTF/wtf/WorkQueue.h:114 > + HashMap<HANDLE, RefPtr<HandleWorkItem> > m_items; >> instead of > > Also, could the value be made a Ref? > Source/WTF/wtf/win/WorkQueueWin.cpp:76 > + unregisterWaitAndDestroyItemSoon(item); Would it be useful to assert that item is non-null? Maybe unregisterWaitAndDestroyItemSoon could take a Ref. > Source/WTF/wtf/win/WorkQueueWin.cpp:256 > + DWORD error = ::GetLastError(); Do we want to return this? > Source/WTF/wtf/win/WorkItemWin.cpp:44 > + return adoptRef(new WorkItemWin(WTFMove(function), queue)); adoptRef(*new > Source/WTF/wtf/win/WorkItemWin.cpp:54 > + , m_waitHandle(0) This should use C++11-style initializer lists in the header. > Source/WTF/wtf/win/WorkItemWin.cpp:61 > + return adoptRef(new HandleWorkItem(handle, WTFMove(function), queue)); This should return a Ref. return adoptRef(*new ...) > Source/WTF/wtf/win/WorkItemWin.h:61 > + void setWaitHandle(HANDLE waitHandle) { m_waitHandle = waitHandle; } Can we not put this waitHandle in the constructor somehow? It's usually a bad idea to make an object and then have to remember to call setters in order to make that object valid. It makes it easy to forget or to have complicated flow paths that makes it hard to tell that you forgot.

Created attachment 323984 [details] [PATCH] Add WTF files for WinCairo WebKit (In reply to Alex Christensen from comment #7) > > Source/WTF/wtf/WorkQueue.h:114 > > + HashMap<HANDLE, RefPtr<HandleWorkItem> > m_items; > > + unregisterWaitAndDestroyItemSoon(item); > > + return adoptRef(new WorkItemWin(WTFMove(function), queue)); > > + return adoptRef(new HandleWorkItem(handle, WTFMove(function), queue)); Fixed these points which used RefPtr. > > Source/WTF/wtf/win/WorkQueueWin.cpp:256 > > + DWORD error = ::GetLastError(); > > Do we want to return this? "error" is unnecessary. Those codes to just get errors are deleted, and those cases are treated as ASSERT cases. > > Source/WTF/wtf/win/WorkItemWin.cpp:54 > > + , m_waitHandle(0) > > This should use C++11-style initializer lists in the header. Fixed. > > Source/WTF/wtf/win/WorkItemWin.h:61 > > + void setWaitHandle(HANDLE waitHandle) { m_waitHandle = waitHandle; } > > Can we not put this waitHandle in the constructor somehow? It's usually a > bad idea to make an object and then have to remember to call setters in > order to make that object valid. It makes it easy to forget or to have > complicated flow paths that makes it hard to tell that you forgot. In this patch, WorkItemContext is added to handle context for tasks to solve the point. It is not a good method but setWaitHandle() was deleted.

Attachment 323984 [details] did not pass style-queue: ERROR: Source/WTF/wtf/win/WorkItemContext.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WTF/wtf/win/WorkItemContext.cpp:29: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 323999 [details] [PATCH] Add WTF files for WinCairo WebKit

Attachment 323999 [details] did not pass style-queue: ERROR: Source/WTF/wtf/win/WorkItemContext.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WTF/wtf/win/WorkItemContext.cpp:29: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to Build Bot from comment #11) > Attachment 323999 [details] did not pass style-queue: > > > ERROR: Source/WTF/wtf/win/WorkItemContext.cpp:28: Found other header before > a header this file implements. Should be: config.h, primary header, blank > line, and then alphabetically sorted. [build/include_order] [4] > ERROR: Source/WTF/wtf/win/WorkItemContext.cpp:29: Alphabetical sorting > problem. [build/include_order] [4] > Total errors found: 2 in 6 files I'm trying to fix this style error.

Created attachment 324021 [details] [PATCH] Add WTF files for WinCairo WebKit

Comment on attachment 324021 [details] [PATCH] Add WTF files for WinCairo WebKit View in context: https://bugs.webkit.org/attachment.cgi?id=324021&action=review > Source/WTF/wtf/win/WorkItemContext.cpp:43 > + ASSERT_ARG(handle, handle); There is a Win32Handle class that might be of use for lifetime of handles.

(In reply to Don Olmstead from comment #14) > Comment on attachment 324021 [details] > [PATCH] Add WTF files for WinCairo WebKit > > View in context: > https://bugs.webkit.org/attachment.cgi?id=324021&action=review > > > Source/WTF/wtf/win/WorkItemContext.cpp:43 > > + ASSERT_ARG(handle, handle); > > There is a Win32Handle class that might be of use for lifetime of handles. Thank you for your advice. I'll try to use the class.

Comment on attachment 324021 [details] [PATCH] Add WTF files for WinCairo WebKit View in context: https://bugs.webkit.org/attachment.cgi?id=324021&action=review > Source/WTF/wtf/win/WorkItemContext.cpp:42 > + // how to delete handles?? Yeah, let's not land code with a comment like this.

Created attachment 325022 [details] [PATCH] Add WTF files for WinCairo WebKit (In reply to Alex Christensen from comment #16) > Comment on attachment 324021 [details] > [PATCH] Add WTF files for WinCairo WebKit > > View in context: > https://bugs.webkit.org/attachment.cgi?id=324021&action=review > > > Source/WTF/wtf/win/WorkItemContext.cpp:42 > > + // how to delete handles?? > > Yeah, let's not land code with a comment like this. The unnecessary comment was deleted and Win32Handle is used to manage windows handles in this patch.

Comment on attachment 325022 [details] [PATCH] Add WTF files for WinCairo WebKit View in context: https://bugs.webkit.org/attachment.cgi?id=325022&action=review > Source/WTF/wtf/win/WorkItemContext.h:49 > + HANDLE* waitHandlePtr() { return &m_waitHandle.m_handle; } This is unnecessary. > Source/WTF/wtf/win/WorkItemContext.h:56 > + Win32Handle m_handle = { }; These default constructors are unnecessary. If we needed them, the = would be unnecessary. > Source/WTF/wtf/win/WorkQueueWin.cpp:57 > + { This scope is unnecessary.

Created attachment 325106 [details] [PATCH] Add WTF files for WinCairo WebKit (In reply to Alex Christensen from comment #18) > Comment on attachment 325022 [details] > [PATCH] Add WTF files for WinCairo WebKit > > View in context: > https://bugs.webkit.org/attachment.cgi?id=325022&action=review > > > Source/WTF/wtf/win/WorkItemContext.h:49 > > + HANDLE* waitHandlePtr() { return &m_waitHandle.m_handle; } > > This is unnecessary. I deleted this line and used a direct reference to the address. > > Source/WTF/wtf/win/WorkItemContext.h:56 > > + Win32Handle m_handle = { }; > > These default constructors are unnecessary. If we needed them, the = would > be unnecessary. > > > Source/WTF/wtf/win/WorkQueueWin.cpp:57 > > + { > > This scope is unnecessary. Those unnecessary lines were deleted.

Created attachment 325142 [details] [PATCH] Add WTF files for WinCairo WebKit (In reply to Yousuke Kimoto from comment #19) > Created attachment 325106 [details] > [PATCH] Add WTF files for WinCairo WebKit This patch is just modified its reviewer part.

Comment on attachment 325142 [details] [PATCH] Add WTF files for WinCairo WebKit Clearing flags on attachment: 325142 Committed r224137: <https://trac.webkit.org/changeset/224137>

All reviewed patches have been landed. Closing bug.

<rdar://problem/35568814>