Implementation of shared worker code path for WebView file system permission.

Implementation of shared worker code path when WebView guest creates a shared worker to request file system access.

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

[Fady Samuel, 2014-06-19 14:50:47]

lgtm

[Xi Han, 2014-06-19 15:42:44]

Drew: Could you please review:
- content/browser/shared_worker/shared_worker_host.cc
- content/browser/shared_worker/shared_worker_host.h
-  content/browser/shared_worker/shared_worker_message_filter.cc
- content/browser/shared_worker/shared_worker_message_filter.h
- content/browser/shared_worker/shared_worker_service_impl.cc
- content/browser/shared_worker/shared_worker_service_impl.h
- content/browser/worker_host/worker_process_host.cc
- content/browser/worker_host/worker_process_host.h

Thanks a lot for your help!

[Xi Han, 2014-06-19 15:43:46]

Bo: Could you please review:
  - android_webview/browser/aw_content_browser_client.cc
  - android_webview/browser/aw_content_browser_client.h

Thanks a lot for your help!

[boliu, 2014-06-19 15:57:35]

One name nit that's really a content level issue

aw lgtm

https://codereview.chromium.org/338353007/diff/20001/android_webview/browser/...
File android_webview/browser/aw_content_browser_client.h (right):

https://codereview.chromium.org/338353007/diff/20001/android_webview/browser/...
android_webview/browser/aw_content_browser_client.h:84: virtual void
RequestFileSystemAccessSync(
Weird name? It appears the callback allows this to be async, yet the name says
sync?

[Xi Han, 2014-06-19 16:19:42]

John: could you please review:
 - chrome/browser/apps/web_view_browsertest.cc
 - chrome/browser/chrome_content_browser_client.h
 - chrome/browser/chrome_content_browser_client.cc
 - chrome/browser/renderer_host/chrome_render_message_filter.cc
 - chrome/browser/renderer_host/chrome_render_message_filter.h
 - content/public/browser/content_browser_client.cc
 - content/public/browser/content_browser_client.h

[Xi Han, 2014-06-19 16:37:46]

https://codereview.chromium.org/338353007/diff/20001/android_webview/browser/...
File android_webview/browser/aw_content_browser_client.h (right):

https://codereview.chromium.org/338353007/diff/20001/android_webview/browser/...
android_webview/browser/aw_content_browser_client.h:84: virtual void
RequestFileSystemAccessSync(
On 2014/06/19 15:57:35, boliu wrote:
> Weird name? It appears the callback allows this to be async, yet the name says
> sync?

The name comes from the webview guest, when a webview guest requests file system
access, whether it calls a sync api or async api. Now we have both apis:
1) sync api for a request from shared worker & worker (the request will block
workers);
2) async api for a request from a webview guest main thread (request won't block
main thread).

We use a sync message (WorkerProcessHostMsg_RequestFileSystemAccessSync) to
request this api.

Sorry for the confusion.

[jam, 2014-06-19 16:47:28]

