WebUIDataSource: Add ability to query whether a URL is handled by a request filter.

Before this CL
WebUIDataSource::SetRequestFilter() accepted a single callback function as a parameter.
That callback was responsible for
 - communicating via a boolean whether a given URL will be handled.
 - producing the response (bytes) for this request.

This is inflexible, because there are cases where we need to know whether a URL
will be handled by the request filter, without triggering the request yet. This is in
preparation of simplifying the mechanism that WebUIDataSourceImpl uses to determine
when a WebUI resource is gzipped.

After this CL:
WebUIDataSource::SetRequestFilter() accepts two callback functions.
 - |should_handle_request_callback| simply returns a boolean, indicating whether the
   request will be handled by the request filter, without handling the request.
 - |handle_request_callback| handles the request, without returning any boolean.

TBR=jam@chromium.org

Bug: 738243
Change-Id: I915b87801908885bcbf05e785bbacf8ad140c481
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1535164
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: calamity <calamity@chromium.org>
Reviewed-by: Dan Beam <dbeam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#649289}

chrome/browser/accessibility/accessibility_ui.cc

@@ -132,12 +132,15 @@ std::unique_ptr<base::DictionaryValue> BuildTargetDescriptor(Browser* browser) {
 }
 #endif  // !defined(OS_ANDROID)
 
