[webkit-reviews] review denied: [Bug 176894] [WinCairo] Add WTF files for wincairo webkit : [Attachment 323012] [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 323012: [PATCH] Add WTF files for WinCairo WebKit

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




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