Avoid sync IPCs for FileSystem API [chromium] (2/4)

Avoid sync IPCs for FileSystem API
This patch continues the work of issue 170733004(https://codereview.chromium.org/170733004).

Two things are done in this patch:
1) Update the patch to compile again.
2) Move IPC handler to chrome_render_message_filter, and update content_settings_observer.cc.

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

[Xi Han, 2014-05-07 18:01:20]

jochen: I update Fady's patch according to your comments. please take a look at
of the changes in chrome_render_message_filter.cc(h).

kinuko: please take another look at the changes in
chrome/renderer/content_settings_observer.cc.

cdn: please review file chrome/common/render_messages.h.

[jochen (gone - plz use gerrit), 2014-05-08 07:38:52]

https://codereview.chromium.org/273513005/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/273513005/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/tab_specific_content_settings.cc:22: #include
"chrome/browser/content_settings/cookie_settings.h"
unrelated change?

https://codereview.chromium.org/273513005/diff/1/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/273513005/diff/1/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:641: return;
when can this happen?

https://codereview.chromium.org/273513005/diff/1/chrome/renderer/worker_permi...
File chrome/renderer/worker_permission_client_proxy.cc (right):

https://codereview.chromium.org/273513005/diff/1/chrome/renderer/worker_permi...
chrome/renderer/worker_permission_client_proxy.cc:59: routing_id_,
document_origin_url_, top_frame_origin_url_, &result));
can't this be async either?

[Xi Han, 2014-05-08 19:48:30]

https://codereview.chromium.org/273513005/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/273513005/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/tab_specific_content_settings.cc:22: #include
"chrome/browser/content_settings/cookie_settings.h"
On 2014/05/08 07:38:53, jochen wrote:
> unrelated change?

Done.

https://codereview.chromium.org/273513005/diff/1/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/273513005/diff/1/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:641: return;
On 2014/05/08 07:38:53, jochen wrote:
> when can this happen?

I am not sure when it would happen, but it is better to do the check when doing
search, just for the safety of the code. What do you think?

https://codereview.chromium.org/273513005/diff/1/chrome/renderer/worker_permi...
File chrome/renderer/worker_permission_client_proxy.cc (right):

https://codereview.chromium.org/273513005/diff/1/chrome/renderer/worker_permi...
chrome/renderer/worker_permission_client_proxy.cc:59: routing_id_,
document_origin_url_, top_frame_origin_url_, &result));
On 2014/05/08 07:38:53, jochen wrote:
> can't this be async either?

I think the requestFileSystemAccess() is called in the worker's thread, not the
main thread. Since it won't block the main thread, it  is fine to send a sync
message. What is your opinion?

[Xi Han, 2014-05-13 14:52:37]

A bug from my previous CL is fixed. Please take another look at. Thanks a lot.

