Bug 176894 – [WinCairo] Add WTF files for wincairo webkit

• Component: WebKit2
• Status: RESOLVED FIXED

[Yousuke Kimoto]

Add WTF files for wincairo webkit.

[Yousuke Kimoto]

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

[Alex Christensen]

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.

[Yousuke Kimoto]

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.

[Yousuke Kimoto]

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

[Build Bot]

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.

[Yousuke Kimoto]

(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.

[Alex Christensen]

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.

[Yousuke Kimoto]

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.

[Build Bot]

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.

[Yousuke Kimoto]

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

[Build Bot]

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.

[Yousuke Kimoto]

(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.

[Yousuke Kimoto]

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

[Don Olmstead]

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.

[Yousuke Kimoto]

(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.

[Alex Christensen]

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.

[Yousuke Kimoto]

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.

[Alex Christensen]

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.

[Yousuke Kimoto]

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.

[Yousuke Kimoto]

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.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35568814>