Bug 888363 - Fix default browser activation. r=jimm
authorBrian R. Bondy <netzen@gmail.com>
Thu, 15 Aug 2013 22:30:57 -0400
changeset 142991 60bf438fa2f29afd7778e00a5d0b3ace53643724
parent 142990 7c4e4b3afb0d159bcb85d63959fdd31332215638
child 142992 67714091ce5bd9c67e8c2d6415e06d7cd9edf900
push id25119
push userphilringnalda@gmail.com
push dateMon, 19 Aug 2013 00:45:03 +0000
treeherdermozilla-central@c8c9bd74cc40 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjimm
bugs888363
milestone26.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 888363 - Fix default browser activation. r=jimm Extra notes: GetModulePath has no changes, it is just moved up so it can be used earlier. GetDesktopBrowserPath was moved into the class because it needs to use a member there. GetDesktopBrowserPath implementation has changed, so please review it. Also its comments header is new and explains some stuff. IsTargetBrowser is no longer used because we instead use 2 other boolean values set on the SetTarget call.
browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
--- a/browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
+++ b/browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
@@ -32,19 +32,43 @@
 #define REQUEST_WAIT_TIMEOUT 30
 // Pulled from desktop browser's shell
 #define APP_REG_NAME L"Firefox"
 
 static const WCHAR* kFirefoxExe = L"firefox.exe";
 static const WCHAR* kMetroFirefoxExe = L"firefox.exe";
 static const WCHAR* kDefaultMetroBrowserIDPathKey = L"FirefoxURL";
 
-static bool GetDesktopBrowserPath(CStringW& aPathBuffer);
 static bool GetDefaultBrowserPath(CStringW& aPathBuffer);
 
+/*
+ * Retrieve our module dir path.
+ *
+ * @aPathBuffer Buffer to fill
+ */
+static bool GetModulePath(CStringW& aPathBuffer)
+{
+  WCHAR buffer[MAX_PATH];
+  memset(buffer, 0, sizeof(buffer));
+
+  if (!GetModuleFileName(NULL, buffer, MAX_PATH)) {
+    Log(L"GetModuleFileName failed.");
+    return false;
+  }
+
+  WCHAR* slash = wcsrchr(buffer, '\\');
+  if (!slash)
+    return false;
+  *slash = '\0';
+
+  aPathBuffer = buffer;
+  return true;
+}
+
+
 template <class T>void SafeRelease(T **ppT)
 {
   if (*ppT) {
     (*ppT)->Release();
     *ppT = NULL;
   }
 }
 
