Bug 176894 - [WinCairo] Add WTF files for wincairo webkit
Summary: [WinCairo] Add WTF files for wincairo webkit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 174003
  Show dependency treegraph
 
Reported: 2017-09-14 01:11 PDT by Yousuke Kimoto
Modified: 2017-11-15 13:06 PST (History)
6 users (show)

See Also:


Attachments
[PATCH] Add WTF files for WinCairo WebKit (19.71 KB, patch)
2017-10-02 18:16 PDT, Yousuke Kimoto
achristensen: review-
Details | Formatted Diff | Diff
[PATCH] Add WTF files for WinCairo WebKit (14.85 KB, patch)
2017-10-04 22:29 PDT, Yousuke Kimoto
no flags Details | Formatted Diff | Diff
[PATCH] Add WTF files for WinCairo WebKit (14.86 KB, patch)
2017-10-06 06:09 PDT, Yousuke Kimoto
achristensen: review-
Details | Formatted Diff | Diff
[PATCH] Add WTF files for WinCairo WebKit (14.29 KB, patch)
2017-10-16 20:45 PDT, Yousuke Kimoto
no flags Details | Formatted Diff | Diff
[PATCH] Add WTF files for WinCairo WebKit (14.29 KB, patch)
2017-10-16 23:51 PDT, Yousuke Kimoto
no flags Details | Formatted Diff | Diff
[PATCH] Add WTF files for WinCairo WebKit (15.17 KB, patch)
2017-10-17 10:07 PDT, Yousuke Kimoto
achristensen: review-
Details | Formatted Diff | Diff
[PATCH] Add WTF files for WinCairo WebKit (15.63 KB, patch)
2017-10-26 10:03 PDT, Yousuke Kimoto
achristensen: review+
Details | Formatted Diff | Diff
[PATCH] Add WTF files for WinCairo WebKit (15.45 KB, patch)
2017-10-26 20:03 PDT, Yousuke Kimoto
no flags Details | Formatted Diff | Diff
[PATCH] Add WTF files for WinCairo WebKit (15.45 KB, patch)
2017-10-27 01:53 PDT, Yousuke Kimoto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yousuke Kimoto 2017-09-14 01:11:35 PDT
Add WTF files for wincairo webkit.
Comment 1 Yousuke Kimoto 2017-10-02 18:16:28 PDT
Created attachment 322481 [details]
[PATCH] Add WTF files for WinCairo WebKit
Comment 2 Alex Christensen 2017-10-02 21:18:10 PDT
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.
Comment 3 Yousuke Kimoto 2017-10-04 22:29:35 PDT
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.
Comment 4 Yousuke Kimoto 2017-10-06 06:09:13 PDT
Created attachment 323012 [details]
[PATCH] Add WTF files for WinCairo WebKit
Comment 5 Build Bot 2017-10-06 06:12:15 PDT
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.
Comment 6 Yousuke Kimoto 2017-10-06 06:59:47 PDT
(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 7 Alex Christensen 2017-10-06 10:52:56 PDT
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.
Comment 8 Yousuke Kimoto 2017-10-16 20:45:31 PDT
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.
Comment 9 Build Bot 2017-10-16 20:48:10 PDT
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.
Comment 10 Yousuke Kimoto 2017-10-16 23:51:42 PDT
Created attachment 323999 [details]
[PATCH] Add WTF files for WinCairo WebKit
Comment 11 Build Bot 2017-10-16 23:53:57 PDT
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.
Comment 12 Yousuke Kimoto 2017-10-17 03:33:35 PDT
(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.
Comment 13 Yousuke Kimoto 2017-10-17 10:07:06 PDT
Created attachment 324021 [details]
[PATCH] Add WTF files for WinCairo WebKit
Comment 14 Don Olmstead 2017-10-24 16:34:23 PDT
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.
Comment 15 Yousuke Kimoto 2017-10-24 20:01:59 PDT
(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 16 Alex Christensen 2017-10-25 13:48:58 PDT
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.
Comment 17 Yousuke Kimoto 2017-10-26 10:03:12 PDT
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 18 Alex Christensen 2017-10-26 12:06:03 PDT
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.
Comment 19 Yousuke Kimoto 2017-10-26 20:03:45 PDT
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.
Comment 20 Yousuke Kimoto 2017-10-27 01:53:36 PDT
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 21 WebKit Commit Bot 2017-10-27 17:09:50 PDT
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>
Comment 22 WebKit Commit Bot 2017-10-27 17:09:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Radar WebKit Bug Importer 2017-11-15 13:06:49 PST
<rdar://problem/35568814>