Get IPC working for Indexed DB in shared workers.

Get IPC working for Indexed DB in shared workers.

BUG=112855
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=122231

[dgrogan, 2012-02-09 22:46:06]

Most of these files are getting WebKitContext (which holds IndexedDBContext)
available to WorkerProcessHost.

Before committing this I'm going to have to move files from
content/renderer/indexed_db/ into content/common/indexed_db/.

http://codereview.chromium.org/9375024/diff/1001/content/renderer/indexed_db/...
File content/renderer/indexed_db/indexed_db_dispatcher.cc (left):

http://codereview.chromium.org/9375024/diff/1001/content/renderer/indexed_db/...
content/renderer/indexed_db/indexed_db_dispatcher.cc:184: RenderViewImpl*
render_view = RenderViewImpl::FromWebView(web_frame->view());
This always fails in the shared worker.  I would have thought that there would
no webFrame in the shared worker either but apparently there is.

I don't know how useful this check was in the renderer, anyway.

[jsbell, 2012-02-09 23:23:56]

lgtm, with comments. Possibly get jam@ to review the render_view check? (git
blame says he may have touched one of the other instances)

http://codereview.chromium.org/9375024/diff/1001/content/renderer/indexed_db/...
File content/renderer/indexed_db/indexed_db_dispatcher.cc (left):

http://codereview.chromium.org/9375024/diff/1001/content/renderer/indexed_db/...
content/renderer/indexed_db/indexed_db_dispatcher.cc:184: RenderViewImpl*
render_view = RenderViewImpl::FromWebView(web_frame->view());
Being paranoid, I see this pattern used in a few other places for similar early
exit.... jam@ ?

http://codereview.chromium.org/9375024/diff/1001/content/renderer/indexed_db/...
content/renderer/indexed_db/indexed_db_dispatcher.cc:205: RenderViewImpl*
render_view = RenderViewImpl::FromWebView(web_frame->view());
Remove here too, even though this isn't supported in workers yet?

http://codereview.chromium.org/9375024/diff/1001/content/renderer/indexed_db/...
content/renderer/indexed_db/indexed_db_dispatcher.cc:226: RenderViewImpl*
render_view = RenderViewImpl::FromWebView(web_frame->view());
And here? If not useful, should get rid of it everywhere. (And if it's needed,
this might tickle tests.)

http://codereview.chromium.org/9375024/diff/1001/content/worker/worker_thread.cc
File content/worker/worker_thread.cc (right):

http://codereview.chromium.org/9375024/diff/1001/content/worker/worker_thread...
content/worker/worker_thread.cc:12: #include
"content/renderer/indexed_db/indexed_db_message_filter.h"
I assume this is moving, so order will become correct.

[michaeln, 2012-02-09 23:59:40]

http://codereview.chromium.org/9375024/diff/1001/content/renderer/indexed_db/...
File content/renderer/indexed_db/indexed_db_dispatcher.cc (left):

http://codereview.chromium.org/9375024/diff/1001/content/renderer/indexed_db/...
content/renderer/indexed_db/indexed_db_dispatcher.cc:184: RenderViewImpl*
render_view = RenderViewImpl::FromWebView(web_frame->view());
> I don't know how useful this check was in the renderer, anyway.

These render_view checks are probably here for good reason (script running in a
detached iframe?) and probably shouldn't be removed without understanding that
reason. You can distinguish between the 'worker' vs 'document' case by looking
at the value of CurrentWorkerId(). We should be able to conditionally skip the
sanity-tests for web_frame/render_view in the worker case.

The non-null value of 'web_frame' in the worker case comes from
IDBFactoryBackendProxy.

