[webkit-reviews] review denied: [Bug 71903] [chromium] Missing wait() in CCThreadProxy::setVisible : [Attachment 114262] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Iain Merrick
<husky at google.com>'s request for review:
Bug 71903: [chromium] Missing wait() in CCThreadProxy::setVisible
https://bugs.webkit.org/show_bug.cgi?id=71903

Attachment 114262: Patch
https://bugs.webkit.org/attachment.cgi?id=114262&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114262&action=review


No chance of a unit test? We have some CCLayerTreeHostTests that bounce between
threads already.

> Source/WebCore/platform/graphics/chromium/cc/CCCompletionEvent.h:56
> +	   ASSERT(!m_waited && (m_waited = true));

side-effects in an ASSERT() is pretty ugly - can you just do the assignment in
an #ifndef NDEBUG block?  this reads like a bug, not something intentional

> Source/WebCore/platform/graphics/chromium/cc/CCCompletionEvent.h:63
> +	   ASSERT(!m_signalled && (m_signalled = true));

same here