@@ -63,16 +87,18 @@ class __declspec(uuid("5100FEC1-212B-4BF
 {
 public:
 
   CExecuteCommandVerb() :
     mRef(1),
     mShellItemArray(NULL),
     mUnkSite(NULL),
     mTargetIsFileSystemLink(false),
+    mTargetIsDefaultBrowser(false),
+    mTargetIsBrowser(false),
     mIsDesktopRequest(true),
     mRequestMet(false)
   {
   }
 
   bool RequestMet() { return mRequestMet; }
   long RefCount() { return mRef; }
 
@@ -274,16 +300,47 @@ public:
     } else {
       Log(L"returning AHE_IMMERSIVE");
       *aLaunchType = AHE_IMMERSIVE;
       mIsDesktopRequest = false;
     }
     return S_OK;
   }
 
+  /*
+   * Retrieve the target path if it is the default browser
+   * or if not default, retreives the target path if it is a firefox browser
+   * or if the target is not firefox, relies on a hack to get the
+   * 'module dir path\firefox.exe'
+   * The reason why it's not good to rely on the CEH path is because there is
+   * no guarantee win8 will use the CEH at our expected path.  It has an in
+   * memory cache even if the registry is updated for the CEH path.
+   *
+   * @aPathBuffer Buffer to fill
+   */
+  bool GetDesktopBrowserPath(CStringW& aPathBuffer)
+  {
+    // If the target was the default browser itself then return early.  Otherwise
+    // rely on a hack to check CEH path and calculate it relative to it.
+
+    if (mTargetIsDefaultBrowser || mTargetIsBrowser) {
+      aPathBuffer = mTarget;
+      return true;
+    }
+
+    if (!GetModulePath(aPathBuffer))
+      return false;
+
+    // ceh.exe sits in dist/bin root with the desktop browser. Since this
+    // is a firefox only component, this hardcoded filename is ok.
+    aPathBuffer.Append(L"\\");
+    aPathBuffer.Append(kFirefoxExe);
+    return true;
+  }
+
   bool IsDefaultBrowser()
   {
     IApplicationAssociationRegistration* pAAR;
     HRESULT hr = CoCreateInstance(CLSID_ApplicationAssociationRegistration,
                                   NULL,
                                   CLSCTX_INPROC,
                                   IID_IApplicationAssociationRegistration,
                                   (void**)&pAAR);
@@ -313,88 +370,46 @@ public:
     CoTaskMemFree(registeredApp);
     if (!result)
       return false;
 
     // If the registry points another browser's path,
     // activating the Metro browser will fail. So fallback to the desktop.
     CStringW selfPath;
     GetDesktopBrowserPath(selfPath);
-    selfPath.MakeLower();
     CStringW browserPath;
     GetDefaultBrowserPath(browserPath);
-    browserPath.MakeLower();
 
-    return selfPath == browserPath;
+    return !selfPath.CompareNoCase(browserPath);
   }
 private:
   ~CExecuteCommandVerb()
   {
     SafeRelease(&mShellItemArray);
     SafeRelease(&mUnkSite);
   }
 
   void LaunchDesktopBrowser();
   bool SetTargetPath(IShellItem* aItem);
-  bool IsTargetBrowser();
 
   long mRef;
   IShellItemArray *mShellItemArray;
   IUnknown *mUnkSite;
   CStringW mVerb;
   CStringW mTarget;
   CStringW mParameters;
   bool mTargetIsFileSystemLink;
+  bool mTargetIsDefaultBrowser;
+  bool mTargetIsBrowser;
   DWORD mKeyState;
   bool mIsDesktopRequest;
   bool mRequestMet;
 };
 
 /*
- * Retrieve our module dir path.
- *
- * @aPathBuffer Buffer to fill
- */
-static bool GetModulePath(CStringW& aPathBuffer)
-{
-  WCHAR buffer[MAX_PATH];
-  memset(buffer, 0, sizeof(buffer));
-
-  if (!GetModuleFileName(NULL, buffer, MAX_PATH)) {
-    Log(L"GetModuleFileName failed.");
-    return false;
-  }
-
-  WCHAR* slash = wcsrchr(buffer, '\\');
-  if (!slash)
-    return false;
-  *slash = '\0';
-
-  aPathBuffer = buffer;
-  return true;
-}
-
-/*
- * Retrieve 'module dir path\firefox.exe'
- *
- * @aPathBuffer Buffer to fill
- */
-static bool GetDesktopBrowserPath(CStringW& aPathBuffer)
-{
-  if (!GetModulePath(aPathBuffer))
-    return false;
-
-  // ceh.exe sits in dist/bin root with the desktop browser. Since this
-  // is a firefox only component, this hardcoded filename is ok.
-  aPathBuffer.Append(L"\\");
-  aPathBuffer.Append(kFirefoxExe);
-  return true;
-}
-
-/*
  * Retrieve the current default browser's path.
  *
  * @aPathBuffer Buffer to fill
  */
 static bool GetDefaultBrowserPath(CStringW& aPathBuffer)
 {
   WCHAR buffer[MAX_PATH];
   DWORD length = MAX_PATH;
@@ -440,46 +455,16 @@ static bool GetDefaultBrowserAppModelID(
                        (LPBYTE)aIDBuffer, &len) != ERROR_SUCCESS || !len) {
     RegCloseKey(key);
     return false;
   }
   RegCloseKey(key);
   return true;
 }
 