void IDBFactoryBackendProxy::openFromWorker(const String& name, IDBCallbacks*
callbacks, PassRefPtr<SecurityOrigin> prpOrigin, WorkerContext* context, const
String& dataDir)
{
#if ENABLE(WORKERS)
    WebSecurityOrigin origin(prpOrigin);
    if (!allowIDBFromWorkerThread(context, name, origin)) {
        callbacks->onError(WebIDBDatabaseError(0, "The user denied permission to
access the database."));
        return;
    }
    WorkerLoaderProxy* workerLoaderProxy =
&context->thread()->workerLoaderProxy();
    WebWorkerBase* webWorker = static_cast<WebWorkerBase*>(workerLoaderProxy);
    WebFrame* webFrame = webWorker->view()->mainFrame();
    m_webIDBFactory->open(name, new WebIDBCallbacksImpl(callbacks), origin,
webFrame, dataDir);
#endif
}

Since we're on the worker context thread, it'd probably be safer to pass NULL
for webFrame at this callsite since WebFrame is not thread-safe.

[michaeln, 2012-02-10 00:25:52]

Passing NULL in the worker case would simplify the IDBFactoryBackendProxy method
by a fair amount too.

void IDBFactoryBackendProxy::openFromWorker(...)
{
#if ENABLE(WORKERS)
    WebSecurityOrigin origin(prpOrigin);
    if (!allowIDBFromWorkerThread(context, name, origin)) {
        callbacks->onError(WebIDBDatabaseError(0, "..."));
        return;
    }
    m_webIDBFactory->open(name, new WebIDBCallbacksImpl(callbacks),
                          origin, 0, dataDir);
#endif
}

[dgrogan, 2012-02-15 02:15:33]

This is ready for another look.  The interdiffs aren't clear, sorry.

[michaeln, 2012-02-15 03:45:03]

lgtm, can you run some try jobs

https://chromiumcodereview.appspot.com/9375024/diff/7002/chrome/browser/profi...
File chrome/browser/profiles/profile_io_data.cc (right):

https://chromiumcodereview.appspot.com/9375024/diff/7002/chrome/browser/profi...
chrome/browser/profiles/profile_io_data.cc:478: WebKitContext*
ProfileIOData::ResourceContext::GetWebKitContext() {
probably want the same DCHECK here as the other accessors

https://chromiumcodereview.appspot.com/9375024/diff/7002/content/worker/worke...
File content/worker/worker_webkitplatformsupport_impl.cc (right):

https://chromiumcodereview.appspot.com/9375024/diff/7002/content/worker/worke...
content/worker/worker_webkitplatformsupport_impl.cc:214:
web_idb_factory_.reset(new RendererWebIDBFactoryImpl());
RendererWebIDBFactory looks out of place, is the plan to to rename later? Also
don't need the {}'s

[dgrogan, 2012-02-15 08:34:58]

https://chromiumcodereview.appspot.com/9375024/diff/7002/chrome/browser/profi...
File chrome/browser/profiles/profile_io_data.cc (right):

https://chromiumcodereview.appspot.com/9375024/diff/7002/chrome/browser/profi...
chrome/browser/profiles/profile_io_data.cc:478: WebKitContext*
ProfileIOData::ResourceContext::GetWebKitContext() {
On 2012/02/15 03:45:03, michaeln wrote:
> probably want the same DCHECK here as the other accessors

Done.

https://chromiumcodereview.appspot.com/9375024/diff/7002/content/worker/worke...
File content/worker/worker_webkitplatformsupport_impl.cc (right):

https://chromiumcodereview.appspot.com/9375024/diff/7002/content/worker/worke...
content/worker/worker_webkitplatformsupport_impl.cc:214:
web_idb_factory_.reset(new RendererWebIDBFactoryImpl());
On 2012/02/15 03:45:03, michaeln wrote:
> RendererWebIDBFactory looks out of place, is the plan to to rename later?

Yep.

Also
> don't need the {}'s

Removed.

[jsbell, 2012-02-15 16:44:30]

lgtm

http://codereview.chromium.org/9375024/diff/15001/content/worker/worker_webki...
File content/worker/worker_webkitplatformsupport_impl.cc (right):

http://codereview.chromium.org/9375024/diff/15001/content/worker/worker_webki...
content/worker/worker_webkitplatformsupport_impl.cc:214:
web_idb_factory_.reset(new RendererWebIDBFactoryImpl());
This will only ever be called on the worker thread, right? (I see this pattern
used below, so I assume it's safe)

I notice the WebBlobRegistry API below passes the worker thread into the
constructor. As a future item perhaps we want the ProxyWebIDBFactoryImpl to
take/store its "home" thread and DCHECK that its methods are always called from
that thread?

[commit-bot: I haz the power, 2012-02-15 17:20:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/9375024/15001

[commit-bot: I haz the power, 2012-02-15 17:20:57]

Presubmit check for 9375024-15001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/mock_resource_context.cc,content/worker/worker_thread.cc,content/public/browser/resource_context.h,content/worker/worker_webkitplatformsupport_impl.cc,content/worker/worker_webkitplatformsupport_impl.h,content/browser/mock_resource_context.h,content/browser/worker_host/worker_process_host.cc,content/worker/worker_thread.h,chrome/browser/profiles/profile_io_data.cc,chrome/browser/profiles/profile_io_data.h

Presubmit checks took 1.5s to calculate.

[dgrogan, 2012-02-15 17:49:34]

jam, willchan, could you review as OWNERS?

willchan: chrome/browser/profiles/
jam: content/

[willchan no longer on Chromium, 2012-02-15 18:26:14]

On 2012/02/15 17:49:34, dgrogan wrote:
> jam, willchan, could you review as OWNERS?
> 
> willchan: chrome/browser/profiles/
> jam: content/

LGTM

I don't actually know what the WebKitContext is, and ideally I'd like it to have
better defined lifetime semantics (it appears that it has a NonThreadSafe member
object, DOMStorageContext, that should probably get destroyed on the WebKit
thread, but doesn't appear to be a guarantee that that's the case, since
WebKitContext is refcounted).

I'm speaking out of my domain expertise, and WebKitContext has already existed
for awhile I'm guessing, so I'm fine with this change.

[dgrogan, 2012-02-15 18:40:11]

> I don't actually know what the WebKitContext is, and ideally I'd like it to
have
> better defined lifetime semantics (it appears that it has a NonThreadSafe
member
> object, DOMStorageContext, that should probably get destroyed on the WebKit
> thread, but doesn't appear to be a guarantee that that's the case, since
> WebKitContext is refcounted).

WebKitContext's destructor deletes DOMStorageContext on the WebKit thread, so I
think is ok.

WebKitContext::~WebKitContext() {
  // If the WebKit thread was ever spun up, delete the object there.  The task
  // will just get deleted if the WebKit thread isn't created (which only
  // happens during testing).
  dom_storage_context_->set_clear_local_state_on_exit_(
      clear_local_state_on_exit_);
  DOMStorageContext* dom_storage_context = dom_storage_context_.release();
  if (!BrowserThread::DeleteSoon(
          BrowserThread::WEBKIT_DEPRECATED, FROM_HERE, dom_storage_context)) {
    // The WebKit thread wasn't created, and the task got deleted without
    // freeing the DOMStorageContext, so delete it manually.
    delete dom_storage_context;
  }
...
}

[willchan no longer on Chromium, 2012-02-15 18:44:21]

On Wed, Feb 15, 2012 at 10:40 AM, <dgrogan@chromium.org> wrote:

> I don't actually know what the WebKitContext is, and ideally I'd like it to
>>
> have
>
>> better defined lifetime semantics (it appears that it has a NonThreadSafe
>>
> member
>
>> object, DOMStorageContext, that should probably get destroyed on the
>> WebKit
>> thread, but doesn't appear to be a guarantee that that's the case, since
>> WebKitContext is refcounted).
>>
>
> WebKitContext's destructor deletes DOMStorageContext on the WebKit thread,
> so I
> think is ok.
>

Great


>
> WebKitContext::~WebKitContext(**) {
>  // If the WebKit thread was ever spun up, delete the object there.  The
> task
>  // will just get deleted if the WebKit thread isn't created (which only
>  // happens during testing).
>  dom_storage_context_->set_**clear_local_state_on_exit_(
>      clear_local_state_on_exit_);
>  DOMStorageContext* dom_storage_context = dom_storage_context_.release()**
> ;
>  if (!BrowserThread::DeleteSoon(
>          BrowserThread::WEBKIT_**DEPRECATED, FROM_HERE,
> dom_storage_context)) {
>    // The WebKit thread wasn't created, and the task got deleted without
>    // freeing the DOMStorageContext, so delete it manually.
>    delete dom_storage_context;
>  }
> ...
> }
>
>
http://codereview.chromium.**org/9375024/<http://codereview.chromium.org/9375...
>

[jam, 2012-02-16 00:03:19]

lgtm

[commit-bot: I haz the power, 2012-02-16 00:26:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/9375024/13003

[commit-bot: I haz the power, 2012-02-16 00:26:13]

Can't apply patch for file chrome/browser/profiles/profile_io_data.h.
While running patch -p1 --forward --force;
patching file chrome/browser/profiles/profile_io_data.h
Hunk #1 FAILED at 1.
1 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/profiles/profile_io_data.h.rej

[commit-bot: I haz the power, 2012-02-16 01:31:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/9375024/17015

[commit-bot: I haz the power, 2012-02-16 03:23:43]

Change committed as 122231