Bug 463949 - [Browsers] org.eclipse.ui.internal.browser.ExternalBrowserInstance.openUrl() should use open(1) --args on OS XI20150408-1100

And discovered during testing:
  * Use open(1) only if an app bundle is specified, otherwise treat as
    command-line launch
  * Hook in StreamConsumer to report launch messages to browser.log
  * Fix WebBrowserUtil#reportError as it assumed it was running on
    the SWT thread, which is not true with external browsers

2 files changed, 41 insertions, 10 deletions

diff --git a/bundles/org.eclipse.ui.browser/src/org/eclipse/ui/internal/browser/ExternalBrowserInstance.java b/bundles/org.eclipse.ui.browser/src/org/eclipse/ui/internal/browser/ExternalBrowserInstance.java
index 0a6f4d3df19..1589fa90f77 100644
--- a/bundles/org.eclipse.ui.browser/src/org/eclipse/ui/internal/browser/ExternalBrowserInstance.java
+++ b/bundles/org.eclipse.ui.browser/src/org/eclipse/ui/internal/browser/ExternalBrowserInstance.java
@@ -12,6 +12,7 @@
  *******************************************************************************/
 package org.eclipse.ui.internal.browser;
 
+import java.io.File;
 import java.net.URL;
 import java.util.ArrayList;
 
@@ -19,6 +20,7 @@ import org.eclipse.jface.util.Util;
 import org.eclipse.osgi.util.NLS;
 import org.eclipse.ui.PartInitException;
 import org.eclipse.ui.browser.AbstractWebBrowser;
+import org.eclipse.ui.internal.browser.browsers.StreamConsumer;
 
 /**
  * An instance of a running Web browser. rundll32.exe
@@ -35,7 +37,7 @@ public class ExternalBrowserInstance extends AbstractWebBrowser {
 	}
 
 	public void openURL(URL url) throws PartInitException {
-		String urlText = url.toExternalForm();
+		final String urlText = url == null ? null : url.toExternalForm();
 
 		ArrayList<String> cmdOptions = new ArrayList<String>();
 		String location = browser.getLocation();
@@ -49,9 +51,11 @@ public class ExternalBrowserInstance extends AbstractWebBrowser {
 		String[] params = WebBrowserUtil.createParameterArray(parameters, urlText);
 
 		try {
-			if ( Util.isMac()) {
+			if (Util.isMac() && isMacAppBundle(location)) {
 				cmdOptions.add(0, "-a"); //$NON-NLS-1$
 				cmdOptions.add(0, "open"); //$NON-NLS-1$
+				// --args supported in 10.6 and later
+				cmdOptions.add("--args");//$NON-NLS-1$
 			}
 
 			for (String param : params) {
@@ -61,6 +65,12 @@ public class ExternalBrowserInstance extends AbstractWebBrowser {
 			Trace.trace(Trace.FINEST, "Launching " + join(" ", cmd)); //$NON-NLS-1$//$NON-NLS-2$
 
 			process = Runtime.getRuntime().exec(cmd);
+			Thread outConsumer = new StreamConsumer(process.getInputStream());
+			outConsumer.setName("External browser output reader"); //$NON-NLS-1$
+			outConsumer.start();
+			Thread errConsumer = new StreamConsumer(process.getErrorStream());
+			errConsumer.setName("External browser  error reader"); //$NON-NLS-1$
+			errConsumer.start();
 		} catch (Exception e) {
 			Trace.trace(Trace.SEVERE, "Could not launch external browser", e); //$NON-NLS-1$
 			WebBrowserUtil.openError(NLS.bind(
@@ -70,6 +80,10 @@ public class ExternalBrowserInstance extends AbstractWebBrowser {
 			public void run() {
 				try {
 					process.waitFor();
+					if (process.exitValue() != 0) {
+						Trace.trace(Trace.SEVERE, "Could not launch external browser"); //$NON-NLS-1$
+						WebBrowserUtil.openError(NLS.bind(Messages.errorCouldNotLaunchWebBrowser, urlText));
+					}
 					DefaultBrowserSupport.getInstance().removeBrowser(
 							ExternalBrowserInstance.this);
 				} catch (Exception e) {
@@ -81,6 +95,19 @@ public class ExternalBrowserInstance extends AbstractWebBrowser {
 		thread.start();
 	}
 
+	/**
+	 * @return true if the location appears to be a Mac Application bundle
+	 *         (.app)
+	 */
+	private boolean isMacAppBundle(String location) {
+		// A very quick heuristic based on Apple's Bundle Programming Guide
+		// https://developer.apple.com/library/mac/documentation/CoreFoundation/Conceptual/CFBundles/BundleTypes/BundleTypes.html#//apple_ref/doc/uid/10000123i-CH101-SW19
+		File bundleLoc = new File(location);
+		File macosDir = new File(new File(bundleLoc, "Contents"), "MacOS"); //$NON-NLS-1$ //$NON-NLS-2$
+		File plist = new File(new File(bundleLoc, "Contents"), "Info.plist"); //$NON-NLS-1$ //$NON-NLS-2$
+		return bundleLoc.isDirectory() && macosDir.isDirectory() && plist.isFile();
+	}
+
 	private String join (String delim, String ... data) {
 		StringBuilder sb = new StringBuilder();
 		for (int i = 0; i < data.length; i++) {
diff --git a/bundles/org.eclipse.ui.browser/src/org/eclipse/ui/internal/browser/WebBrowserUtil.java b/bundles/org.eclipse.ui.browser/src/org/eclipse/ui/internal/browser/WebBrowserUtil.java
index b5a6b13ade2..c49eec938a9 100644
--- a/bundles/org.eclipse.ui.browser/src/org/eclipse/ui/internal/browser/WebBrowserUtil.java
+++ b/bundles/org.eclipse.ui.browser/src/org/eclipse/ui/internal/browser/WebBrowserUtil.java
@@ -81,13 +81,15 @@ public class WebBrowserUtil {
 	 * @param message
 	 *            java.lang.String
 	 */
-	public static void openError(String message) {
+	public static void openError(final String message) {
 		Display d = Display.getCurrent();
 		if (d == null)
 			d = Display.getDefault();
-
-		Shell shell = d.getActiveShell();
-		MessageDialog.openError(shell, Messages.errorDialogTitle, message);
+		d.asyncExec(new Runnable() {
+			public void run() {
+				MessageDialog.openError(null, Messages.errorDialogTitle, message);
+			}
+		});
 	}
 
 	/**
@@ -96,14 +98,16 @@ public class WebBrowserUtil {
 	 * @param message
 	 *            java.lang.String
 	 */
-	public static void openMessage(String message) {
+	public static void openMessage(final String message) {
 		Display d = Display.getCurrent();
 		if (d == null)
 			d = Display.getDefault();
 
-		Shell shell = d.getActiveShell();
-		MessageDialog.openInformation(shell, Messages.searchingTaskName,
-				message);
+		d.asyncExec(new Runnable() {
+			public void run() {
+				MessageDialog.openInformation(null, Messages.searchingTaskName, message);
+			}
+		});
 	}
 
 	/**