-bool HandleAccessibilityRequestCallback(
+bool ShouldHandleAccessibilityRequestCallback(const std::string& path) {
+  return path == kTargetsDataFile;
+}
+
+void HandleAccessibilityRequestCallback(
     content::BrowserContext* current_context,
     const std::string& path,
     const content::WebUIDataSource::GotDataCallback& callback) {
-  if (path != kTargetsDataFile)
-    return false;
+  DCHECK(ShouldHandleAccessibilityRequestCallback(path));
 
   base::DictionaryValue data;
   PrefService* pref = Profile::FromBrowserContext(current_context)->GetPrefs();
@@ -224,7 +227,6 @@ bool HandleAccessibilityRequestCallback(
   base::JSONWriter::Write(data, &json_string);
 
   callback.Run(base::RefCountedString::TakeString(&json_string));
-  return true;
 }
 
 std::string RecursiveDumpAXPlatformNodeAsString(ui::AXPlatformNode* node,
@@ -256,6 +258,7 @@ AccessibilityUI::AccessibilityUI(content::WebUI* web_ui)
   html_source->AddResourcePath("accessibility.js", IDR_ACCESSIBILITY_JS);
   html_source->SetDefaultResource(IDR_ACCESSIBILITY_HTML);
   html_source->SetRequestFilter(
+      base::BindRepeating(&ShouldHandleAccessibilityRequestCallback),
       base::Bind(&HandleAccessibilityRequestCallback,
                  web_ui->GetWebContents()->GetBrowserContext()));
 

chrome/browser/ui/webui/chromeos/login/oobe_ui.cc

@@ -261,7 +261,8 @@ content::WebUIDataSource* CreateOobeUIDataSource(
   const bool is_running_test = command_line->HasSwitch(::switches::kTestName) ||
                                command_line->HasSwitch(::switches::kTestType);
   if (is_running_test)
-    source->SetRequestFilter(::test::GetTestFilesRequestFilter());
+    source->SetRequestFilter(::test::GetTestShouldHandleRequest(),
+                             ::test::GetTestFilesRequestFilter());
 
   return source;
 }

chrome/browser/ui/webui/print_preview/print_preview_ui.cc

@@ -131,28 +131,32 @@ base::LazyInstance<PrintPreviewRequestIdMapWithLock>::DestructorAtExit
 base::LazyInstance<base::IDMap<PrintPreviewUI*>>::DestructorAtExit
     g_print_preview_ui_id_map = LAZY_INSTANCE_INITIALIZER;
 
+bool ShouldHandleRequestCallback(const std::string& path) {
+  // ChromeWebUIDataSource handles most requests except for the print preview
+  // data.
+  return PrintPreviewUI::ParseDataPath(path, nullptr, nullptr);
+}
+
 // Get markup or other resources for the print preview page.
-bool HandleRequestCallback(
+void HandleRequestCallback(
     const std::string& path,
     const content::WebUIDataSource::GotDataCallback& callback) {
   // ChromeWebUIDataSource handles most requests except for the print preview
   // data.
   int preview_ui_id;
   int page_index;
-  if (!PrintPreviewUI::ParseDataPath(path, &preview_ui_id, &page_index))
-    return false;
+  CHECK(PrintPreviewUI::ParseDataPath(path, &preview_ui_id, &page_index));
 
   scoped_refptr<base::RefCountedMemory> data;
   PrintPreviewDataService::GetInstance()->GetDataEntry(preview_ui_id,
                                                        page_index, &data);
   if (data.get()) {
     callback.Run(data.get());
-    return true;
+    return;
   }
   // Invalid request.
   auto empty_bytes = base::MakeRefCounted<base::RefCountedBytes>();
   callback.Run(empty_bytes.get());
-  return true;
 }
 
 void AddPrintPreviewStrings(content::WebUIDataSource* source) {
@@ -416,7 +420,8 @@ std::vector<std::string> SetupPrintPreviewPlugin(
     source->AddResourcePath(resource.path, resource.id);
   }
 
-  source->SetRequestFilter(base::BindRepeating(&HandleRequestCallback));
+  source->SetRequestFilter(base::BindRepeating(&ShouldHandleRequestCallback),
+                           base::BindRepeating(&HandleRequestCallback));
   source->OverrideContentSecurityPolicyChildSrc("child-src 'self';");
   source->DisableDenyXFrameOptions();
   source->OverrideContentSecurityPolicyObjectSrc("object-src 'self';");

chrome/browser/ui/webui/signin/inline_login_ui.cc

@@ -46,8 +46,10 @@ content::WebUIDataSource* CreateWebUIDataSource() {
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
   const bool is_running_test = command_line->HasSwitch(::switches::kTestName) ||
                                command_line->HasSwitch(::switches::kTestType);
-  if (is_running_test)
-    source->SetRequestFilter(test::GetTestFilesRequestFilter());
+  if (is_running_test) {
+    source->SetRequestFilter(test::GetTestShouldHandleRequest(),
+                             test::GetTestFilesRequestFilter());
+  }
 
   source->AddResourcePath("inline_login.css", IDR_INLINE_LOGIN_CSS);
   source->AddResourcePath("inline_login.js", IDR_INLINE_LOGIN_JS);

chrome/browser/ui/webui/test_files_request_filter.cc

@@ -14,27 +14,37 @@
 
 namespace {
 
-bool HandleTestFileRequestCallback(
-    const std::string& path,
-    const content::WebUIDataSource::GotDataCallback& callback) {
-  base::ScopedAllowBlockingForTesting allow_blocking;
+bool ShouldHandleTestFileRequestCallback(const std::string& path) {
   std::vector<std::string> url_substr =
       base::SplitString(path, "/", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
   if (url_substr.size() != 2 || url_substr[0] != "test")
     return false;
 
+  base::ScopedAllowBlockingForTesting allow_blocking;
+  base::FilePath test_data_dir;
+  base::PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);
+  return base::PathExists(
+      test_data_dir.AppendASCII("webui").AppendASCII(url_substr[1]));
+}
+
+void HandleTestFileRequestCallback(
+    const std::string& path,
+    const content::WebUIDataSource::GotDataCallback& callback) {
+  DCHECK(ShouldHandleTestFileRequestCallback(path));
+  base::ScopedAllowBlockingForTesting allow_blocking;
+
+  std::vector<std::string> url_substr =
+      base::SplitString(path, "/", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
   std::string contents;
   base::FilePath test_data_dir;
   base::PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);
-  if (!base::ReadFileToString(
-          test_data_dir.AppendASCII("webui").AppendASCII(url_substr[1]),
-          &contents))
-    return false;
+  CHECK(base::ReadFileToString(
+      test_data_dir.AppendASCII("webui").AppendASCII(url_substr[1]),
+      &contents));
 
   base::RefCountedString* ref_contents = new base::RefCountedString();
   ref_contents->data() = contents;
   callback.Run(ref_contents);
-  return true;
 }
 
 }  // namespace
@@ -45,4 +55,9 @@ content::WebUIDataSource::HandleRequestCallback GetTestFilesRequestFilter() {
   return base::Bind(&HandleTestFileRequestCallback);
 }
 
+content::WebUIDataSource::ShouldHandleRequestCallback
+GetTestShouldHandleRequest() {
+  return base::BindRepeating(&ShouldHandleTestFileRequestCallback);
+}
+
 }  // namespace test

chrome/browser/ui/webui/test_files_request_filter.h

@@ -14,6 +14,10 @@ namespace test {
 // request path has "/test/<filename>" format.
 content::WebUIDataSource::HandleRequestCallback GetTestFilesRequestFilter();
 
+// Returns a callback indicating which requests should be handled by the filter.
+content::WebUIDataSource::ShouldHandleRequestCallback
+GetTestShouldHandleRequest();
+
 }  // namespace test
 
 #endif  // CHROME_BROWSER_UI_WEBUI_TEST_FILES_REQUEST_FILTER_H_

chrome/browser/ui/webui/welcome/welcome_ui.cc

@@ -49,10 +49,8 @@ const bool kIsBranded =
 
 const char kPreviewBackgroundPath[] = "preview-background.jpg";
 
-bool HandleRequestCallback(
-    base::WeakPtr<WelcomeUI> weak_ptr,
-    const std::string& path,
-    const content::WebUIDataSource::GotDataCallback& callback) {
+bool ShouldHandleRequestCallback(base::WeakPtr<WelcomeUI> weak_ptr,
+                                 const std::string& path) {
   if (!base::StartsWith(path, kPreviewBackgroundPath,
                         base::CompareCase::SENSITIVE)) {
     return false;
@@ -65,12 +63,22 @@ bool HandleRequestCallback(
     return false;
   }
 
-  if (weak_ptr) {
-    weak_ptr->CreateBackgroundFetcher(background_index, callback);
-    return true;
-  }
+  return !weak_ptr ? false : true;
+}
+
+void HandleRequestCallback(
+    base::WeakPtr<WelcomeUI> weak_ptr,
+    const std::string& path,
+    const content::WebUIDataSource::GotDataCallback& callback) {
+  DCHECK(ShouldHandleRequestCallback(weak_ptr, path));
+
+  std::string index_param = path.substr(path.find_first_of("?") + 1);
+  int background_index = -1;
+  CHECK(base::StringToInt(index_param, &background_index) ||
+        background_index < 0);
 
-  return false;
+  DCHECK(weak_ptr);
+  weak_ptr->CreateBackgroundFetcher(background_index, callback);
 }
 
 void AddOnboardingStrings(content::WebUIDataSource* html_source) {
@@ -211,8 +219,11 @@ WelcomeUI::WelcomeUI(content::WebUI* web_ui, const GURL& url)
                            nux::GetNuxOnboardingModules(profile)
                                .FindKey("returning-user")
                                ->GetString());
-    html_source->SetRequestFilter(base::BindRepeating(
-        &HandleRequestCallback, weak_ptr_factory_.GetWeakPtr()));
+    html_source->SetRequestFilter(
+        base::BindRepeating(&ShouldHandleRequestCallback,
+                            weak_ptr_factory_.GetWeakPtr()),
+        base::BindRepeating(&HandleRequestCallback,
+                            weak_ptr_factory_.GetWeakPtr()));
     html_source->UseGzip(base::BindRepeating(&WelcomeUI::IsGzipped));
   } else if (kIsBranded &&
              AccountConsistencyModeManager::IsDiceEnabledForProfile(profile)) {

chrome/browser/web_applications/bookmark_apps/system_web_app_manager_browsertest.cc

@@ -67,21 +67,24 @@ class TestWebUIController : public content::WebUIController {
     content::WebUIDataSource* data_source =
         content::WebUIDataSource::Create("test-system-app");
     data_source->AddResourcePath("icon-256.png", IDR_PRODUCT_LOGO_256);
-    data_source->SetRequestFilter(base::BindRepeating(
-        [](const std::string& id,
-           const content::WebUIDataSource::GotDataCallback& callback) {
-          scoped_refptr<base::RefCountedString> ref_contents(
-              new base::RefCountedString);
-          if (id == "manifest.json")
-            ref_contents->data() = kSystemAppManifestText;
-          else if (id == "pwa.html")
-            ref_contents->data() = kPwaHtml;
-          else
-            return false;
-
-          callback.Run(ref_contents);
-          return true;
-        }));
+    data_source->SetRequestFilter(
+        base::BindRepeating([](const std::string& path) {
+          return path == "manifest.json" || path == "pwa.html";
+        }),
+        base::BindRepeating(
+            [](const std::string& id,
+               const content::WebUIDataSource::GotDataCallback& callback) {
+              scoped_refptr<base::RefCountedString> ref_contents(
+                  new base::RefCountedString);
+              if (id == "manifest.json")
+                ref_contents->data() = kSystemAppManifestText;
+              else if (id == "pwa.html")
+                ref_contents->data() = kPwaHtml;
+              else
+                NOTREACHED();
+
+              callback.Run(ref_contents);
+            }));
     content::WebUIDataSource::Add(web_ui->GetWebContents()->GetBrowserContext(),
                                   data_source);
   }

content/browser/net/network_errors_listing_ui.cc

@@ -57,19 +57,21 @@ std::unique_ptr<base::ListValue> GetNetworkErrorData() {
   return error_list;
 }
 
-bool HandleWebUIRequestCallback(
+bool ShouldHandleWebUIRequestCallback(const std::string& path) {
+  return path == kNetworkErrorDataFile;
+}
+
+void HandleWebUIRequestCallback(
     BrowserContext* current_context,
     const std::string& path,
     const WebUIDataSource::GotDataCallback& callback) {
-  if (path != kNetworkErrorDataFile)
-    return false;
+  DCHECK(ShouldHandleWebUIRequestCallback(path));
 
   base::DictionaryValue data;
   data.Set(kErrorCodesDataName, GetNetworkErrorData());
   std::string json_string;
   base::JSONWriter::Write(data, &json_string);
   callback.Run(base::RefCountedString::TakeString(&json_string));
-  return true;
 }
 
 } // namespace
@@ -88,6 +90,7 @@ NetworkErrorsListingUI::NetworkErrorsListingUI(WebUI* web_ui)
                                IDR_NETWORK_ERROR_LISTING_JS);
   html_source->SetDefaultResource(IDR_NETWORK_ERROR_LISTING_HTML);
   html_source->SetRequestFilter(
+      base::BindRepeating(&ShouldHandleWebUIRequestCallback),
       base::Bind(&HandleWebUIRequestCallback,
                  web_ui->GetWebContents()->GetBrowserContext()));
 

content/browser/tracing/tracing_ui.cc

@@ -139,16 +139,17 @@ bool OnBeginJSONRequest(const std::string& path,
   return false;
 }
 
-bool OnTracingRequest(const std::string& path,
+bool OnShouldHandleRequest(const std::string& path) {
+  return base::StartsWith(path, "json/", base::CompareCase::SENSITIVE);
+}
+
+void OnTracingRequest(const std::string& path,
                       const WebUIDataSource::GotDataCallback& callback) {
-  if (base::StartsWith(path, "json/", base::CompareCase::SENSITIVE)) {
-    if (!OnBeginJSONRequest(path, callback)) {
-      std::string error("##ERROR##");
-      callback.Run(base::RefCountedString::TakeString(&error));
-    }
-    return true;
+  DCHECK(OnShouldHandleRequest(path));
+  if (!OnBeginJSONRequest(path, callback)) {
+    std::string error("##ERROR##");
+    callback.Run(base::RefCountedString::TakeString(&error));
   }
-  return false;
 }
 
 }  // namespace
@@ -185,7 +186,8 @@ TracingUI::TracingUI(WebUI* web_ui)
   source->SetJsonPath("strings.js");
   source->SetDefaultResource(IDR_TRACING_HTML);
   source->AddResourcePath("tracing.js", IDR_TRACING_JS);
-  source->SetRequestFilter(base::Bind(OnTracingRequest));
+  source->SetRequestFilter(base::BindRepeating(OnShouldHandleRequest),
+                           base::BindRepeating(OnTracingRequest));
   WebUIDataSource::Add(browser_context, source);
   TracingControllerImpl::GetInstance()->RegisterTracingUI(this);
 }

content/browser/webui/web_ui_data_source_impl.cc

@@ -181,8 +181,10 @@ void WebUIDataSourceImpl::SetDefaultResource(int resource_id) {
 }
 
 void WebUIDataSourceImpl::SetRequestFilter(
-    const WebUIDataSource::HandleRequestCallback& callback) {
-  filter_callback_ = callback;
+    const ShouldHandleRequestCallback& should_handle_request_callback,
+    const HandleRequestCallback& handle_request_callback) {
+  should_handle_request_callback_ = should_handle_request_callback;
+  filter_callback_ = handle_request_callback;
 }
 
 void WebUIDataSourceImpl::DisableReplaceExistingSource() {
@@ -280,8 +282,9 @@ void WebUIDataSourceImpl::StartDataRequest(
     const std::string& path,
     const ResourceRequestInfo::WebContentsGetter& wc_getter,
     const URLDataSource::GotDataCallback& callback) {
-  if (!filter_callback_.is_null() &&
-      filter_callback_.Run(path, callback)) {
+  if (!should_handle_request_callback_.is_null() &&
+      should_handle_request_callback_.Run(path)) {
+    filter_callback_.Run(path, callback);
     return;
   }
 
@@ -319,6 +322,14 @@ bool WebUIDataSourceImpl::IsGzipped(const std::string& path) const {
   if (!use_gzip_)
     return false;
 
+  // Note: In the hypothetical case of requests handled by |filter_callback_|
+  // that involve gzipped data, the callback itself is responsible for
+  // ungzipping, and IsGzipped will return false for such cases.
+  if (!should_handle_request_callback_.is_null() &&
+      should_handle_request_callback_.Run(path)) {
+    return false;
+  }
+
   // TODO(dbeam): does anybody care about the "dirty" path (i.e. stuff after ?).
   const std::string clean_path = CleanUpPath(path);
   if (!json_path_.empty() && clean_path == json_path_)

content/browser/webui/web_ui_data_source_impl.h

@@ -40,8 +40,10 @@ class CONTENT_EXPORT WebUIDataSourceImpl : public URLDataSourceImpl,
   void SetJsonPath(base::StringPiece path) override;
   void AddResourcePath(base::StringPiece path, int resource_id) override;
   void SetDefaultResource(int resource_id) override;
-  void SetRequestFilter(
-      const WebUIDataSource::HandleRequestCallback& callback) override;
+  void SetRequestFilter(const WebUIDataSource::ShouldHandleRequestCallback&
+                            should_handle_request_callback,
+                        const WebUIDataSource::HandleRequestCallback&
+                            handle_request_callback) override;
   void DisableReplaceExistingSource() override;
   void DisableContentSecurityPolicy() override;
   void OverrideContentSecurityPolicyScriptSrc(const std::string& data) override;
@@ -112,6 +114,8 @@ class CONTENT_EXPORT WebUIDataSourceImpl : public URLDataSourceImpl,
   // to |load_time_flags_| if the usage is reduced to storing flags only).
   base::DictionaryValue localized_strings_;
   WebUIDataSource::HandleRequestCallback filter_callback_;
+  WebUIDataSource::ShouldHandleRequestCallback should_handle_request_callback_;
+
   bool add_csp_;
   bool script_src_set_;
   std::string script_src_;

content/browser/webui/web_ui_data_source_unittest.cc

@@ -65,10 +65,9 @@ class WebUIDataSourceTest : public testing::Test {
     return source_->GetMimeType(path);
   }
 
-  bool HandleRequest(const std::string& path,
+  void HandleRequest(const std::string& path,
                      const WebUIDataSourceImpl::GotDataCallback&) {
     request_path_ = path;
-    return true;
   }
 
   void RequestFilterQueryStringCallback(
@@ -180,6 +179,7 @@ void WebUIDataSourceTest::RequestFilterQueryStringCallback(
 TEST_F(WebUIDataSourceTest, RequestFilterQueryString) {
   request_path_ = std::string();
   source()->SetRequestFilter(
+      base::BindRepeating([](const std::string& path) { return true; }),
       base::Bind(&WebUIDataSourceTest::HandleRequest, base::Unretained(this)));
   source()->SetDefaultResource(kDummyDefaultResourceId);
   source()->AddResourcePath("foobar", kDummyResourceId);

content/browser/webui/web_ui_mojo_browsertest.cc

@@ -59,7 +59,7 @@ base::FilePath GetFilePathForJSResource(const std::string& path) {
 
 // The bindings for the page are generated from a .mojom file. This code looks
 // up the generated file from disk and returns it.
-bool GetResource(const std::string& id,
+void GetResource(const std::string& id,
                  const WebUIDataSource::GotDataCallback& callback) {
   base::ScopedAllowBlockingForTesting allow_blocking;
 
@@ -77,7 +77,6 @@ bool GetResource(const std::string& id,
   base::RefCountedString* ref_contents = new base::RefCountedString;
   ref_contents->data() = contents;
   callback.Run(ref_contents);
-  return true;
 }
 
 class BrowserTargetImpl : public mojom::BrowserTarget {
@@ -113,18 +112,21 @@ class TestWebUIController : public WebUIController {
     web_ui->SetBindings(bindings);
     {
       WebUIDataSource* data_source = WebUIDataSource::Create("mojo-web-ui");
-      data_source->SetRequestFilter(base::BindRepeating(&GetResource));
+      data_source->SetRequestFilter(
+          base::BindRepeating([](const std::string& path) { return true; }),
+          base::BindRepeating(&GetResource));
       WebUIDataSource::Add(web_ui->GetWebContents()->GetBrowserContext(),
                            data_source);
     }
     {
       WebUIDataSource* data_source = WebUIDataSource::Create("dummy-web-ui");
-      data_source->SetRequestFilter(base::BindRepeating(
-          [](const std::string& id,
-             const WebUIDataSource::GotDataCallback& callback) {
-            callback.Run(new base::RefCountedString);
-            return true;
-          }));
+      data_source->SetRequestFilter(
+          base::BindRepeating([](const std::string& path) { return true; }),
+          base::BindRepeating(
+              [](const std::string& id,
+                 const WebUIDataSource::GotDataCallback& callback) {
+                callback.Run(new base::RefCountedString);
+              }));
       WebUIDataSource::Add(web_ui->GetWebContents()->GetBrowserContext(),
                            data_source);
     }

content/public/browser/web_ui_data_source.h

@@ -83,14 +83,24 @@ class WebUIDataSource {
       GotDataCallback;
 
   // Used by SetRequestFilter. The string parameter is the path of the request.
-  // If the callee doesn't want to handle the data, false is returned. Otherwise
-  // true is returned and the GotDataCallback parameter is called either then or
-  // asynchronously with the response.
-  typedef base::Callback<bool(const std::string&, const GotDataCallback&)>
+  // The return value indicates if the callee wants to handle the request. Iff
+  // true is returned, |handle_request_callback| will be called to provide the
+  // request's response.
+  typedef base::RepeatingCallback<bool(const std::string&)>
+      ShouldHandleRequestCallback;
+
+  // Used by SetRequestFilter. The string parameter is the path of the request.
+  // This callback is only called if a prior call to ShouldHandleRequestCallback
+  // returned true. GotDataCallback should be used to provide the response
+  // bytes.
+  typedef base::RepeatingCallback<void(const std::string&,
+                                       const GotDataCallback&)>
       HandleRequestCallback;
 
   // Allows a caller to add a filter for URL requests.
-  virtual void SetRequestFilter(const HandleRequestCallback& callback) = 0;
+  virtual void SetRequestFilter(
+      const ShouldHandleRequestCallback& should_handle_request_callback,
+      const HandleRequestCallback& handle_request_callback) = 0;
 
   // The following map to methods on URLDataSource. See the documentation there.
   // NOTE: it's not acceptable to call DisableContentSecurityPolicy for new