Bug 59452 - IndexedDB: Move LevelDB key coding routines to separate file
Summary: IndexedDB: Move LevelDB key coding routines to separate file
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Hans Wennborg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-26 07:15 PDT by Hans Wennborg
Modified: 2011-04-27 04:11 PDT (History)
5 users (show)

See Also:


Attachments
Patch (139.44 KB, patch)
2011-04-26 07:22 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (100.75 KB, patch)
2011-04-27 02:31 PDT, Hans Wennborg
tonyg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Wennborg 2011-04-26 07:15:56 PDT
IndexedDB: Move LevelDB key coding routines to separate file
Comment 1 Hans Wennborg 2011-04-26 07:22:48 PDT
Created attachment 91100 [details]
Patch
Comment 2 Hans Wennborg 2011-04-26 07:25:36 PDT
It's a large patch, but it's really just moving a large chunk of code from one file to another, and making the functions and classes members of IDBLevelDBCoding.

I'm not sure about the use of namespaces within WebKit, but if a reviewer agrees, maybe it would be nicer to have the IDBLevelDBCoding class be a namespace instead, and just do "using namespace IDBLevelDBCoding;" where it's used.
Comment 3 Tony Gentilcore 2011-04-26 10:16:08 PDT
Comment on attachment 91100 [details]
Patch

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

> I'm not sure about the use of namespaces within WebKit, but if a reviewer agrees, maybe it would be nicer to have the IDBLevelDBCoding class be a namespace instead, and just do "using namespace IDBLevelDBCoding;" where it's used.

Namespaces are used very conservatively in WebKit, but there are some in inspector, graphics, audio, xpath. There's even one for leveldb already. This code is already using the class as a namespace (it has lots of classes, static methods, no members and can't be instantiated). So I agree that it would be a lot cleaner to make a namespace. Would you mind making the change?

> Source/WebCore/WebCore.gypi:5526
> +            'storage/IDBLevelDBCoding.h',

Should these also be added to WebCore/GNUMakefile.list.am?

> Source/WebCore/storage/IDBLevelDBCoding.cpp:154
> +#endif

These macros are still in IDBLevelDBBackingStore.cpp. They should only be in one place, right?
Comment 4 Hans Wennborg 2011-04-27 02:31:09 PDT
(In reply to comment #3)
> (From update of attachment 91100 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91100&action=review
> 
> > I'm not sure about the use of namespaces within WebKit, but if a reviewer agrees, maybe it would be nicer to have the IDBLevelDBCoding class be a namespace instead, and just do "using namespace IDBLevelDBCoding;" where it's used.
> 
> Namespaces are used very conservatively in WebKit, but there are some in inspector, graphics, audio, xpath. There's even one for leveldb already. This code is already using the class as a namespace (it has lots of classes, static methods, no members and can't be instantiated). So I agree that it would be a lot cleaner to make a namespace. Would you mind making the change?

Sure. Done.

> 
> > Source/WebCore/WebCore.gypi:5526
> > +            'storage/IDBLevelDBCoding.h',
> 
> Should these also be added to WebCore/GNUMakefile.list.am?

Done.

> 
> > Source/WebCore/storage/IDBLevelDBCoding.cpp:154
> > +#endif
> 
> These macros are still in IDBLevelDBBackingStore.cpp. They should only be in one place, right?

They're used in both files at the moment :S
Comment 5 Hans Wennborg 2011-04-27 02:31:39 PDT
Created attachment 91258 [details]
Patch
Comment 6 WebKit Review Bot 2011-04-27 03:29:53 PDT
Attachment 91258 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8510908
Comment 7 Hans Wennborg 2011-04-27 04:11:48 PDT
Committed r85045: <http://trac.webkit.org/changeset/85045>