Summary: | [WK2] Set IndexedDB database path in WK2 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Pruett <michael> | ||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | alecflett, benjamin, gyuyoung.kim, jsbell, jussi.kukkonen, rakuco, sam, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Michael Pruett
2013-01-07 14:51:06 PST
Created attachment 181570 [details]
Patch
Comment on attachment 181570 [details]
Patch
I don't understand the reason for this new manager. It seems like overkill to set a single string.
Created attachment 181583 [details]
Patch
I've eliminated the IndexedDB database manager.
Comment on attachment 181583 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181583&action=review What this is doing is basically create a global variable m_indexedDBDatabasePath indirectly through WebProcess, and use that to initialize IndexedDBDatabasePath. We should probably pass the path down to WebPageGroupProxy instead of making it global(?). If you really want to make this global, please generalize WebKeyValueStorageManager instead of extending WebProcess (if I am not mistaken, IndexedDB is still a kind of key-value store). > Source/WebKit2/WebProcess/WebProcess.h:183 > +#if ENABLE(INDEXED_DATABASE) > + String indexedDBDatabasePath() const { return m_indexedDBDatabasePath; } > +#endif The return type should be const String&. > Source/WebKit2/WebProcess/WebProcess.h:366 > +#if ENABLE(INDEXED_DATABASE) > + String m_indexedDBDatabasePath; > +#endif This looks like the wrong place to store something like this, this is what Settings are for. Created attachment 181827 [details]
Patch
I've moved the IndexedDB database directory from WebProcess to WebKeyValueStorageManager.
Comment on attachment 181827 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181827&action=review It is not ideal but I think that looks okay to bootstrap IndexedDB. It will probably evolve separately. > Source/WebKit2/WebProcess/WebPage/WebPageGroupProxy.cpp:71 > + WebKeyValueStorageManager& keyValueStorageManager = *WebProcess::shared().supplement<WebKeyValueStorageManager>(); > + m_pageGroup->groupSettings()->setIndexedDBDatabasePath(keyValueStorageManager.indexedDBDatabaseDirectory()); Please don't do the pointer-to-reference conversion. Just use the pointer directly. Created attachment 181992 [details]
Patch
I've removed an unnecessary pointer-to-reference conversion.
Comment on attachment 181992 [details]
Patch
I assume you want to commit queue too? (you did not set cq?)
I set cq- for now, I'll try to get Sam have an other look.
Comment on attachment 181992 [details]
Patch
No news from Sam. Let's go ahead.
Comment on attachment 181992 [details] Patch Clearing flags on attachment: 181992 Committed r139364: <http://trac.webkit.org/changeset/139364> All reviewed patches have been landed. Closing bug. |