[webkit-reviews] review denied: [Bug 176894] [WinCairo] Add WTF files for wincairo webkit : [Attachment 322481] [PATCH] Add WTF files for WinCairo WebKit

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Yousuke Kimoto
<Yousuke.Kimoto at sony.com>'s request for review:
Bug 176894: [WinCairo] Add WTF files for wincairo webkit
https://bugs.webkit.org/show_bug.cgi?id=176894

Attachment 322481: [PATCH] Add WTF files for WinCairo WebKit

https://bugs.webkit.org/attachment.cgi?id=322481&action=review




--- Comment #2 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 322481
  --> https://bugs.webkit.org/attachment.cgi?id=322481
[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.