Bug 59452 – IndexedDB: Move LevelDB key coding routines to separate file

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

[Hans Wennborg]

IndexedDB: Move LevelDB key coding routines to separate file

[Hans Wennborg]

Created attachment 91100 [details]
Patch

[Hans Wennborg]

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.

[Tony Gentilcore]

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?

[Hans Wennborg]

(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

[Hans Wennborg]

Created attachment 91258 [details]
Patch

[WebKit Review Bot]

Attachment 91258 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8510908

[Hans Wennborg]

Committed r85045: <http://trac.webkit.org/changeset/85045>