[webkit-reviews] review denied: [Bug 34992] Add async bindings for Worker access to DB : [Attachment 51662] Patch

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Eric U. <ericu at chromium.org>'s
request for review:
Bug 34992: Add async bindings for Worker access to DB
https://bugs.webkit.org/show_bug.cgi?id=34992

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

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
I'm not exactly JCS expert but since it collects dust I'm going to try...

> Index: WebCore/ChangeLog
> +	   Add async bindings for Worker access to DB

I'd say "Add bindings for async DB API in Workers". The bindings themselves are
not async.

> Index: WebCore/bindings/js/JSWorkerContextCustom.cpp
> +JSValue JSWorkerContext::openDatabase(ExecState* exec, const ArgList& args) 

> +{ 
> +    ExceptionCode ec = 0; 

I understand this is basically a copy from JSDOMWindowCustom, but I'd move
ExceptionCode declaration close to the line that actually uses it.

> Index: LayoutTests/ChangeLog
> +	   * storage/execute-sql-args.js: This is the extracted shared core of
the test.
> +	   (throwOnToStringObject.toString):
> +	   (var):
> +	   ():

It might be better to just remove the lines describing actual changes inside
test files, unless you want to add some additional info.

> Index: LayoutTests/storage/change-version-handle-reuse.html
>  {
>      if (window.layoutTestController) {
> +	   layoutTestController.clearAllDatabases();

It seems existing db tests are gaining the new call to clearAllDatabases()
before start. It may change slightly what they test, at first look it makes
them run in a more 'sterile' environment. I wonder if it is necessary. 

> Index: LayoutTests/storage/change-version-handle-reuse.js
> +function runTest()
> +{
> +    try {
> +	   db = openDatabase("ChangeVersion", "", "Test that changing a
database version doesn't kill our handle to it", 1);

What is going to happen when multi-process test harness runs those tests? One
process could run regular test, while another will run worker test - both of
which will access the same database. Should we use different names for
databases in workers?

A general note on the tests to consider: As I understand the new worker tests
can be located either in LayoutTests/Storage or in
LayoutTests/fast/workers/storage (and refer back for .js files). It might be
slightly better to move the worker test files to the fast/workers/storage
simply because workers tests are skipped already on many platforms that do not
enable workers, and at least one port (chromium) has completely different
handling for workers tests for now so it'd be more convenient to add new tests
in the directories that already are treated in a special way.