[webkit-reviews] review granted: [Bug 30612] Remove threadsafe refcounting from tasks used with WTF::MessageQueue. : [Attachment 42245] Proposed patch.

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Dmitry Titov
<dimich at chromium.org>'s request for review:
Bug 30612: Remove threadsafe refcounting from tasks used with
WTF::MessageQueue.
https://bugs.webkit.org/show_bug.cgi?id=30612

Attachment 42245: Proposed patch.
https://bugs.webkit.org/attachment.cgi?id=42245&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Just a few things to fix on landing.

> diff --git a/JavaScriptCore/ChangeLog b/JavaScriptCore/ChangeLog
> +	   * wtf/MessageQueue.h:
> +	   (WTF::MessageQueue::alwaysTruePredicate):
> +	   (WTF::::~MessageQueue):
> +	   (WTF::::append):
> +	   (WTF::::appendAndCheckEmpty):
> +	   (WTF::::prepend):
> +	   (WTF::::waitForMessage):
> +	   (WTF::::waitForMessageFilteredWithTimeout):
> +	   (WTF::::tryGetMessage):
> +	   (WTF::::removeIf):
These functions declarations are messed up. Please fix them.


> diff --git a/JavaScriptCore/wtf/MessageQueue.h
b/JavaScriptCore/wtf/MessageQueue.h
> +    inline void MessageQueue<DataType>::~MessageQueue()
> +    {
> +	   MutexLocker lock(m_mutex);
> +
> +	   while (!m_queue.isEmpty()) {
> +	       DequeConstIterator<DataType*> found = m_queue.begin();
> +	       DataType* ptr = *found;
> +	       m_queue.remove(found);
> +	       delete ptr;
> +	   }
> +    }
Consider using "deleteAllValues" from Deque.h

> +
> +
Extra blank line.


> +    inline PassOwnPtr<DataType>
MessageQueue<DataType>::waitForMessageFilteredWithTimeout(MessageQueueWaitResul
t& result, Predicate& predicate, double absoluteTime)
> +	   DataType* ptr = *found;
ptr? (name) How about message or data.... ?


> +    inline PassOwnPtr<DataType> MessageQueue<DataType>::tryGetMessage()
> +	   DataType* ptr = m_queue.first();
ptr? (name)


> @@ -157,9 +181,11 @@ namespace WTF {
>      inline void MessageQueue<DataType>::removeIf(Predicate& predicate)
> +	       DataType* ptr = *found;

ptr? (name)

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
>  2009-10-30  Dmitry Titov  <dimich at chromium.org>
>  
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Remove threadsafe refcounting from tasks used with
WTF::MessageQueue.
> +	   https://bugs.webkit.org/show_bug.cgi?id=30612
> +
> +	   No new tests since no new functionality. Storage, MessagePorts and
Workers tests cover this.
> +
> +	   There is a lot of files but most changes are simply replace RefPtr
and PassRefPtr to

/There is/There are/
/to/with/


> diff --git a/WebCore/dom/default/PlatformMessagePortChannel.cpp
b/WebCore/dom/default/PlatformMessagePortChannel.cpp
> @@ -193,7 +193,7 @@ void
PlatformMessagePortChannel::postMessageToRemote(PassOwnPtr<MessagePortChann
>  bool
PlatformMessagePortChannel::tryGetMessageFromRemote(OwnPtr<MessagePortChannel::
EventData>& result)
>  {
>      MutexLocker lock(m_mutex);
> +    return (result = m_incomingQueue->tryGetMessage());

It would be nice to not write this on one line, which can easily making it look
like == was intended at first glance.
  result = m_incomingQueue->tryGetMessage();
  return result;


> diff --git a/WebCore/storage/LocalStorageThread.cpp
b/WebCore/storage/LocalStorageThread.cpp
>      while (true) {
> -	   RefPtr<LocalStorageTask> task;
> -	   if (!m_queue.waitForMessage(task))
> +	   OwnPtr<LocalStorageTask> task = m_queue.waitForMessage();
> +	   if (!task)
>	       break;

For similar code in WebCore/storage/DatabaseThread.cpp, you re-wrote this to be

  while (OwnPtr<LocalStorageTask> task = m_queue.waitForMessage()) {

(and if you do that I think the contents will be a single line so no {} around
that.)


> diff --git a/WebCore/workers/WorkerRunLoop.cpp
b/WebCore/workers/WorkerRunLoop.cpp
> +void WorkerRunLoop::Task::performTask(ScriptExecutionContext* context) {
m_task->performTask(context); }

Should be multiple lines.