-/*
- * Determines if the current target points directly to a particular
- * browser or to a file or url.
- */
-bool CExecuteCommandVerb::IsTargetBrowser()
-{
-  if (!mTarget.GetLength() || !mTargetIsFileSystemLink)
-    return false;
-
-  CStringW modulePath;
-  if (!GetModulePath(modulePath))
-    return false;
-
-  modulePath.MakeLower();
-
-  CStringW tmpTarget = mTarget;
-  tmpTarget.Replace(L"\"", L"");
-  tmpTarget.MakeLower();
-  
-  CStringW checkPath;
-  
-  checkPath = modulePath;
-  checkPath.Append(L"\\");
-  checkPath.Append(kFirefoxExe);
-  if (tmpTarget == checkPath) {
-    return true;
-  }
-  return false;
-}
-
 namespace {
   const FORMATETC kPlainTextFormat =
     {CF_TEXT, 0, DVASPECT_CONTENT, -1, TYMED_HGLOBAL};
   const FORMATETC kPlainTextWFormat =
     {CF_UNICODETEXT, 0, DVASPECT_CONTENT, -1, TYMED_HGLOBAL};
 }
 
 bool HasPlainText(IDataObject* aDataObj) {
@@ -540,16 +525,17 @@ bool CExecuteCommandVerb::SetTargetPath(
     // note, more advanced use may have issues with paths with spaces.
     if (!InternetCrackUrlW(cstrText, 0, 0, &components)) {
       Log(L"Failed to identify object text '%s'", cstrText);
       return false;
     }
 
     mTargetIsFileSystemLink = (components.nScheme == INTERNET_SCHEME_FILE);
     mTarget = cstrText;
+
     return true;
   }
 
   Log(L"No data object or data object has no text.");
 
   // Use the shell item display name
   LPWSTR str = NULL;
   mTargetIsFileSystemLink = true;
@@ -557,34 +543,45 @@ bool CExecuteCommandVerb::SetTargetPath(
     mTargetIsFileSystemLink = false;
     if (FAILED(aItem->GetDisplayName(SIGDN_URL, &str))) {
       Log(L"Failed to get parameter string.");
       return false;
     }
   }
   mTarget = str;
   CoTaskMemFree(str);
+
+  CStringW defaultPath;
+  GetDefaultBrowserPath(defaultPath);
+  mTargetIsDefaultBrowser = !mTarget.CompareNoCase(defaultPath);
+
+  size_t browserEXELen = wcslen(kFirefoxExe);
+  mTargetIsBrowser = mTarget.GetLength() >= browserEXELen &&
+                     !mTarget.Right(browserEXELen).CompareNoCase(kFirefoxExe);
+
   return true;
 }
 
 /*
  * Desktop launch - Launch the destop browser to display the current
  * target using shellexecute.
  */
 void CExecuteCommandVerb::LaunchDesktopBrowser()
 {
   CStringW browserPath;
   if (!GetDesktopBrowserPath(browserPath)) {
     return;
   }
 
   // If a taskbar shortcut, link or local file is clicked, the target will
-  // be the browser exe or file.
+  // be the browser exe or file.  Don't pass in -url for the target if the
+  // target is known to be a browser.  Otherwise, one instance of Firefox
+  // will try to open another instance.
   CStringW params;
-  if (!IsTargetBrowser() && !mTarget.IsEmpty()) {
+  if (!mTargetIsDefaultBrowser && !mTargetIsBrowser && !mTarget.IsEmpty()) {
     // Fallback to the module path if it failed to get the default browser.
     GetDefaultBrowserPath(browserPath);
     params += "-url ";
     params += "\"";
     params += mTarget;
     params += "\"";
   }
 
@@ -665,17 +662,17 @@ IFACEMETHODIMP CExecuteCommandVerb::Exec
     Log(L"CoAllowSetForegroundWindow result %X", hr);
     activateMgr->Release();
     return false;
   }
 
   Log(L"Metro Launch: verb:%s appid:%s params:%s", mVerb, appModelID, mTarget); 
 
   // shortcuts to the application
-  if (IsTargetBrowser()) {
+  if (mTargetIsDefaultBrowser) {
     hr = activateMgr->ActivateApplication(appModelID, L"", AO_NONE, &processID);
     Log(L"ActivateApplication result %X", hr);
   // files
   } else if (mTargetIsFileSystemLink) {
     hr = activateMgr->ActivateForFile(appModelID, mShellItemArray, mVerb, &processID);
     Log(L"ActivateForFile result %X", hr);
   // protocols
   } else {