Bug 108724 – webdatabase: Replace ExceptionCode with DatabaseError in the openDatabase() code path

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mark Lam]

This is a sub-task of https://bugs.webkit.org/show_bug.cgi?id=107475.  Breaking out as a step for easier review.  Replacing ExceptionCode with DatabaseError because we may want to have other database specific errors later that don't make sense to propagate out of the webdatabase module.  Also cleaned up DatabaseBackend::performOpenAndVerify() so that it's a little more straightforward and less verbose.

[Mark Lam]

Created attachment 186186 [details]
The patch.

[Mark Lam]

Comment on attachment 186186 [details]
The patch.

Diff looks good.  May I have a review please?  Thanks.

[Alexey Proskuryakov]

Comment on attachment 186186 [details]
The patch.

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.

[Mark Lam]

Comment on attachment 186186 [details]
The patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=186186&action=review

>> Source/WebCore/Modules/webdatabase/DOMWindowWebDatabase.cpp:57
>>          ec = SECURITY_ERR;
> 
> This code looks not very future-proof, as it's unclear what's expected if new errors are added.

Added an assertion that error should be either None or CannotOpenDatabase.  The assertion is placed before the if(error) statement.

>> 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.

Changed to setOpenSucceeded().

>> Source/WebCore/Modules/webdatabase/DatabaseBackend.cpp:421
>> +    error = DatabaseError::None; // Clear the presumed error from above.
> 
> Can these lines be swapped?

Swapped.

>> Source/WebCore/Modules/webdatabase/WorkerContextWebDatabase.cpp:-49
>> -    }
> 
> Same comment about being future proof as above.

Ditto on the adding an appropriate assertion.

>> Source/WebCore/Modules/webdatabase/WorkerContextWebDatabase.cpp:-60
>> -    }
> 
> Ditto.

Ditto on the adding an appropriate assertion.

[Mark Lam]

Landed in r141668: <http://trac.webkit.org/changeset/141668>.