[webkit-reviews] review denied: [Bug 36213] Assertion failure when inspecting a page with workers : [Attachment 51421] Send worker creation/destruction notifications to inspector frontend asynchronously

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Andrey Kosyakov
<caseq at chromium.org>'s request for review:
Bug 36213: Assertion failure when inspecting a page with workers
https://bugs.webkit.org/show_bug.cgi?id=36213

Attachment 51421: Send worker creation/destruction notifications to inspector
frontend asynchronously
https://bugs.webkit.org/attachment.cgi?id=51421&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
Looks good! 

One tiny nit:

> +void InspectorController::postWorkerNotificationToFrontend(const
InspectorWorkerResource& worker, InspectorController::WorkerAction action)
> +{
> +    if (!m_frontend)
> +	   return;
> +    switch (action) {
> +    case InspectorController::WorkerCreated:
> +	   m_frontend->didCreateWorker(worker);
> +	   break;
> +    case InspectorController::WorkerDestroyed:
> +	   m_frontend->didDestroyWorker(worker);
> +	   break;
> +    default:
> +	   ASSERT_NOT_REACHED();

WebKit usually does not have this 'default' case since if the value is enum and
there is no 'default', the compiler verifies all values of the enum are used.
If there is desire to check for some integer value outside of enum range, the
'brake' could be replaced by 'return' and the ASSERT_NOT_REACHED() added after
the whole switch. This way, both checks are active.

Also: if you want to use the commit-queue, mark the corresponding flag as '?'
as well, this way the reviewer could see your intention to use the bot and flip
it to + with r+.