Send Allow{Database,FileSystem,IndexedDB} sync IPCs directly from worker thread

Send Allow{Database,FileSystem,IndexedDB} sync IPCs directly from worker thread

- To deprecate the notorious WorkerAllowMainThreadBridge & runInMode code in Blink
- To simplify the worker class inheritance chain

Blink-side change: https://codereview.chromium.org/50773002/

BUG=none

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

[kinuko, 2013-10-29 14:00:43]

Hello, I want to make a few more cleanups in the existing worker code and
wondering if we can do this.  Currently we have number of problems in
WorkerAllowMainThreadBridge in blink code which just bridges all
allow{Database,FileSystem,IndexedDB} queries to the main thread to send the
query from the loading document context. I'm wondering if we can just skip all
these bridge code in Blink but send sync IPC message (using ThreadSafeSender)
from worker threads.  How stupid or sane does this sound?  I might be missing
something obvious, but want to hear your opinion.

Blink side change: https://codereview.chromium.org/50773002/

[jam, 2013-10-29 17:19:57]

seems very reasonable to simplify the blink code by using threading primitives
that we already have on the other side

lgtm

https://codereview.chromium.org/46583005/diff/30001/chrome/renderer/in_render...
File chrome/renderer/in_renderer_worker_permission_client_proxy.cc (right):

https://codereview.chromium.org/46583005/diff/30001/chrome/renderer/in_render...
chrome/renderer/in_renderer_worker_permission_client_proxy.cc:21:
DCHECK(content::RenderThread::Get());
nit: this isn't very useful, since in release builds it does nothing, and in
debug ones the next line is just as helpful to the developer

https://codereview.chromium.org/46583005/diff/30001/chrome/renderer/in_render...
chrome/renderer/in_renderer_worker_permission_client_proxy.cc:35: if
(is_unique_origin_ || !sync_message_filter_)
nit: here and below, why is there the !sync_message_filter check? this should
always be non null

https://codereview.chromium.org/46583005/diff/30001/chrome/renderer/in_render...
File chrome/renderer/in_renderer_worker_permission_client_proxy.h (right):

https://codereview.chromium.org/46583005/diff/30001/chrome/renderer/in_render...
chrome/renderer/in_renderer_worker_permission_client_proxy.h:23: class
InRendererWorkerPermissionClientProxy
nit: InRenderer in the class name is redundant since this code is in
chrome\renderer...

https://codereview.chromium.org/46583005/diff/30001/content/worker/worker_per...
File content/worker/worker_permission_client_proxy.h (right):

https://codereview.chromium.org/46583005/diff/30001/content/worker/worker_per...
content/worker/worker_permission_client_proxy.h:24: content::ThreadSafeSender*
thread_safe_sender);
nit: here and below and in the cc file, no content:: needed

[michaeln, 2013-10-29 21:37:00]

i'll take a look at the blink changes too

https://codereview.chromium.org/46583005/diff/30001/chrome/renderer/in_render...
File chrome/renderer/in_renderer_worker_permission_client_proxy.h (right):

https://codereview.chromium.org/46583005/diff/30001/chrome/renderer/in_render...
chrome/renderer/in_renderer_worker_permission_client_proxy.h:23: class
InRendererWorkerPermissionClientProxy
On 2013/10/29 17:19:57, jam wrote:
> nit: InRenderer in the class name is redundant since this code is in
> chrome\renderer...

I think this is used specifically for dedicated workers, which happens to occur
'in renderer'. If so would it make sense to name this class
DedicatedWorkerPermissionClientProxy and the other
SharedWorkerPermissionClientProxy?

https://codereview.chromium.org/46583005/diff/30001/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/46583005/diff/30001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:196: frame, GetRoutingID());
It looks like the current impl uses the RoutingID of the RenderViewImpl in the
allow message params (see content_settings_observer.cc).

[michaeln, 2013-10-29 22:30:50]

https://codereview.chromium.org/46583005/diff/30001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/46583005/diff/30001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:273: int routing_id);
This api might be cleaner/clearer if a RenderView* param was here in place of
'routing_id'?

[kinuko, 2013-10-30 00:24:56]

Thanks, updated. (I'm going to start blink review thread as this one depends on
it)

