"Redirecting URLs to Packaged Apps" implementation: revised

chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc

Index: chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
diff --git a/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc b/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
index 266d8e1b9ed2def0d49ea851cea7b0bc2193f9df..ca87d1cc3fe4421e7ca9f698e54eff62d3af5072 100644
--- a/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
+++ b/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
@@ -72,6 +72,10 @@
 #include "chrome/browser/renderer_host/offline_resource_throttle.h"
 #endif
 
+#if !defined(OS_ANDROID) && !defined(OS_IOS)
+#include "chrome/browser/apps/app_url_redirector.h"
+#endif
+
 using content::BrowserThread;
 using content::RenderViewHost;
 using content::ResourceDispatcherHostLoginDelegate;
@@ -262,12 +266,22 @@ void ChromeResourceDispatcherHostDelegate::RequestBeginning(
     request->SetPriority(net::IDLE);
   }
 
+#if !defined(OS_ANDROID) && !defined(OS_IOS)
+  // Redirect some navigations to apps that have registered matching URL
+  // handlers ('url_handlers' in the manifest).
+  if (!is_prerendering && resource_type == ResourceType::MAIN_FRAME &&

[darin (slow to review), 2013/09/05 23:54:25]

I don't think this file should be built in the OS_IOS build. Have you found
otherwise?

Wouldn't it be better to share the first two checks with OS_ANDROID?

  if (!is_prerendering && resource_type == ResourceType::MAIN_FRAME) {
#if defined(OS_ANDROID)
    ...
#else
    if (request->method() == "GET")
      throttles->push_back(AppUrlRedirector::CreateThrottleFor(request));
#endif
  }

^^^ Seems better to test for GET instead of POST. Since you are only filtering
on navigations it doesn't strictly matter, but in case some other kind of HTTP
method ends up being used for navigations, it might be nice to not accidentally
route those as well to the app.

[sergeygs, 2013/09/06 02:51:14]

Done. I guess you're right about this file not build for OS_IOS.

+      request->method() != "POST") {
+    throttles->push_back(AppUrlRedirector::CreateThrottleFor(request));

[not at google - send to devlin, 2013/09/06 00:51:30]

come to think of it, you should only need to register this if there is an app
that has a URL handler, which in the common case isn't true.

make sure to test adding/removing apps with URL handler(s) though.

[sergeygs, 2013/09/06 02:51:14]

Sure, I'll test that case. This is registered for top-level navigations only, so
it shouldn't be very expensive overall. It could be possible to prefind a
handling app before creating a throttle, but as discussed in person, this seems
to be difficult right now. Leaving as is.

What I can do here easily (just noticed), is skip adding a throttle in
incognito: I'll do that.

[not at google - send to devlin, 2013/09/06 03:02:11]

As more recently discussed in person, the ExtensionInfoMap is already available
to this class so this can actually be simplified some more.

+  }
+#endif
+
 #if defined(OS_ANDROID)
   if (!is_prerendering && resource_type == ResourceType::MAIN_FRAME) {
     throttles->push_back(
         InterceptNavigationDelegate::CreateThrottleFor(request));
   }
 #endif
+
 #if defined(OS_CHROMEOS)
   if (resource_type == ResourceType::MAIN_FRAME) {
     // We check offline first, then check safe browsing so that we still can