Bug 143329 – Use constants of sqlite3 directly for status of SQL result in webdatabase

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gyuyoung Kim]

In webdatabase, it has used constants vairables as well as using SQL constants directly. If SQL constants are changed, we should modify many files. Besides it causes to use if~else statement which consumes more cpu cycles compared to switch~case. This patch introduces SQLResultType eunm type, and use it. Additionally if~else statment is changed by switch~case.

[Gyuyoung Kim]

Created attachment 249963 [details]
Patch

[Gyuyoung Kim]

Created attachment 249964 [details]
Patch

[Gyuyoung Kim]

Created attachment 250016 [details]
Patch

[Gyuyoung Kim]

Created attachment 250067 [details]
Patch

[Build Bot]

Comment on attachment 250067 [details]
Patch

Attachment 250067 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4584866775564288

New failing tests:
fast/fixed-layout/fixed-layout.html

[Build Bot]

Created attachment 250069 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Gyuyoung Kim]

(In reply to comment #5)
> Comment on attachment 250067 [details]
> Patch
> 
> Attachment 250067 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/4584866775564288
> 
> New failing tests:
> fast/fixed-layout/fixed-layout.html

Looks unrelated test failure.

[Gyuyoung Kim]

Created attachment 250074 [details]
Patch

[Darin Adler]

Comment on attachment 250074 [details]
Patch

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

I don’t understand why we are defining our own enum. Why not use the SQLite3 constants directly?

> Source/WebCore/platform/sql/SQLiteDatabase.h:48
> +typedef enum {

In C++ preferred syntax is enum X { } rather than typedef enum { } X;

Also, in new code we prefer to use enum class. So this should be:

    enum class SQLResult {
        Done = SQLITE_DONE,
        ...
    }

Then the constants are named SQLResult::Done.

[Gyuyoung Kim]

Created attachment 250113 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #9)
> Comment on attachment 250074 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250074&action=review
> 
> I don’t understand why we are defining our own enum. Why not use the SQLite3
> constants directly?

As I wrote in ChangeLog, it can cause to modify many files if constants of SQLite3 are changed. However If you think to use the constants directly, I don't object to do it. Updated patch uses the constants directly.
 
> > Source/WebCore/platform/sql/SQLiteDatabase.h:48
> > +typedef enum {
> 
> In C++ preferred syntax is enum X { } rather than typedef enum { } X;
> 
> Also, in new code we prefer to use enum class. So this should be:
> 
>     enum class SQLResult {
>         Done = SQLITE_DONE,
>         ...
>     }
> 
> Then the constants are named SQLResult::Done.

If I change name of type, I had to modify more files which have used existing own constants variables. However new patch just uses SQLite3 constatns directly.

[Gyuyoung Kim]

Created attachment 250117 [details]
Patch

[Gyuyoung Kim]

Darin, I wonder what do you think about updated patch.

[WebKit Commit Bot]

Comment on attachment 250117 [details]
Patch

Clearing flags on attachment: 250117

Committed r182365: <http://trac.webkit.org/changeset/182365>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.