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

chrome/renderer/worker_permission_client_proxy.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/common/render_messages.h"
 #include "chrome/renderer/worker_permission_client_proxy.h"
 #include "content/public/renderer/render_frame.h"
 #include "content/public/renderer/render_thread.h"
 #include "ipc/ipc_sync_message_filter.h"
+#include "third_party/WebKit/public/platform/WebPermissionCallbacks.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
 #include "third_party/WebKit/public/web/WebSecurityOrigin.h"
 
 WorkerPermissionClientProxy::WorkerPermissionClientProxy(
     content::RenderFrame* render_frame,
     blink::WebFrame* frame)
     : routing_id_(render_frame->GetRoutingID()),
       is_unique_origin_(false) {
   if (frame->document().securityOrigin().isUnique() ||
@@ skipping 24 matching lines @@
 bool WorkerPermissionClientProxy::allowFileSystem() {
   if (is_unique_origin_)
     return false;
 
   bool result = false;
   sync_message_filter_->Send(new ChromeViewHostMsg_AllowFileSystem(
       routing_id_, document_origin_url_, top_frame_origin_url_, &result));
   return result;
 }
 
+void WorkerPermissionClientProxy::requestFileSystemAccess(
+    const blink::WebPermissionCallbacks& callbacks) {
+  bool result = false;
+  sync_message_filter_->Send(new ChromeViewHostMsg_AllowFileSystem(

[kinuko, 2014/05/14 08:48:50]

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]

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:

+      routing_id_, document_origin_url_, top_frame_origin_url_, &result));
+  blink::WebPermissionCallbacks permission_callbacks(callbacks);
+  if (result) {
+    permission_callbacks.doAllow();
+    return;
+  }
+  permission_callbacks.doDeny();
+}
+
 bool WorkerPermissionClientProxy::allowIndexedDB(
     const blink::WebString& name) {
   if (is_unique_origin_)
     return false;
 
   bool result = false;
   sync_message_filter_->Send(new ChromeViewHostMsg_AllowIndexedDB(
       routing_id_, document_origin_url_, top_frame_origin_url_, name, &result));
   return result;
 }