https://codereview.chromium.org/338353007/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/338353007/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.h:348: virtual void
RequestWorkerFileSystemAccessSync(
I'm confused, this has a Sync suffix but takes a callback. should it be Async?

[Fady Samuel, 2014-06-19 16:50:11]

https://codereview.chromium.org/338353007/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/338353007/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.h:348: virtual void
RequestWorkerFileSystemAccessSync(
On 2014/06/19 16:47:27, jam wrote:
> I'm confused, this has a Sync suffix but takes a callback. should it be Async?

This is for the sync filesystem API. For webview's case, we're simply blocking
the guest until the embedder makes a permission decision, hence the callback.
This is a delayed response.

[Fady Samuel, 2014-06-19 16:50:11]

https://codereview.chromium.org/338353007/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/338353007/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.h:348: virtual void
RequestWorkerFileSystemAccessSync(
On 2014/06/19 16:47:27, jam wrote:
> I'm confused, this has a Sync suffix but takes a callback. should it be Async?

This is for the sync filesystem API. For webview's case, we're simply blocking
the guest until the embedder makes a permission decision, hence the callback.
This is a delayed response.

[Xi Han, 2014-06-19 17:22:27]

https://codereview.chromium.org/338353007/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/338353007/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.h:348: virtual void
RequestWorkerFileSystemAccessSync(
On 2014/06/19 16:50:11, Fady Samuel wrote:
> On 2014/06/19 16:47:27, jam wrote:
> > I'm confused, this has a Sync suffix but takes a callback. should it be
Async?
> 
> This is for the sync filesystem API. For webview's case, we're simply blocking
> the guest until the embedder makes a permission decision, hence the callback.
> This is a delayed response.

The name comes from the webview guest, whether the guest calls a sync api or
async filesystem API. Now we have both APIs:
1) sync API for a request from shared worker & worker (the request will block
workers);
2) async API for a request from a webview guest main thread (request
won't block main thread).
We use a sync IPC message (WorkerProcessHostMsg_RequestFileSystemAccessSync) to
send request.

[Xi Han, 2014-06-19 20:55:09]

Andrew: Could you please review:
- content/browser/shared_worker/shared_worker_host.cc
- content/browser/shared_worker/shared_worker_host.h
- content/browser/shared_worker/shared_worker_message_filter.cc
- content/browser/shared_worker/shared_worker_message_filter.h
- content/browser/shared_worker/shared_worker_service_impl.cc
- content/browser/shared_worker/shared_worker_service_impl.h
- content/browser/worker_host/worker_process_host.cc
- content/browser/worker_host/worker_process_host.h

Thanks a lot for your help!

[Andrew T Wilson (Slow), 2014-06-20 22:04:27]

I made a pass at this, but honestly jam@ is the expert on the content/ level
worker code and I'd be a bit nervous giving this an OK without his input.

https://codereview.chromium.org/338353007/diff/40001/content/browser/shared_w...
File content/browser/shared_worker/shared_worker_host.cc (right):

https://codereview.chromium.org/338353007/diff/40001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_host.cc:232: base::Unretained(this),
Is this object guaranteed to still be around when the response comes back? What
if the SharedWorker process crashes? Maybe this should be a weak ptr? And if so,
do you need to take steps to prevent reply_msg from leaking?

https://codereview.chromium.org/338353007/diff/40001/content/browser/shared_w...
File content/browser/shared_worker/shared_worker_service_impl.cc (right):

https://codereview.chromium.org/338353007/diff/40001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl.cc:400:
host->RequestFileSystemAccessSync(url, reply_msg);
What happens to reply_msg if we can't find the shared worker?

https://codereview.chromium.org/338353007/diff/40001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/338353007/diff/40001/content/public/browser/c...
content/public/browser/content_browser_client.h:348: virtual void
RequestWorkerFileSystemAccessSync(
Why is this called ...AccessSync() - I'm assuming since you have a callback,
this is an Async call?

[Andrew T Wilson (Slow), 2014-06-20 22:05:32]

>
https://codereview.chromium.org/338353007/diff/40001/content/public/browser/c...
> content/public/browser/content_browser_client.h:348: virtual void
> RequestWorkerFileSystemAccessSync(
> Why is this called ...AccessSync() - I'm assuming since you have a callback,
> this is an Async call?

I noticed jam@ asked this question as well - I think you should probably change
the name of the routine if both external reviewers were confused by this.

[Xi Han, 2014-06-23 02:51:13]

I answered the naming question, and I will take care of other comments ASAP
tomorrow (Monday). Thank you for all the comments.

https://codereview.chromium.org/338353007/diff/40001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/338353007/diff/40001/content/public/browser/c...
content/public/browser/content_browser_client.h:348: virtual void
RequestWorkerFileSystemAccessSync(
On 2014/06/20 22:04:27, Andrew T Wilson wrote:
> Why is this called ...AccessSync() - I'm assuming since you have a callback,
> this is an Async call?

Sorry for the confusion. But this is a sync call, with delay reply. Initially we
implemented it in the way that we did for the Worker: just carry this IPC
reply_msg to the end, and write the result to it and send it. However, the
difficulty that we meet is that we have no way to send a IPC msg in both
aw_content_browser_client.cc and content_browser_client.cc, which need to give
default reply to the IPC msg (true or false). So we adopt the way to pass in a
callback, rather than IPC::Message* reply_msg. So aw_content_browser_client.cc
and content_browser_client.cc only need to give their own default replies, and
the worker_process_host.cc who passes the message will be responsible to send
the reply msg. 

Hope my explanation will help here. If you think the name is still confusing, we
can remove the Sync sufix and just use "RequestWorkerFileSystem". What do you
think?

[jam, 2014-06-23 07:02:00]

https://codereview.chromium.org/338353007/diff/40001/content/browser/shared_w...
File content/browser/shared_worker/shared_worker_host.cc (right):

https://codereview.chromium.org/338353007/diff/40001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_host.cc:232: base::Unretained(this),
On 2014/06/20 22:04:26, Andrew T Wilson wrote:
> Is this object guaranteed to still be around when the response comes back?
What
> if the SharedWorker process crashes? Maybe this should be a weak ptr? And if
so,
> do you need to take steps to prevent reply_msg from leaking?

(i was going backwards and left the same comment on the other callsite :) )

https://codereview.chromium.org/338353007/diff/40001/content/browser/worker_h...
File content/browser/worker_host/worker_process_host.cc (right):

https://codereview.chromium.org/338353007/diff/40001/content/browser/worker_h...
content/browser/worker_host/worker_process_host.cc:485: base::Unretained(this),
what's the guarantee that the embedder will call this while WPH is alive? if
they don't, this will crash. perhaps use weakptrfactory here?

https://codereview.chromium.org/338353007/diff/40001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/338353007/diff/40001/content/public/browser/c...
content/public/browser/content_browser_client.h:348: virtual void
RequestWorkerFileSystemAccessSync(
On 2014/06/23 02:51:13, hanxi wrote:
> On 2014/06/20 22:04:27, Andrew T Wilson wrote:
> > Why is this called ...AccessSync() - I'm assuming since you have a callback,
> > this is an Async call?
> 
> Sorry for the confusion. But this is a sync call, with delay reply.

What does this line mean? If there's a delay reply, you mean the callback is
called back after this method returns right? if so it's async. that's how
chrome's implementation is done right now.

> Initially we
> implemented it in the way that we did for the Worker: just carry this IPC
> reply_msg to the end, and write the result to it and send it. However, the
> difficulty that we meet is that we have no way to send a IPC msg in both
> aw_content_browser_client.cc and content_browser_client.cc, which need to give
> default reply to the IPC msg (true or false). So we adopt the way to pass in a
> callback, rather than IPC::Message* reply_msg. So aw_content_browser_client.cc
> and content_browser_client.cc only need to give their own default replies, and
> the worker_process_host.cc who passes the message will be responsible to send
> the reply msg. 
> 
> Hope my explanation will help here. If you think the name is still confusing,
we
> can remove the Sync sufix and just use "RequestWorkerFileSystem". What do you
> think?

[Xi Han, 2014-06-23 20:43:35]

Changes are made according to all the comments. Also, I did a little bit code
clean up. Please take another look. Thank you for reviewing this CL:)

https://codereview.chromium.org/338353007/diff/40001/content/browser/shared_w...
File content/browser/shared_worker/shared_worker_host.cc (right):

https://codereview.chromium.org/338353007/diff/40001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_host.cc:232: base::Unretained(this),
On 2014/06/20 22:04:26, Andrew T Wilson wrote:
> Is this object guaranteed to still be around when the response comes back?
What
> if the SharedWorker process crashes? Maybe this should be a weak ptr? And if
so,
> do you need to take steps to prevent reply_msg from leaking?


I adopt weak_factory for this pointer; and use scoped_ptr to maintain the life
time of reply_msg. Great catch!

https://codereview.chromium.org/338353007/diff/40001/content/browser/shared_w...
File content/browser/shared_worker/shared_worker_service_impl.cc (right):

https://codereview.chromium.org/338353007/diff/40001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl.cc:400:
host->RequestFileSystemAccessSync(url, reply_msg);
On 2014/06/20 22:04:27, Andrew T Wilson wrote:
> What happens to reply_msg if we can't find the shared worker?

Change the code to send the reply_msg directly if we can't find the shared
worker.

https://codereview.chromium.org/338353007/diff/40001/content/browser/worker_h...
File content/browser/worker_host/worker_process_host.cc (right):

https://codereview.chromium.org/338353007/diff/40001/content/browser/worker_h...
content/browser/worker_host/worker_process_host.cc:485: base::Unretained(this),
On 2014/06/23 07:02:00, jam wrote:
> what's the guarantee that the embedder will call this while WPH is alive? if
> they don't, this will crash. perhaps use weakptrfactory here?

Done.

https://codereview.chromium.org/338353007/diff/40001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/338353007/diff/40001/content/public/browser/c...
content/public/browser/content_browser_client.h:348: virtual void
RequestWorkerFileSystemAccessSync(
On 2014/06/23 07:02:00, jam wrote:
> On 2014/06/23 02:51:13, hanxi wrote:
> > On 2014/06/20 22:04:27, Andrew T Wilson wrote:
> > > Why is this called ...AccessSync() - I'm assuming since you have a
callback,
> > > this is an Async call?
> > 
> > Sorry for the confusion. But this is a sync call, with delay reply.
> 

> What does this line mean? If there's a delay reply, you mean the callback is
> called back after this method returns right? if so it's async. that's how
> chrome's implementation is done right now.
> 
> > Initially we
> > implemented it in the way that we did for the Worker: just carry this IPC
> > reply_msg to the end, and write the result to it and send it. However, the
> > difficulty that we meet is that we have no way to send a IPC msg in both
> > aw_content_browser_client.cc and content_browser_client.cc, which need to
give
> > default reply to the IPC msg (true or false). So we adopt the way to pass in
a
> > callback, rather than IPC::Message* reply_msg. So
aw_content_browser_client.cc
> > and content_browser_client.cc only need to give their own default replies,
and
> > the worker_process_host.cc who passes the message will be responsible to
send
> > the reply msg. 
> > 
> > Hope my explanation will help here. If you think the name is still
confusing,
> we
> > can remove the Sync sufix and just use "RequestWorkerFileSystem". What do
you
> > think?
> 


I see. Thank you for the explanation. All of the "sync" suffix are removed.

https://codereview.chromium.org/338353007/diff/130026/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/338353007/diff/130026/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.h:257: static void
FileSystemAccessed(int render_process_id,
Code clean up is done here: use the same code for both code paths (request from
main thread, or from worker/shared worker thread). Callback is used to do the
different tasks for these two code paths.

[Fady Samuel, 2014-06-24 18:30:43]

https://codereview.chromium.org/338353007/diff/130026/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/338353007/diff/130026/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:232:
base::Unretained(this),
This should probably be a scoped_ref_ptr. I think you might be able to just pass
this?

[Xi Han, 2014-06-24 20:44:23]

https://codereview.chromium.org/338353007/diff/130026/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/338353007/diff/130026/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:232:
base::Unretained(this),
On 2014/06/24 18:30:43, Fady Samuel wrote:
> This should probably be a scoped_ref_ptr. I think you might be able to just
pass
> this?

Great catch! I also updated the OnRequestFileSystemAccessSync()

[jabdelmalek, 2014-06-24 21:23:32]

lgtm with comments

https://codereview.chromium.org/338353007/diff/150001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_host.h (right):

https://codereview.chromium.org/338353007/diff/150001/content/browser/shared_...
content/browser/shared_worker/shared_worker_host.h:68: void
RequestFileSystemAccess(const GURL& url, IPC::Message* reply_msg);
ditto

https://codereview.chromium.org/338353007/diff/150001/content/browser/shared_...
content/browser/shared_worker/shared_worker_host.h:122: void
RequestFileSystemAccessResponse(scoped_ptr<IPC::Message> reply_msg,
AllowFileSystemResponse

https://codereview.chromium.org/338353007/diff/150001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_service_impl.cc (right):

https://codereview.chromium.org/338353007/diff/150001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl.cc:399: if
(SharedWorkerHost* host = FindSharedWorkerHost(filter, worker_route_id))
nit: need braces here per style guide

https://codereview.chromium.org/338353007/diff/150001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_service_impl.h (right):

https://codereview.chromium.org/338353007/diff/150001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl.h:78: void
RequestFileSystemAccess(int worker_route_id,
ditto keep old name

https://codereview.chromium.org/338353007/diff/150001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/338353007/diff/150001/content/public/browser/...
content/public/browser/content_browser_client.h:320: virtual void
RequestWorkerFileSystemAccess(
nit: keep the old name. The "Allow" is used by convention in this class. also
Access is redundant with Allow prefix

[Xi Han, 2014-06-25 14:50:26]

https://codereview.chromium.org/338353007/diff/150001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_host.h (right):

https://codereview.chromium.org/338353007/diff/150001/content/browser/shared_...
content/browser/shared_worker/shared_worker_host.h:68: void
RequestFileSystemAccess(const GURL& url, IPC::Message* reply_msg);
On 2014/06/24 21:23:31, jabdelmalek wrote:
> ditto

Done.

https://codereview.chromium.org/338353007/diff/150001/content/browser/shared_...
content/browser/shared_worker/shared_worker_host.h:122: void
RequestFileSystemAccessResponse(scoped_ptr<IPC::Message> reply_msg,
On 2014/06/24 21:23:31, jabdelmalek wrote:
> AllowFileSystemResponse

Done.

https://codereview.chromium.org/338353007/diff/150001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_service_impl.cc (right):

https://codereview.chromium.org/338353007/diff/150001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl.cc:399: if
(SharedWorkerHost* host = FindSharedWorkerHost(filter, worker_route_id))
On 2014/06/24 21:23:31, jabdelmalek wrote:
> nit: need braces here per style guide

Done.

https://codereview.chromium.org/338353007/diff/150001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_service_impl.h (right):

https://codereview.chromium.org/338353007/diff/150001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl.h:78: void
RequestFileSystemAccess(int worker_route_id,
On 2014/06/24 21:23:31, jabdelmalek wrote:
> ditto keep old name

Done.

https://codereview.chromium.org/338353007/diff/150001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/338353007/diff/150001/content/public/browser/...
content/public/browser/content_browser_client.h:320: virtual void
RequestWorkerFileSystemAccess(
On 2014/06/24 21:23:32, jabdelmalek wrote:
> nit: keep the old name. The "Allow" is used by convention in this class. also
> Access is redundant with Allow prefix

Done.

[jam, 2014-06-25 21:51:29]

slgtm

[Andrew T Wilson (Slow), 2014-06-26 15:49:33]

lgtm with one suggestion

https://codereview.chromium.org/338353007/diff/170001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_host.cc (right):

https://codereview.chromium.org/338353007/diff/170001/content/browser/shared_...
content/browser/shared_worker/shared_worker_host.cc:226: if (!instance_)
Does reply_msg leak in this case? Perhaps reply_msg should be passed in as a
scoped_ptr?

[Xi Han, 2014-06-26 18:57:53]

https://codereview.chromium.org/338353007/diff/170001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_host.cc (right):

https://codereview.chromium.org/338353007/diff/170001/content/browser/shared_...
content/browser/shared_worker/shared_worker_host.cc:226: if (!instance_)
On 2014/06/26 15:49:33, Andrew T Wilson wrote:
> Does reply_msg leak in this case? Perhaps reply_msg should be passed in as a
> scoped_ptr?

Done.

[Xi Han, 2014-07-03 14:33:00]

This CL is updated due to a test failure in chromeos, which is caused by sending
a reply message multiple times for one request in chrome_content_browser_client.

1. Update the code in a for loop of chrome_content_browser_client.cc to make
sure the reply message will be send only once.
2. Add 3 tests for the case that the embedder containing multiple guests who
actually share the same shared worker.

Please take another look. Thank you very much!

https://codereview.chromium.org/338353007/diff/230001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/338353007/diff/230001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1835: for (i =
render_frames.begin(); i != render_frames.end(); ++i) {
With this implementation, we will guaranty the callback which sends a reply
message will only be run once. Since the shared worker only receives one
request, it is wrong to send a reply multiple times.

When with guests:
1. We will just ask the first the guest who connects to this shared worker
whether the request is allowed or denied. Because we don't now exactly which
guest sends the request, and it is weird that if multiple guests share one
shared worker but set different permissions to the same worker. So we always ask
the first guest in the list to make a decision.
2. updates all the frames connecting to this shared worker.

Without guests:
1. updates all the frames connecting to this shared worker.

https://codereview.chromium.org/338353007/diff/230001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/shared_worker/multiple/embedder.js
(right):

https://codereview.chromium.org/338353007/diff/230001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/shared_worker/multiple/embedder.js:124:
function testAllow() {
Add 3 tests for the case that embedder with multiple guests.

[Xi Han, 2014-07-03 15:02:51]

https://codereview.chromium.org/338353007/diff/250001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/338353007/diff/250001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1835: for (i =
render_frames.begin(); i != render_frames.end(); ++i) {
With this implementation, we will guaranty the callback which sends a reply
message will only be run once. Since the shared worker only receives one
request, it is wrong to send a reply multiple times.

When with guests:
1. We will just ask the first the guest who connects to this shared worker
whether the request is allowed or denied. Because we don't now exactly which
guest sends the request, and it is weird that if multiple guests share one
shared worker but set different permissions to the same worker. So we always ask
the first guest in the list to make a decision.
2. updates all the frames connecting to this shared worker.

Without guests:
1. updates all the frames connecting to this shared worker.

https://codereview.chromium.org/338353007/diff/250001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/shared_worker/multiple/embedder.js
(right):

https://codereview.chromium.org/338353007/diff/250001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/shared_worker/multiple/embedder.js:125:
var webview = embedder.setUpGuest_();
Add 3 tests for the case that embedder with multiple guests.

[Fady Samuel, 2014-07-08 14:42:46]

https://codereview.chromium.org/338353007/diff/250001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/338353007/diff/250001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1842:
base::Bind(&WebViewGuest::RequestFileSystemAccessPermission,
RequestFileSystemPermissionFromIOThread

https://codereview.chromium.org/338353007/diff/250001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/338353007/diff/250001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:965: callback.Run(allowed);
attached() && allowed

https://codereview.chromium.org/338353007/diff/250001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:969: void
WebViewGuest::RequestFileSystemAccessPermission(
RequestFileSystemPermissionFromIOThread

Do the thread hop here.

https://codereview.chromium.org/338353007/diff/250001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:973: bool
blocked_by_policy,
Can we reduce "blocked_by_policy"? Replace instances of blocked_by_policy with
allow in WebViewGuest.

https://codereview.chromium.org/338353007/diff/250001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_service_impl.cc (right):

https://codereview.chromium.org/338353007/diff/250001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl.cc:401: }
} else {

[Xi Han, 2014-07-08 21:28:24]

https://codereview.chromium.org/338353007/diff/250001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/338353007/diff/250001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1842:
base::Bind(&WebViewGuest::RequestFileSystemAccessPermission,
On 2014/07/08 14:42:46, Fady Samuel wrote:
> RequestFileSystemPermissionFromIOThread

I declare a static function in
ChromeContentBrowserClient::FileSystemAccessedOnUIThread to do the thread hop,
and remove WebViewGuest::RequestFileSystemAccessPermission will be removed.

https://codereview.chromium.org/338353007/diff/250001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/338353007/diff/250001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:965: callback.Run(allowed);
On 2014/07/08 14:42:46, Fady Samuel wrote:
> attached() && allowed

The attached() && allowed is done in
WebViewGuest::OnWebViewFileSystemPermissionResponse. This callback is a
different one, which sends a reply message.

https://codereview.chromium.org/338353007/diff/250001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:969: void
WebViewGuest::RequestFileSystemAccessPermission(
On 2014/07/08 14:42:46, Fady Samuel wrote:
> RequestFileSystemPermissionFromIOThread
> 
> Do the thread hop here.

Done.

https://codereview.chromium.org/338353007/diff/250001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:973: bool
blocked_by_policy,
On 2014/07/08 14:42:46, Fady Samuel wrote:
> Can we reduce "blocked_by_policy"? Replace instances of blocked_by_policy with
> allow in WebViewGuest.

Done.

https://codereview.chromium.org/338353007/diff/250001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_service_impl.cc (right):

https://codereview.chromium.org/338353007/diff/250001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl.cc:401: }
On 2014/07/08 14:42:46, Fady Samuel wrote:
> } else {

Done.

[Fady Samuel, 2014-07-08 21:49:11]

https://codereview.chromium.org/338353007/diff/290001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/338353007/diff/290001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1836:
WebViewRendererState::GetInstance()->IsGuest(i->first);
This code doesn't make sense to me. If there are two webviews in the same
process, then we don't need to make the decision again. If there are multiple
processes, then we need to make multiple permission requests.

I would suggest iterating over the vector, creating a map:

process ID => vector of routing ID.

Then for each key in the map, request permission of a webview.

[Fady Samuel, 2014-07-08 21:49:11]

https://codereview.chromium.org/338353007/diff/290001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/338353007/diff/290001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1836:
WebViewRendererState::GetInstance()->IsGuest(i->first);
This code doesn't make sense to me. If there are two webviews in the same
process, then we don't need to make the decision again. If there are multiple
processes, then we need to make multiple permission requests.

I would suggest iterating over the vector, creating a map:

process ID => vector of routing ID.

Then for each key in the map, request permission of a webview.

[Xi Han, 2014-07-09 15:28:33]

https://codereview.chromium.org/338353007/diff/290001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/338353007/diff/290001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1836:
WebViewRendererState::GetInstance()->IsGuest(i->first);
On 2014/07/08 21:49:11, Fady Samuel wrote:
> This code doesn't make sense to me. If there are two webviews in the same
> process, then we don't need to make the decision again. If there are multiple
> processes, then we need to make multiple permission requests.
> 
> I would suggest iterating over the vector, creating a map:
> 
> process ID => vector of routing ID.
> 
> Then for each key in the map, request permission of a webview.

Done.

[Fady Samuel, 2014-07-09 21:08:33]

lgtm with nit

https://codereview.chromium.org/338353007/diff/370001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/338353007/diff/370001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1858: if
(WebViewRendererState::GetInstance()->IsGuest(i->first)) {
Remove block.

[Xi Han, 2014-07-09 21:27:28]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-07-09 21:28:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hanxi@chromium.org/338353007/390001

[commit-bot: I haz the power, 2014-07-10 02:49:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-10 07:45:53]

Change committed as 282272