[webkit-reviews] review granted: [Bug 108724] webdatabase: Replace ExceptionCode with DatabaseError in the openDatabase() code path : [Attachment 186186] The patch.

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 108724: webdatabase: Replace ExceptionCode with DatabaseError in the
openDatabase() code path
https://bugs.webkit.org/show_bug.cgi?id=108724

Attachment 186186: The patch.
https://bugs.webkit.org/attachment.cgi?id=186186&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=186186&action=review


> Source/WebCore/Modules/webdatabase/DOMWindowWebDatabase.cpp:57
> +    if (error == DatabaseError::CannotOpenDatabase)
> +	   ec = INVALID_STATE_ERR;
>      if (!database && !ec)
>	   ec = SECURITY_ERR;

This code looks not very future-proof, as it's unclear what's expected if new
errors are added.

> Source/WebCore/Modules/webdatabase/DatabaseBackend.cpp:297
> +    void openSucceeded() { m_openSucceeded = true; }

This name sounds a little like a getter, not a setter. It should probably have
a verb.

> Source/WebCore/Modules/webdatabase/DatabaseBackend.cpp:421
> +    onExitCaller.openSucceeded();
> +    error = DatabaseError::None; // Clear the presumed error from above.

Can these lines be swapped?

> Source/WebCore/Modules/webdatabase/WorkerContextWebDatabase.cpp:-49
> +    if (error == DatabaseError::CannotOpenDatabase)
> +	   ec = INVALID_STATE_ERR;
> +    if (!database && !ec)
>	   ec = SECURITY_ERR;
> -	   return 0;
> -    }

Same comment about being future proof as above.

> Source/WebCore/Modules/webdatabase/WorkerContextWebDatabase.cpp:-60
> +    if (error == DatabaseError::CannotOpenDatabase)
> +	   ec = INVALID_STATE_ERR;
> +    if (!database && !ec)
>	   ec = SECURITY_ERR;
> -	   return 0;
> -    }

Ditto.