Bug 106257

Summary: [WK2] Set IndexedDB database path in WK2
Product: WebKit Reporter: Michael Pruett <michael>
Component: New BugsAssignee: 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 Flags
Patch
sam: review-
Patch
benjamin: review-
Patch
benjamin: review+, benjamin: commit-queue-
Patch none

Description Michael Pruett 2013-01-07 14:51:06 PST
The IndexedDB database path should be initialized with the same directory used for web databases in WK2.
Comment 1 Michael Pruett 2013-01-07 14:53:53 PST
Created attachment 181570 [details]
Patch
Comment 2 Sam Weinig 2013-01-07 15:27:34 PST
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.
Comment 3 Michael Pruett 2013-01-07 15:49:44 PST
Created attachment 181583 [details]
Patch

I've eliminated the IndexedDB database manager.
Comment 4 Benjamin Poulain 2013-01-08 16:54:49 PST
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.
Comment 5 Michael Pruett 2013-01-08 20:06:55 PST
Created attachment 181827 [details]
Patch

I've moved the IndexedDB database directory from WebProcess to WebKeyValueStorageManager.
Comment 6 Benjamin Poulain 2013-01-09 14:39:33 PST
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.
Comment 7 Michael Pruett 2013-01-09 14:57:02 PST
Created attachment 181992 [details]
Patch

I've removed an unnecessary pointer-to-reference conversion.
Comment 8 Benjamin Poulain 2013-01-09 15:20:17 PST
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 9 Benjamin Poulain 2013-01-10 13:26:18 PST
Comment on attachment 181992 [details]
Patch

No news from Sam. Let's go ahead.
Comment 10 WebKit Review Bot 2013-01-10 13:50:51 PST
Comment on attachment 181992 [details]
Patch

Clearing flags on attachment: 181992

Committed r139364: <http://trac.webkit.org/changeset/139364>
Comment 11 WebKit Review Bot 2013-01-10 13:50:56 PST
All reviewed patches have been landed.  Closing bug.