[webkit-reviews] review denied: [Bug 36213] Assertion failure when inspecting a page with workers : [Attachment 51283] 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 51283: Send worker creation/destruction notifications to inspector
frontend asynchronously
https://bugs.webkit.org/attachment.cgi?id=51283&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
Awesome, thanks a lot!

> Index: WebCore/inspector/InjectedScriptHost.h
> -    void willDestroyWorker(long id);
> +    void didDestroyWorker(long id);

These might need to be "long long" now, to be able to roundtrip the intptr_t on
64-bit. "long long" is supported by idl and JSValue.

> Index: WebCore/inspector/InspectorController.cpp

>  #if ENABLE(WORKERS)
> -void InspectorController::didCreateWorker(long id, const String& url, bool
isSharedWorker)
> +class PostWorkerNotificationToFrontendTask : public
ScriptExecutionContext::Task {
> +public:
> +    static PassOwnPtr<PostWorkerNotificationToFrontendTask>
create(RefPtr<InspectorWorkerResource> worker, bool isCreated)

The parameter should be PassRefPtr. (http://webkit.org/coding/RefPtr.html)

Also, normally WebKit style is to create and use an enum rather then bool
parameter - this makes callsite more readable. In this case, there could be
WorkerCreated and WorkerDestroyed values.

> +    PostWorkerNotificationToFrontendTask(RefPtr<InspectorWorkerResource>
worker, bool isCreated)

Also should be PassRefPtr.

> +void
InspectorController::postWorkerNotificationToFrontend(RefPtr<InspectorWorkerRes
ource> worker, bool isCreated)

Here, just raw pointer InspectorWorkerResource* would be just fine since this
function does not take ownership or affects lifetime of the passed resource and
relies on the caller to keep it alive.

> +void InspectorController::didCreateWorker(intptr_t id, const String& url,
bool isSharedWorker)

I would add another note about bool parameters and named enum values but this
is code that already existed before the patch, so no need to change it. :-)

I'd r+ with "fix it on landing" note but it feels there are a few possible
changes so let me r- and look forward for an updated patch!