Enable <webview>.executeScript outside of Apps and Extensions [2]

extensions/browser/api/guest_view/web_view/web_view_internal_api.cc

Index: extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
diff --git a/extensions/browser/api/guest_view/web_view/web_view_internal_api.cc b/extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
index c55733559faf6d05473f668707230c2e577cd733..ad7155c8e49b4a53c4480af491e9f0809d1b04b1 100644
--- a/extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
+++ b/extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
@@ -6,17 +6,23 @@
 
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/stop_find_action.h"
+#include "content/public/common/url_fetcher.h"
 #include "extensions/common/api/web_view_internal.h"
+#include "net/base/load_flags.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_fetcher_delegate.h"
 #include "third_party/WebKit/public/web/WebFindOptions.h"
 
 using content::WebContents;
 using extensions::core_api::web_view_internal::SetPermission::Params;
 using extensions::core_api::extension_types::InjectDetails;
+using net::URLFetcher;
 namespace webview = extensions::core_api::web_view_internal;
 
 namespace {
@@ -49,6 +55,61 @@ int MaskForKey(const char* key) {
 
 namespace extensions {
 
+// WebUIURLFetcher downloads the content of a file by giving its |url| on WebUI.
+// Each WebUIURLFetcher is associated with a given |render_process_id,
+// render_view_id| pair.
+class WebViewInternalExecuteCodeFunction::WebUIURLFetcher
+    : public net::URLFetcherDelegate {
+ public:
+  WebUIURLFetcher(content::BrowserContext* context,
+                  const GURL& url,
+                  int render_process_id,
+                  int render_view_id,
+                  const ExecuteCodeFunction::WebUILoadFileCallback& callback)
+      : context_(context),
+        url_(url),
+        render_process_id_(render_process_id),
+        render_view_id_(render_view_id),
+        callback_(callback) {}
+  ~WebUIURLFetcher() override {}
+
+  void Start() {
+    fetcher_.reset(URLFetcher::Create(url_, URLFetcher::GET, this));
+    fetcher_->SetRequestContext(context_->GetRequestContext());
+    fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES);
+
+    content::AssociateURLFetcherWithRenderFrame(
+        fetcher_.get(), url_, render_process_id_, render_view_id_);
+    fetcher_->Start();
+  }
+
+ private:
+  // net::URLFetcherDelegate:
+  void OnURLFetchComplete(const URLFetcher* source) override {
+    CHECK_EQ(fetcher_.get(), source);
+
+    std::string data;
+    if (fetcher_->GetStatus().status() != net::URLRequestStatus::SUCCESS ||
+        fetcher_->GetResponseCode() != 200) {
+      callback_.Run(false, data);

[Devlin, 2015/03/19 15:45:35]

I think it'd be nice to rewrite this as:

std::string data;
bool result = false;
if (fetcher_->GetStatus().status() == net::URLRequestStatus::SUCCESS &&
    fetcher_->GetResponseCode() == 200) {
  result = fetcher_->GetResponseAsString(&data);
  DCHECK(result);
}

callback_.Run(result, data);

I think it's a little clearer, and also a few less lines of code.

[Devlin, 2015/03/19 15:45:35]

I think it'd be nice to rewrite this as:

std::string data;
bool result = false;
if (fetcher_->GetStatus().status() == net::URLRequestStatus::SUCCESS &&
    fetcher_->GetResponseCode() == 200) {
  result = fetcher_->GetResponseAsString(&data);
  DCHECK(result);
}

callback_.Run(result, data);

I think it's a little clearer, and also a few less lines of code.

[Xi Han, 2015/03/19 19:53:15]

Updated.

[asargent_no_longer_on_chrome, 2015/03/19 20:24:38]

Random drive-by: In the past we found bugs in extension code when we were
explicitly checking for an http status code of 200, and considering it a failure
when we got a perfectly valid and usable response from the cache with a status
code of 304. 

Since this is a special URL it might not matter since it might not be something
that is cacheable anyway, but as a general rule you should be wary of assuming
that 200 is the only way a request can succeed and have usable results for you.
Often just checking for URLRequestStatus::SUCCESS is sufficient. 

[Xi Han, 2015/03/19 21:05:14]

That is a good point. Also I fount out URLRequestChromeJob::GetResponseCode()is
always return ok(200), so it doesn't matter whether to check the
GetResponseCode() here or not.

+      return;
+    }
+
+    bool result = fetcher_->GetResponseAsString(&data);
+    DCHECK(result) << "Invalid fetcher setting.";
+    callback_.Run(true, data);
+  }
+
+  content::BrowserContext* context_;
+  GURL url_;
+  const int render_process_id_;

[Devlin, 2015/03/19 15:45:35]

Is there a reason to store all these as member variables, instead of passing
them into Start()?

[Xi Han, 2015/03/19 19:53:15]

There is no special reason, just makes the start() more clear. I am ok with
passing them in start(). Updated.

+  const int render_view_id_;
+  const ExecuteCodeFunction::WebUILoadFileCallback callback_;
+  scoped_ptr<URLFetcher> fetcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(WebUIURLFetcher);
+};
+
 bool WebViewInternalExtensionFunction::RunAsync() {
   int instance_id = 0;
   EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(0, &instance_id));
@@ -145,6 +206,58 @@ const GURL& WebViewInternalExecuteCodeFunction::GetWebViewSrc() const {
   return guest_src_;
 }
 
+bool WebViewInternalExecuteCodeFunction::LoadFileForWebUI(
+    const std::string& file_src,
+    const WebUILoadFileCallback& callback) {
+  if (!render_view_host() || !render_view_host()->GetProcess())
+    return false;
+  WebViewGuest* guest = WebViewGuest::From(
+      render_view_host()->GetProcess()->GetID(), guest_instance_id_);
+  if (!guest || host_id().type() != HostID::WEBUI)
+    return false;
+
+  GURL owner_base_url(guest->GetOwnerSiteURL().GetWithEmptyPath());
+  GURL file_url(owner_base_url.Resolve(file_src));
+
+  scoped_ptr<WebUIURLFetcher> fetcher(
+      new WebUIURLFetcher(this->browser_context(), file_url,
+                          render_view_host()->GetProcess()->GetID(),
+                          render_view_host()->GetRoutingID(), callback));
+  fetcher->Start();
+  fetcher_.reset(fetcher.release());  // Pass ownership to |fetcher_|.

[Devlin, 2015/03/19 15:45:35]

... Why not just do fetcher_.reset(new WebUIUrlFetcher())?

[Xi Han, 2015/03/19 19:53:15]

I also find that the old code causes memory leak. Updated.

+  return true;
+}
+
+bool WebViewInternalExecuteCodeFunction::LoadFile(const std::string& file) {
+  if (!extension()) {
+    bool is_success = false;
+    is_success = LoadFileForWebUI(
+        *details_->file,
+        base::Bind(&WebViewInternalExecuteCodeFunction::DidLoadFileForWebUI,
+                   this));
+    if (!is_success) {

[Devlin, 2015/03/19 15:45:35]

inline is_success

[Xi Han, 2015/03/19 19:53:15]

Done.

+      SendResponse(false);

[Devlin, 2015/03/19 15:45:35]

Set an error?

[Xi Han, 2015/03/19 19:53:14]

Done.

+      return false;
+    }
+    // Will finish asynchronously.
+    return true;
+  }
+  return ExecuteCodeFunction::LoadFile(file);
+}
+
+void WebViewInternalExecuteCodeFunction::DidLoadFileForWebUI(
+    bool success,
+    const std::string& data) {
+  if (success) {
+    if (!base::IsStringUTF8(data))
+      SendResponse(false);

[Devlin, 2015/03/19 15:45:36]

Set errors.

[Xi Han, 2015/03/19 19:53:15]

Done.

+    else if (!Execute(data))
+      SendResponse(false);
+  } else {
+    SendResponse(false);
+  }
+}
+
 WebViewInternalExecuteScriptFunction::WebViewInternalExecuteScriptFunction() {
 }