[webkit-reviews] review denied: [Bug 38742] [chromium] Implement canEstablishDatabase call for workers. : [Attachment 55815] Patch

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied jochen at chromium.org's request for
review:
Bug 38742: [chromium] Implement canEstablishDatabase call for workers.
https://bugs.webkit.org/show_bug.cgi?id=38742

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

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
Looks great, would r+ with "change on landing" but since cq is needed, we need
a perfect patch.

One small thing:

WebKit/chromium/src/WebWorkerBase.cpp:217
 +	// The controller might be 0 when the worker context is in the process
of shutting down.
I see the discussion in the comments about this check. It is not really needed.
There is another method which returns a V8 proxy
(WorkerScriptController::proxy()) which indeed returns 0 when the termination
of JS in the worker is requested. The
WorkerScriptController::controllerForContext() always returns a non-null
controller in worker context.
See http://trac.webkit.org/changeset/56580. That change split the old function
that could return 0 if either the context is not WorkerContext or the
termination was requested, to be able to make these checks separately. This is
why I think there is no real case when we need an "if(!controller)" here and
rather could just have an assert, or even better no check at all since the very
next statement is going to crash reliably anyways.