Avoid sync IPCs for FileSystem API

chrome/renderer/content_settings_observer.cc

 // Copyright (c) 2012 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/renderer/content_settings_observer.h"
 
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/render_messages.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/renderer/extensions/dispatcher.h"
+#include "chrome/renderer/permission_request_map.h"
 #include "content/public/renderer/document_state.h"
 #include "content/public/renderer/navigation_state.h"
 #include "content/public/renderer/render_frame.h"
 #include "content/public/renderer/render_view.h"
 #include "extensions/common/constants.h"
+#include "third_party/WebKit/public/platform/WebPermissionCallbacks.h"
 #include "third_party/WebKit/public/platform/WebURL.h"
 #include "third_party/WebKit/public/web/WebDataSource.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
 #include "third_party/WebKit/public/web/WebFrameClient.h"
 #include "third_party/WebKit/public/web/WebSecurityOrigin.h"
 #include "third_party/WebKit/public/web/WebView.h"
 #include "webkit/child/weburlresponse_extradata_impl.h"
 
 using blink::WebDataSource;
 using blink::WebDocument;
 using blink::WebFrame;
 using blink::WebFrameClient;
+using blink::WebPermissionCallbacks;
 using blink::WebSecurityOrigin;
 using blink::WebString;
 using blink::WebURL;
 using blink::WebView;
 using content::DocumentState;
 using content::NavigationState;
 using extensions::APIPermission;
 
 namespace {
 
@@ skipping 146 matching lines @@
 bool ContentSettingsObserver::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(ContentSettingsObserver, message)
     IPC_MESSAGE_HANDLER(ChromeViewMsg_SetAsInterstitial, OnSetAsInterstitial)
     IPC_MESSAGE_HANDLER(ChromeViewMsg_NPAPINotSupported, OnNPAPINotSupported)
     IPC_MESSAGE_HANDLER(ChromeViewMsg_SetAllowDisplayingInsecureContent,
                         OnSetAllowDisplayingInsecureContent)
     IPC_MESSAGE_HANDLER(ChromeViewMsg_SetAllowRunningInsecureContent,
                         OnSetAllowRunningInsecureContent)
     IPC_MESSAGE_HANDLER(ChromeViewMsg_ReloadFrame, OnReloadFrame);
+    IPC_MESSAGE_HANDLER(ChromeViewMsg_RequestFileSystemAccess_ACK,
+                        OnRequestFileSystemAccessACK)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   if (handled)
     return true;
 
   // Don't swallow LoadBlockedPlugins messages, as they're sent to every
   // blocked plugin.
   IPC_BEGIN_MESSAGE_MAP(ContentSettingsObserver, message)
     IPC_MESSAGE_HANDLER(ChromeViewMsg_LoadBlockedPlugins, OnLoadBlockedPlugins)
   IPC_END_MESSAGE_MAP()
@@ skipping 47 matching lines @@
       frame->top()->document().securityOrigin().isUnique())
     return false;
 
   bool result = false;
   Send(new ChromeViewHostMsg_AllowFileSystem(
       routing_id(), GURL(frame->document().securityOrigin().toString()),
       GURL(frame->top()->document().securityOrigin().toString()), &result));
   return result;
 }
 
+void ContentSettingsObserver::requestFileSystemAccess(
+    WebFrame* frame, const WebPermissionCallbacks& callbacks) {
+  int request_id =
+      content::PermissionRequestMap::GetOrCreate()->
+          RegisterPermissionCallbacks(callbacks);

[kinuko, 2014/03/05 08:35:44]

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.

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

Done.

+
+  Send(new ChromeViewHostMsg_RequestFileSystemAccess(
+      routing_id(),
+      request_id,
+      GURL(frame->document().securityOrigin().toString()),
+      GURL(frame->top()->document().securityOrigin().toString())));
+}
+
 bool ContentSettingsObserver::allowImage(WebFrame* frame,
                                          bool enabled_per_settings,
                                          const WebURL& image_url) {
   bool allow = enabled_per_settings;
   if (enabled_per_settings) {
     if (is_interstitial_page_)
       return true;
     if (IsWhitelistedForContentSettings(frame))
       return true;
 
@@ skipping 308 matching lines @@
   allow_running_insecure_content_ = allow;
   OnSetAllowDisplayingInsecureContent(allow);
 }
 
 void ContentSettingsObserver::OnReloadFrame() {
   DCHECK(!render_frame()->GetWebFrame()->parent()) <<
       "Should only be called on the main frame";
   render_frame()->GetWebFrame()->reload();
 }
 
+void ContentSettingsObserver::OnRequestFileSystemAccessACK(int request_id,
+                                                           bool allowed) {
+  scoped_ptr<WebPermissionCallbacks> callbacks(
+      content::PermissionRequestMap::Get()->GetAndUnregisterCallbacks(
+          request_id));
+  if (allowed) {
+    callbacks->doAllow();
+  } else {
+    callbacks->doDeny();
+  }

[kinuko, 2014/03/05 08:35:44]

nit: no need of { } for one-line body

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

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.

+}
+
 void ContentSettingsObserver::ClearBlockedContentSettings() {
   for (size_t i = 0; i < arraysize(content_blocked_); ++i)
     content_blocked_[i] = false;
   cached_storage_permissions_.clear();
   cached_script_permissions_.clear();
 }
 
 const extensions::Extension* ContentSettingsObserver::GetExtension(
     const WebSecurityOrigin& origin) const {
   if (!EqualsASCII(origin.protocol(), extensions::kExtensionScheme))
@@ skipping 47 matching lines @@
 
   // If the scheme is file:, an empty file name indicates a directory listing,
   // which requires JavaScript to function properly.
   if (EqualsASCII(origin.protocol(), content::kFileScheme)) {
     return document_url.SchemeIs(content::kFileScheme) &&
            document_url.ExtractFileName().empty();
   }
 
   return false;
 }