Bug 108279 - webdatabase: Introducing WTF::TypeSafeEnum and DatabaseError
Summary: webdatabase: Introducing WTF::TypeSafeEnum and DatabaseError
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks: 107475
  Show dependency treegraph
 
Reported: 2013-01-29 18:33 PST by Mark Lam
Modified: 2013-01-29 21:24 PST (History)
10 users (show)

See Also:


Attachments
the patch. (16.99 KB, patch)
2013-01-29 20:46 PST, Mark Lam
no flags Details | Formatted Diff | Diff
Patch (7.58 KB, patch)
2013-01-29 21:22 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2013-01-29 18:33:19 PST
This is a sub-task of https://bugs.webkit.org/show_bug.cgi?id=107475.  Breaking out as a step for easier review.  This patch introduces the WTF TypeSafeEnum template and the DatabaseError enum list which instantiates an instance of TypeSafeEnum.  The enums in DatabaseError is currently unused, but will be put to use later in bug 107475.
Comment 1 Mark Lam 2013-01-29 20:46:22 PST
Created attachment 185386 [details]
the patch.
Comment 2 Geoffrey Garen 2013-01-29 20:50:29 PST
Comment on attachment 185386 [details]
the patch.

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

r=me

> Source/WTF/wtf/TypeSafeEnum.h:51
> +//     MyEnum value1; // value1 is assigned MyEnum::VALUE_DEFAULT by default.

Typo: MyEnumDef.

> Source/WebCore/Modules/webdatabase/DatabaseError.h:35
> +struct DatabaseErrorDef {

We don't usually use abbreviations like "Def". How about calling this "DatabaseErrorStruct" or "DatabaseErrorDefinition"?
Comment 3 Mark Lam 2013-01-29 20:54:32 PST
Comment on attachment 185386 [details]
the patch.

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

>> Source/WTF/wtf/TypeSafeEnum.h:51
>> +//     MyEnum value1; // value1 is assigned MyEnum::VALUE_DEFAULT by default.
> 
> Typo: MyEnumDef.

This is not a typo.  With the TypeSafeEnum template, you can say MyEnum::VALUE_DEFAULT and it gets redirected to MyEnumDef::VALUE_DEFAULT.

... which reminds me, I should change that to MyEnum::ValueDefault which is what the style checker says is preferred.  I will update before committing.

>> Source/WebCore/Modules/webdatabase/DatabaseError.h:35
>> +struct DatabaseErrorDef {
> 
> We don't usually use abbreviations like "Def". How about calling this "DatabaseErrorStruct" or "DatabaseErrorDefinition"?

Will change to DatabaseErrorDefinition.
Comment 4 Benjamin Poulain 2013-01-29 21:04:29 PST
C++ 11 has strongly typed enum.

Can't we use that and people without C++ 11 would have weak typed enums? That would make the code simpler.
Comment 5 Mark Lam 2013-01-29 21:11:01 PST
(In reply to comment #4)
> C++ 11 has strongly typed enum.
> 
> Can't we use that and people without C++ 11 would have weak typed enums? That would make the code simpler.

The code is not all that complicated, and the type safety actually has an added benefit of scoping e.g. DatabaseError::None.  Otherwise, I will have to add a prefix.

I'm landing this for now, and will look into how we can abstract this to use C++11 enums if available later.
Comment 6 Benjamin Poulain 2013-01-29 21:15:44 PST
> The code is not all that complicated, and the type safety actually has an added benefit of scoping e.g. DatabaseError::None.  Otherwise, I will have to add a prefix.

enum class are scoped.

> I'm landing this for now, and will look into how we can abstract this to use C++11 enums if available later.

I looked into supporting both but that makes things worse. :(
Ignore me, go ahead with the patch.
Comment 7 Mark Lam 2013-01-29 21:19:25 PST
Landed in r141219: <http://trac.webkit.org/changeset/141219>.
Comment 8 Benjamin Poulain 2013-01-29 21:22:05 PST
Reopening to attach new patch.
Comment 9 Benjamin Poulain 2013-01-29 21:22:06 PST
Created attachment 185390 [details]
Patch
Comment 10 Benjamin Poulain 2013-01-29 21:22:40 PST
Comment on attachment 185390 [details]
Patch

sorry, wrong copy paste :(