https://codereview.chromium.org/273513005/diff/40001/chrome/browser/renderer_...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/273513005/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:479: Send(new
ChromeViewMsg_RequestFileSystemAccessResponse(
Fix a bug by moving the call of sending the asynchronized message to a callback
function. This way guarantees the message will be send after the result (whether
allowed or not) is obtained.

[Xi Han, 2014-05-13 15:02:51]

A bug from my previous CL is fixed. Please take another look at. Thanks a lot.

https://codereview.chromium.org/273513005/diff/60001/chrome/browser/renderer_...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/273513005/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:479: Send(new
ChromeViewMsg_RequestFileSystemAccessResponse(
Fix a bug by moving the call of sending the asynchronized message to a callback
function. This way guarantees the message will be send after the result (whether
allowed or not) is obtained.

[Fady Samuel, 2014-05-13 17:22:25]

https://codereview.chromium.org/273513005/diff/60001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/273513005/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:208: void
TabSpecificContentSettings::FileSystemAccessedAsyn(
No need for this function.

https://codereview.chromium.org/273513005/diff/60001/chrome/browser/renderer_...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/273513005/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:466:
&TabSpecificContentSettings::FileSystemAccessedAsyn,
No need for this function, see below.

https://codereview.chromium.org/273513005/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:479: Send(new
ChromeViewMsg_RequestFileSystemAccessResponse(
On 2014/05/13 15:02:51, hanxi wrote:
> Fix a bug by moving the call of sending the asynchronized message to a
callback
> function. This way guarantees the message will be send after the result
(whether
> allowed or not) is obtained. 

This is running on the UI thread. Is that intended? I think what you want can be
accomplished by PostTaskAndReply or PostTaskAndReplyWithResult:

https://code.google.com/p/chromium/codesearch#chromium/src/base/task_runner.h...

[Xi Han, 2014-05-13 18:08:37]

https://codereview.chromium.org/273513005/diff/60001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/273513005/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:208: void
TabSpecificContentSettings::FileSystemAccessedAsyn(
On 2014/05/13 17:22:25, Fady Samuel wrote:
> No need for this function.

Done.

https://codereview.chromium.org/273513005/diff/60001/chrome/browser/renderer_...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/273513005/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:466:
&TabSpecificContentSettings::FileSystemAccessedAsyn,
On 2014/05/13 17:22:25, Fady Samuel wrote:
> No need for this function, see below.

Done.

https://codereview.chromium.org/273513005/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:479: Send(new
ChromeViewMsg_RequestFileSystemAccessResponse(
On 2014/05/13 17:22:25, Fady Samuel wrote:
> On 2014/05/13 15:02:51, hanxi wrote:
> > Fix a bug by moving the call of sending the asynchronized message to a
> callback
> > function. This way guarantees the message will be send after the result
> (whether
> > allowed or not) is obtained. 
> 
> This is running on the UI thread. Is that intended? I think what you want can
be
> accomplished by PostTaskAndReply or PostTaskAndReplyWithResult:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/task_runner.h...

Done.

[Fady Samuel, 2014-05-13 18:11:49]

https://codereview.chromium.org/273513005/diff/80001/chrome/browser/renderer_...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/273513005/diff/80001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:452: const
IPC::Message& message) {
Add a DCHECK to verify we're on the IO thread.

https://codereview.chromium.org/273513005/diff/80001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:479: void
ChromeRenderMessageFilter::OnRequestFileSyatemAccessCallbacks(
Call it:

ChromeRenderMessageFilter::OnRequestFileSystemAccessResponse

Add a DCHECK to verify that we're on the IO thread.

[Xi Han, 2014-05-13 18:21:29]

https://codereview.chromium.org/273513005/diff/80001/chrome/browser/renderer_...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/273513005/diff/80001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:452: const
IPC::Message& message) {
On 2014/05/13 18:11:49, Fady Samuel wrote:
> Add a DCHECK to verify we're on the IO thread.

Done.

https://codereview.chromium.org/273513005/diff/80001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:479: void
ChromeRenderMessageFilter::OnRequestFileSyatemAccessCallbacks(
On 2014/05/13 18:11:49, Fady Samuel wrote:
> Call it:
> 
> ChromeRenderMessageFilter::OnRequestFileSystemAccessResponse
> 
> Add a DCHECK to verify that we're on the IO thread.

Done.

[Fady Samuel, 2014-05-13 18:22:52]

lgtm, but I'm not an OWNER. Jochen should review this.

[jochen (gone - plz use gerrit), 2014-05-14 08:27:23]

i think it's odd to have both a sync and async code path.

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

https://codereview.chromium.org/273513005/diff/100001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:129:
OnRequestFileSystemAccess(message))
please clang-format

https://codereview.chromium.org/273513005/diff/100001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:459: return;
nit add { }  because of multi-line if

https://codereview.chromium.org/273513005/diff/100001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/273513005/diff/100001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:297: bool inserted =
insert_result.second;
won't this get you an unused variable warning in release builds?

[kinuko, 2014-05-14 08:48:49]

https://codereview.chromium.org/273513005/diff/100001/chrome/renderer/worker_...
File chrome/renderer/worker_permission_client_proxy.cc (right):

https://codereview.chromium.org/273513005/diff/100001/chrome/renderer/worker_...
chrome/renderer/worker_permission_client_proxy.cc:58:
sync_message_filter_->Send(new ChromeViewHostMsg_AllowFileSystem(
Having sync and async path feels a bit weird to me too (I remember Fady once
tried implementing it but has removed the part, was that due to complexity?).

If we have reasonable reason not to do so I think the reason should be stated
here, and also these message names should reflect the fact clearly, i.e. having
2 similar messages AllowFileSystem and RequestFileSystemAccess working syncly
and asyncly feels confusing to me.

[Xi Han, 2014-05-14 19:55:07]

The rename in blink ends up with another review. Please refer to
https://codereview.chromium.org/289793002/.

My cl in this review is also updated, please take another look at it. 

Thank you very much for all your comments.

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

https://codereview.chromium.org/273513005/diff/100001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:129:
OnRequestFileSystemAccess(message))
On 2014/05/14 08:27:23, jochen wrote:
> please clang-format

Done.

https://codereview.chromium.org/273513005/diff/100001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:459: return;
On 2014/05/14 08:27:23, jochen wrote:
> nit add { }  because of multi-line if

Done.

https://codereview.chromium.org/273513005/diff/100001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/273513005/diff/100001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:297: bool inserted =
insert_result.second;
On 2014/05/14 08:27:23, jochen wrote:
> won't this get you an unused variable warning in release builds?

I don't know why I didn't get a warning, maybe because I run debug build every
time, rather than a release build. Good catch, thanks.

https://codereview.chromium.org/273513005/diff/100001/chrome/renderer/worker_...
File chrome/renderer/worker_permission_client_proxy.cc (right):

https://codereview.chromium.org/273513005/diff/100001/chrome/renderer/worker_...
chrome/renderer/worker_permission_client_proxy.cc:58:
sync_message_filter_->Send(new ChromeViewHostMsg_AllowFileSystem(
I asked the same question to Fady and I believe the complexity is the major
concern. We have tested and confirmed that this function is only called in the
worker's thread, and it won't block the main thread of the browser. I have put
comments on the code.

Also, I rename both of these methods as:
allowFileSystem -> requestFileSystemAccessSync &
requestFileSystemAccess -> requestFileSystemAccessAsync.

The related changes in blink please refer to review
https://codereview.chromium.org/289793002/.


On 2014/05/14 08:48:50, kinuko wrote:
> Having sync and async path feels a bit weird to me too (I remember Fady once
> tried implementing it but has removed the part, was that due to complexity?).
> 
> If we have reasonable reason not to do so I think the reason should be stated
> here, and also these message names should reflect the fact clearly, i.e.
having
> 2 similar messages AllowFileSystem and RequestFileSystemAccess working syncly
> and asyncly feels confusing to me.

[jochen (gone - plz use gerrit), 2014-05-15 10:33:05]

On 2014/05/14 19:55:07, hanxi wrote:
> sync_message_filter_->Send(new ChromeViewHostMsg_AllowFileSystem(
> I asked the same question to Fady and I believe the complexity is the major
> concern. We have tested and confirmed that this function is only called in the
> worker's thread, and it won't block the main thread of the browser. I have put
> comments on the code.

I'm not sure I follow. You already have the callback plumbed through all the
way, but don't use it as a callback.

At the same time, the IPC handler on the browser side are duplicate.

In general, we want to avoid sync IPCs as much as possible.

[Xi Han, 2014-05-15 15:57:51]

I realized that I missed the changes of shared worker, and now add them to the
CL. Please take another look at. Thanks a lot.

JoChen & Fady: as discussed offline, I will keep the implementation as it is
now.

[Fady Samuel, 2014-05-15 16:08:09]

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

https://codereview.chromium.org/273513005/diff/180001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:483: void
ChromeRenderMessageFilter::OnRequestFileSyatemAccessAsyncCallbacks(
Spelling error:
ChromeRenderMessageFilter::OnRequestFileSystemAccessAsyncCallbacks.

https://codereview.chromium.org/273513005/diff/180001/chrome/renderer/worker_...
File chrome/renderer/worker_permission_client_proxy.cc (right):

https://codereview.chromium.org/273513005/diff/180001/chrome/renderer/worker_...
chrome/renderer/worker_permission_client_proxy.cc:72: // the main thread of the
browser.
the main thread. Remove "of the browser", we are not in the browser process.

https://codereview.chromium.org/273513005/diff/180001/chrome/renderer/worker_...
chrome/renderer/worker_permission_client_proxy.cc:73: // 2. Compare with sending
a asynchronized message, reuse sync_message_filter
// 2. The sync filesystem API requires blocking. Using an asynchronous IPC would
introduce additional complexity to block the worker thread.

[Xi Han, 2014-05-15 16:19:58]

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

https://codereview.chromium.org/273513005/diff/180001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:483: void
ChromeRenderMessageFilter::OnRequestFileSyatemAccessAsyncCallbacks(
On 2014/05/15 16:08:09, Fady Samuel wrote:
> Spelling error:
> ChromeRenderMessageFilter::OnRequestFileSystemAccessAsyncCallbacks.

Done.

https://codereview.chromium.org/273513005/diff/180001/chrome/renderer/worker_...
File chrome/renderer/worker_permission_client_proxy.cc (right):

https://codereview.chromium.org/273513005/diff/180001/chrome/renderer/worker_...
chrome/renderer/worker_permission_client_proxy.cc:72: // the main thread of the
browser.
On 2014/05/15 16:08:09, Fady Samuel wrote:
> the main thread. Remove "of the browser", we are not in the browser process.

Done.

https://codereview.chromium.org/273513005/diff/180001/chrome/renderer/worker_...
chrome/renderer/worker_permission_client_proxy.cc:73: // 2. Compare with sending
a asynchronized message, reuse sync_message_filter
Thanks for updating the comments.

On 2014/05/15 16:08:09, Fady Samuel wrote:
> // 2. The sync filesystem API requires blocking. Using an asynchronous IPC
would
> introduce additional complexity to block the worker thread.

[Fady Samuel, 2014-05-15 16:26:07]

https://codereview.chromium.org/273513005/diff/200001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/273513005/diff/200001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:282: bool
ContentSettingsObserver::requestFileSystemAccessSync() {
Delete this. This code should only operate in a worker thread.

[Xi Han, 2014-05-15 18:35:39]

https://codereview.chromium.org/273513005/diff/200001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/273513005/diff/200001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:282: bool
ContentSettingsObserver::requestFileSystemAccessSync() {
On 2014/05/15 16:26:07, Fady Samuel wrote:
> Delete this. This code should only operate in a worker thread.

Done.

[Xi Han, 2014-05-15 21:02:59]

Remove function WorkerPermissionClientProxy::requestFileSystemAccessAsync()
which uses sync_message_filter to send async message. This function is no longer
needed in the code path.

Thank you for reviewing my change.

[jochen (gone - plz use gerrit), 2014-05-16 09:25:51]

lgtm

[Xi Han, 2014-05-16 13:22:52]

dcheng: Please take a look at the following two files:
  chrome/common/render_messages.h
  content/common/worker_messages.h

Thanks a lot!

[dcheng, 2014-05-20 21:16:03]

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

https://codereview.chromium.org/273513005/diff/230001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:129:
IPC_MESSAGE_HANDLER_GENERIC(ChromeViewHostMsg_RequestFileSystemAccessAsync,
Why use GENERIC here instead of using the default unpacking?

[Xi Han, 2014-05-20 21:18:31]

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

https://codereview.chromium.org/273513005/diff/230001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:129:
IPC_MESSAGE_HANDLER_GENERIC(ChromeViewHostMsg_RequestFileSystemAccessAsync,
On 2014/05/20 21:16:04, dcheng wrote:
> Why use GENERIC here instead of using the default unpacking?

It is because we need the routing_id of the message. If use the default one, we
won't be able to get this id.

[dcheng, 2014-05-20 21:48:10]

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

https://codereview.chromium.org/273513005/diff/230001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:129:
IPC_MESSAGE_HANDLER_GENERIC(ChromeViewHostMsg_RequestFileSystemAccessAsync,
On 2014/05/20 21:18:32, hanxi wrote:
> On 2014/05/20 21:16:04, dcheng wrote:
> > Why use GENERIC here instead of using the default unpacking?
> 
> It is because we need the routing_id of the message. If use the default one,
we
> won't be able to get this id.

You don't need the routing ID. Use IPC_MESSAGE_HANDLER_DELAY_REPLY.

[jam, 2014-05-20 22:08:06]

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

https://codereview.chromium.org/273513005/diff/230001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:129:
IPC_MESSAGE_HANDLER_GENERIC(ChromeViewHostMsg_RequestFileSystemAccessAsync,
On 2014/05/20 21:48:10, dcheng wrote:
> On 2014/05/20 21:18:32, hanxi wrote:
> > On 2014/05/20 21:16:04, dcheng wrote:
> > > Why use GENERIC here instead of using the default unpacking?
> > 
> > It is because we need the routing_id of the message. If use the default one,
> we
> > won't be able to get this id.
> 
> You don't need the routing ID. Use IPC_MESSAGE_HANDLER_DELAY_REPLY.

IPCs dispatched in a BrowserMessageFilter should be control; routing is used to
_automatically_ route IPCs to a registered per-route observer, which doesn't
happen on the IO thread. The routing_id_ that is sent in the renderer should
then be the first parameter of the control IPC, named render_frame_id

[Xi Han, 2014-05-21 22:04:27]

jam & dcheng: I have adopted the control IPC, please take another look at the
changes. Thank you for this valuable comments.

kinuko: Please take a look at the codes as whole and let me know if you have any
concern. Thanks a lot for reviewing.

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

https://codereview.chromium.org/273513005/diff/230001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:129:
IPC_MESSAGE_HANDLER_GENERIC(ChromeViewHostMsg_RequestFileSystemAccessAsync,
On 2014/05/20 22:08:06, jam wrote:
> On 2014/05/20 21:48:10, dcheng wrote:
> > On 2014/05/20 21:18:32, hanxi wrote:
> > > On 2014/05/20 21:16:04, dcheng wrote:
> > > > Why use GENERIC here instead of using the default unpacking?
> > > 
> > > It is because we need the routing_id of the message. If use the default
one,
> > we
> > > won't be able to get this id.
> > 
> > You don't need the routing ID. Use IPC_MESSAGE_HANDLER_DELAY_REPLY.
> 
> IPCs dispatched in a BrowserMessageFilter should be control; routing is used
to
> _automatically_ route IPCs to a registered per-route observer, which doesn't
> happen on the IO thread. The routing_id_ that is sent in the renderer should
> then be the first parameter of the control IPC, named render_frame_id

Done.

[jam, 2014-05-21 22:10:15]

On 2014/05/21 22:04:27, hanxi wrote:
> jam & dcheng: I have adopted the control IPC, please take another look at the
> changes. Thank you for this valuable comments.

lgtm

[dcheng, 2014-05-21 22:20:34]

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

https://codereview.chromium.org/273513005/diff/250001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:463:
BrowserThread::PostTaskAndReply(
Why use PostTaskAndReply here? You can queue the IPC immediately.

[kinuko, 2014-05-22 10:54:44]

lgtm (if dcheng's comment is addressed), modulo one nit

https://codereview.chromium.org/273513005/diff/250001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/273513005/diff/250001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:288: permissionCallbacks.doDeny();
This pattern (assigning it to a local variable just to call a method on it)
makes me wonder why we pass these callbacks by const ref??  The body of the
callbacks is just a single private ptr, passing it as const ref doesn't seem to
help a lot.

[Xi Han, 2014-05-22 17:24:48]

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

https://codereview.chromium.org/273513005/diff/250001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:463:
BrowserThread::PostTaskAndReply(
On 2014/05/21 22:20:35, dcheng wrote:
> Why use PostTaskAndReply here? You can queue the IPC immediately.

Good catch, I shouldn't use PostTaskAndReply() here, since the result won't be
updated when the callback is called. I move the sending message part to
TabSpecificContentSettings::FileSystemAccessedAsync, and rename
TabSpecificContentSettings::FileSystemAccessed as
TabSpecificContentSettings::FileSystemAccessedSync.

https://codereview.chromium.org/273513005/diff/250001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/273513005/diff/250001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:288: permissionCallbacks.doDeny();
On 2014/05/22 10:54:45, kinuko wrote:
> This pattern (assigning it to a local variable just to call a method on it)
> makes me wonder why we pass these callbacks by const ref??  The body of the
> callbacks is just a single private ptr, passing it as const ref doesn't seem
to
> help a lot.

I took over this patch from Fady, and I discussed this with him before. I agree
with you, we can remove the const, and just pass in a ref. Since
WebPermissionCallback is used in this way in several places, I will do the
refactory in a subsequent patch.

[Xi Han, 2014-05-22 18:29:43]

Daniel: according to the discussion offline, I update the code to send the IPC
message immediately. The reply part is moved to a subsequent patch. Please take
another look. Thanks for your comments.

[dcheng, 2014-05-22 22:06:05]

LGTM

https://codereview.chromium.org/273513005/diff/290001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_render_message_filter.h (right):

https://codereview.chromium.org/273513005/diff/290001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.h:150: bool allowed);
Nit: remove this, since we no longer need it.

[Xi Han, 2014-05-23 13:13:25]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-05-23 13:13:44]

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

[commit-bot: I haz the power, 2014-05-23 15:54:38]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-23 15:59:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-23 15:59:16]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[Xi Han, 2014-05-23 16:25:58]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-05-23 16:26:29]

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

[Xi Han, 2014-05-23 17:51:28]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-05-23 17:52:41]

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

[commit-bot: I haz the power, 2014-05-23 22:03:30]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-24 01:38:01]

Change committed as 272676