https://codereview.chromium.org/46583005/diff/30001/chrome/renderer/in_render...
File chrome/renderer/in_renderer_worker_permission_client_proxy.cc (right):

https://codereview.chromium.org/46583005/diff/30001/chrome/renderer/in_render...
chrome/renderer/in_renderer_worker_permission_client_proxy.cc:21:
DCHECK(content::RenderThread::Get());
On 2013/10/29 17:19:57, jam wrote:
> nit: this isn't very useful, since in release builds it does nothing, and in
> debug ones the next line is just as helpful to the developer

Removed.

https://codereview.chromium.org/46583005/diff/30001/chrome/renderer/in_render...
chrome/renderer/in_renderer_worker_permission_client_proxy.cc:35: if
(is_unique_origin_ || !sync_message_filter_)
On 2013/10/29 17:19:57, jam wrote:
> nit: here and below, why is there the !sync_message_filter check? this should
> always be non null

Removed the check (here and below)

https://codereview.chromium.org/46583005/diff/30001/chrome/renderer/in_render...
File chrome/renderer/in_renderer_worker_permission_client_proxy.h (right):

https://codereview.chromium.org/46583005/diff/30001/chrome/renderer/in_render...
chrome/renderer/in_renderer_worker_permission_client_proxy.h:23: class
InRendererWorkerPermissionClientProxy
On 2013/10/29 21:37:00, michaeln wrote:
> On 2013/10/29 17:19:57, jam wrote:
> > nit: InRenderer in the class name is redundant since this code is in
> > chrome\renderer...
> 
> I think this is used specifically for dedicated workers, which happens to
occur
> 'in renderer'. If so would it make sense to name this class
> DedicatedWorkerPermissionClientProxy and the other
> SharedWorkerPermissionClientProxy?

Looks like for dedicated worker stuff we don't always prefix class names, so I
renamed this to WorkerPermissionClientProxy and the other to
SharedWorkerPermissionClientProxy.

https://codereview.chromium.org/46583005/diff/30001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/46583005/diff/30001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:273: int routing_id);
On 2013/10/29 22:30:50, michaeln wrote:
> This api might be cleaner/clearer if a RenderView* param was here in place of
> 'routing_id'?

Done.

https://codereview.chromium.org/46583005/diff/30001/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/46583005/diff/30001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:196: frame, GetRoutingID());
On 2013/10/29 21:37:00, michaeln wrote:
> It looks like the current impl uses the RoutingID of the RenderViewImpl in the
> allow message params (see content_settings_observer.cc).

Changed the code to pass RenderView (from the frame->view()) and get the routing
id from it.

https://codereview.chromium.org/46583005/diff/30001/content/worker/worker_per...
File content/worker/worker_permission_client_proxy.h (right):

https://codereview.chromium.org/46583005/diff/30001/content/worker/worker_per...
content/worker/worker_permission_client_proxy.h:24: content::ThreadSafeSender*
thread_safe_sender);
On 2013/10/29 17:19:57, jam wrote:
> nit: here and below and in the cc file, no content:: needed

Done.

[michaeln, 2013-10-30 19:27:27]

lgtm

fyi: i keep waiting to see when this cl lands...
https://codereview.chromium.org/23496052/
... until it does, there's a chance that we could get some distracting bug
reports as a result of this sort of change

https://codereview.chromium.org/46583005/diff/290002/chrome/renderer/worker_p...
File chrome/renderer/worker_permission_client_proxy.cc (right):

https://codereview.chromium.org/46583005/diff/290002/chrome/renderer/worker_p...
chrome/renderer/worker_permission_client_proxy.cc:29: ) {}
nit: line wrap is weird

[kinuko, 2013-10-31 05:15:07]

Yup, I'm also waiting for the meacer's shutdown patch to land.

https://codereview.chromium.org/46583005/diff/290002/chrome/renderer/worker_p...
File chrome/renderer/worker_permission_client_proxy.cc (right):

https://codereview.chromium.org/46583005/diff/290002/chrome/renderer/worker_p...
chrome/renderer/worker_permission_client_proxy.cc:29: ) {}
On 2013/10/30 19:27:27, michaeln wrote:
> nit: line wrap is weird

Done.

[commit-bot: I haz the power, 2013-11-01 00:37:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/46583005/620001

[commit-bot: I haz the power, 2013-11-01 05:38:25]

Change committed as 232302