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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 16 20:45:31 PDT 2017


Yousuke Kimoto <Yousuke.Kimoto at sony.com> has asked  for review:
Bug 176894: [WinCairo] Add WTF files for wincairo webkit
https://bugs.webkit.org/show_bug.cgi?id=176894

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

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




--- Comment #8 from Yousuke Kimoto <Yousuke.Kimoto at sony.com> ---
Created attachment 323984

  --> https://bugs.webkit.org/attachment.cgi?id=323984&action=review

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


More information about the webkit-reviews mailing list