Make TestWebIDBFactory allocate a separate WebIDBFactory per thread.

Make TestWebIDBFactory allocate a separate WebIDBFactory per thread.

This allows the main WebKit thread and web worker threads to each get their own
WebIDBFactory instance, which mimics the behavior when we let the WebIDBFactory
be instantiated via the WebKitPlatformSupport::idbFactory() method.

R=alecflett@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180583

[jsbell, 2013-02-04 18:11:35]

On 2013/02/04 08:21:18, darin wrote:

I'm out sick today and and in no shape to review code. Maybe have dgrogan@ take
a look?

[darin (slow to review), 2013-02-04 18:35:59]

over to alec b/c i just ran into him and verbally described this change

[alecflett, 2013-02-04 18:49:25]

On 2013/02/04 18:35:59, darin wrote:
> over to alec b/c i just ran into him and verbally described this change

This seems fine in theory though I don't know the leveldb access patterns for
multiple threads - i.e. if leveldb supports multiple open()'s from multiple
threads on the same directory, (which would then share the same leveldb::DB) or
if you have to open() it once, and then pass the result leveldb::DB to each
thread, which will access the database in a threadsafe way. 

(it kind of looks like the latter though, which would make this patch fail:
http://leveldb.googlecode.com/svn/trunk/doc/index.html)

I'm gonna say lgtm with the above caveat - if the tests are passing (kinda looks
like they are?) then maybe there's something smart in the leveldb or IDB code
that shares already-open databases by path.

[darin (slow to review), 2013-02-04 19:03:28]

What I observed is that WebIDBFactory::create() returns a
WebKit::WebIDBFactoryImpl instance, which is not thread safe.

Also, using DRT, we have never been able to run the layout tests that
access IDB from the main thread and a web worker and expect to see the same
data.

So, I think this change is safe.

-Darin


On Mon, Feb 4, 2013 at 10:49 AM, <alecflett@chromium.org> wrote:

> On 2013/02/04 18:35:59, darin wrote:
>
>> over to alec b/c i just ran into him and verbally described this change
>>
>
> This seems fine in theory though I don't know the leveldb access patterns
> for
> multiple threads - i.e. if leveldb supports multiple open()'s from multiple
> threads on the same directory, (which would then share the same
> leveldb::DB) or
> if you have to open() it once, and then pass the result leveldb::DB to each
> thread, which will access the database in a threadsafe way.
>
> (it kind of looks like the latter though, which would make this patch fail:
>
http://leveldb.googlecode.com/**svn/trunk/doc/index.html<http://leveldb.googl...
> )
>
> I'm gonna say lgtm with the above caveat - if the tests are passing (kinda
> looks
> like they are?) then maybe there's something smart in the leveldb or IDB
> code
> that shares already-open databases by path.
>
>
https://codereview.chromium.**org/12181010/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-02-04 22:34:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/darin@chromium.org/12181010/7005

[commit-bot: I haz the power, 2013-02-05 00:42:01]

Change committed as 180583

[gavinp, 2013-02-05 19:52:09]

I reverted this in
http://src.chromium.org/viewvc/chrome?view=rev&revision=180758