| Summary: | IndexedDB: Move LevelDB key coding routines to separate file | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Hans Wennborg <hans> | ||||||
| Component: | New Bugs | Assignee: | Hans Wennborg <hans> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | andreip, dglazkov, dgrogan, tonyg, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Other | ||||||||
| OS: | OS X 10.5 | ||||||||
| Attachments: |
|
||||||||
|
Description
Hans Wennborg
2011-04-26 07:15:56 PDT
Created attachment 91100 [details]
Patch
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 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? (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 Created attachment 91258 [details]
Patch
Attachment 91258 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8510908 Committed r85045: <http://trac.webkit.org/changeset/85045> |