"Redirecting URLs to Packaged Apps" implementation: revised

chrome/browser/ui/browser_navigator.cc

Index: chrome/browser/ui/browser_navigator.cc
diff --git a/chrome/browser/ui/browser_navigator.cc b/chrome/browser/ui/browser_navigator.cc
index 02a5e8fbd261039c46b400d647a841450a9f2fe3..916e68729368a515a769996136cd9e1150a2551d 100644
--- a/chrome/browser/ui/browser_navigator.cc
+++ b/chrome/browser/ui/browser_navigator.cc
@@ -10,9 +10,11 @@
 #include "base/prefs/pref_service.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/apps/app_url_redirector.h"
 #include "chrome/browser/browser_about_handler.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/google/google_url_tracker.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
@@ -39,6 +41,7 @@
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_view.h"
+#include "extensions/common/switches.h"

[not at google - send to devlin, 2013/09/04 15:37:21]

unused?

[sergeygs, 2013/09/05 09:19:03]

Done.

 
 #if defined(USE_AURA)
 #include "ui/aura/window.h"
@@ -475,9 +478,25 @@ void Navigate(NavigateParams* params) {
   if (!AdjustNavigateParamsForURL(params))
     return;
 
-  ExtensionService* service = params->initiating_profile->GetExtensionService();
-  if (service)
-    service->ShouldBlockUrlInBrowserTab(&params->url);
+  // See if the extension service wants to block this navigation or some
+  // platform app wants to intercept it.
+  extensions::ExtensionSystem* system =

[benwells, 2013/09/04 23:54:20]

It's unfortunate this code has to be here. This function is already complicated
and now it has another special case. Is there no other, more systematic way to
do this?

If this is here why do we need the other code to redirect to platform apps?

[sergeygs, 2013/09/05 09:19:03]

To answer the last question: because this deals only with top-level,
browser-initiated navigations. If you click a link which doesn't have
target='_blank' in a tab, WebKit does most of the job. And if you don't stop it
early enough, it will navigate.

But long story short: this is now obsoleted by the latest patch.

+      extensions::ExtensionSystem::Get(params->initiating_profile);
+  if (system) {
+    ExtensionService* service = system->extension_service();
+    if (service && service->ShouldBlockUrlInBrowserTab(&params->url)) {
+      // Proceed. The call just rewrites the URL to the error page: we still
+      // have to display it.
+    } else {
+      if (AppUrlRedirector::MaybeLaunchAppWithUrl(
+              params->initiating_profile,

[not at google - send to devlin, 2013/09/04 15:37:21]

Just grab the AppUrlRedirector from the initiating profile (using that
profile-keyed-service stuff) and call MaybeLaunchAppWithUrl on it. Making
MaybeLaunchAppWithUrl static is awkward and leads to unnecessary checks and
comments such as the one in that file.

However, you could add a static Get method to AppUrlRedirector if you wish
(similar to how ExtensionSystem has a Get method).

[benwells, 2013/09/04 23:54:20]

At the moment the redirector is a tab helper (or whatever they are called now),
not a PKS.

[not at google - send to devlin, 2013/09/05 00:01:52]

Yes, realised this later after chatting to Sergey. I suggested that these
actually be profile-scoped and that it should be responsible for listening to
WebContents creation and destruction (via WebContents::AddCreatedCallback). That
keeps the fact that these are Tab-scoped as an implementation detail, which is
what they should be.

[sergeygs, 2013/09/05 09:19:03]

Obsolete.

+              params->url,
+              params->referrer.url)) {
+        // An app has intercepted the navigation: cancel it here.
+        return;
+      }
+    }
+  }
 
   // The browser window may want to adjust the disposition.
   if (params->disposition == NEW_POPUP &&