Avoid sync IPCs for FileSystem API

Avoid sync IPCs for FileSystem API on the main thread

BUG=343382

[Fady Samuel, 2014-03-05 00:33:04]

It turns out there's a lot of complexity to making both worker threads and the
main thread use the same async IPC. Worker threads expect to block. Sync IPCs
accomplish exactly this. This CL uses an async IPC for the main thread, and the
existing sync IPC for workers.

[kinuko, 2014-03-05 08:35:43]

https://codereview.chromium.org/170733004/diff/100001/chrome/browser/content_...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/170733004/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:350: if
(!ChromeViewHostMsg_RequestFileSystemAccess::Read(&message, &param))
nit: I think you could directly read out params like:

int request_id;
GURL origin_url;
GURL top_origin_url;
....::Read(&message, &request_id, &origin_url, &top_origin_url);

https://codereview.chromium.org/170733004/diff/100001/chrome/common/render_me...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/170733004/diff/100001/chrome/common/render_me...
chrome/common/render_messages.h:372:
IPC_MESSAGE_ROUTED2(ChromeViewMsg_RequestFileSystemAccess_ACK,
nit: ACK? Since this looks more than just ack maybe this could be
RequestFileSystemAccessResponse ?

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

https://codereview.chromium.org/170733004/diff/100001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:273:
RegisterPermissionCallbacks(callbacks);
If you don't call this on worker thread you don't need to use thread-local map,
ContentSettingsObserver can just have a map as a member variable.  Also I think
in your case the map could be a simple std::map<int,
blink::WebPermissionCallback>, then no extra new/scoped_ptr<> would be
necessary.

https://codereview.chromium.org/170733004/diff/100001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:619: }
nit: no need of { } for one-line body

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

https://codereview.chromium.org/170733004/diff/100001/chrome/renderer/worker_...
chrome/renderer/worker_permission_client_proxy.cc:66: }
no need of { } for single-line body

[Fady Samuel, 2014-03-06 18:38:25]

ptal

https://codereview.chromium.org/170733004/diff/100001/chrome/browser/content_...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/170733004/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:350: if
(!ChromeViewHostMsg_RequestFileSystemAccess::Read(&message, &param))
On 2014/03/05 08:35:44, kinuko wrote:
> nit: I think you could directly read out params like:
> 
> int request_id;
> GURL origin_url;
> GURL top_origin_url;
> ....::Read(&message, &request_id, &origin_url, &top_origin_url);

Done.

https://codereview.chromium.org/170733004/diff/100001/chrome/common/render_me...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/170733004/diff/100001/chrome/common/render_me...
chrome/common/render_messages.h:372:
IPC_MESSAGE_ROUTED2(ChromeViewMsg_RequestFileSystemAccess_ACK,
On 2014/03/05 08:35:44, kinuko wrote:
> nit: ACK? Since this looks more than just ack maybe this could be
> RequestFileSystemAccessResponse ?

Done.

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

https://codereview.chromium.org/170733004/diff/100001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:273:
RegisterPermissionCallbacks(callbacks);
On 2014/03/05 08:35:44, kinuko wrote:
> If you don't call this on worker thread you don't need to use thread-local
map,
> ContentSettingsObserver can just have a map as a member variable.  Also I
think
> in your case the map could be a simple std::map<int,
> blink::WebPermissionCallback>, then no extra new/scoped_ptr<> would be
> necessary.

Done.

https://codereview.chromium.org/170733004/diff/100001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:619: }
On 2014/03/05 08:35:44, kinuko wrote:
> nit: no need of { } for one-line body

I prefer having them when the if statement has an else clause (I don't think if
we explicitly disallow this in the style guide). My reasoning is if we replace
the one line with a nested if in the future, the else clause doesn't
accidentally get attached to the wrong thing later. I replaced else with an
early exit.

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

https://codereview.chromium.org/170733004/diff/100001/chrome/renderer/worker_...
chrome/renderer/worker_permission_client_proxy.cc:66: }
On 2014/03/05 08:35:44, kinuko wrote:
> no need of { } for single-line body

Switched to early exit.

[kinuko, 2014-03-07 08:22:29]

lgtm with one nit

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

https://codereview.chromium.org/170733004/diff/140001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:282:
permission_requests_.erase(insert_result.first);
I feel this could be checked before you insert the callbacks to the internal
map?  Also I don't feel using insert_result here is particularly useful while
it's losing some readability. Probably this could be written w/o typing lengthy
type names like:

if (...unique) {
  callbacks.doDeny();
  return;
}

bool inserted = requests_.insert(id, callbacks).second;
DCHECK(inserted);

Send(...);

[jochen (gone - plz use gerrit), 2014-03-10 15:31:30]

https://codereview.chromium.org/170733004/diff/140001/chrome/browser/content_...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/170733004/diff/140001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:664:
IPC_MESSAGE_HANDLER_GENERIC(ChromeViewHostMsg_RequestFileSystemAccess,
this should be handled in chrome_render_message_filter instead