added a preference so user can specify browser to open html page

added a preference so user can specify browser to open html page
Committed: https://code.google.com/p/dart/source/detail?r=2130

[devoncarew, 2011-12-05 19:58:27]

I have a couple of changes. Also, have you run / tested this on the Mac
platform? Launching apps is a little different there.

http://codereview.chromium.org/8801021/diff/1001/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.tools.debug.ui/src/com/google/dart/tools/debug/ui/internal/preferences/DebugPreferencePage.java
(right):

http://codereview.chromium.org/8801021/diff/1001/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.debug.ui/src/com/google/dart/tools/debug/ui/internal/preferences/DebugPreferencePage.java:232:
browserField.setEnabled(false);
Please pull all the enablement logic out into an updateEnablement() method, then
call that from the button listeners and the UI construction code instead of
having the logic in several places.

Also, you don't need to override the widgetDefaultSelected() methods - they
don't get called. I think there's a SelectionAdapter class you can use.

http://codereview.chromium.org/8801021/diff/1001/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/actions/RunInBrowserAction.java
(right):

http://codereview.chromium.org/8801021/diff/1001/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/actions/RunInBrowserAction.java:210:
IEclipsePreferences prefs = InstanceScope.INSTANCE.getNode(DEBUG_PLUGIN_ID);
Is this code to get around the fact that the UI plugin doesn't depend on the
debug plugin? Maybe it's time to move the RunInBrowserAction to the debug
plugin, and clean this code up a bit.

http://codereview.chromium.org/8801021/diff/1001/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/actions/RunInBrowserAction.java:316:
name = "chrome";
Is this necessary? Shouldn't we just look for whatever program the user
selected?

[danrubel, 2011-12-05 20:58:50]

http://codereview.chromium.org/8801021/diff/1001/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.tools.debug.core/src/com/google/dart/tools/debug/core/DartDebugCorePlugin.java
(right):

http://codereview.chromium.org/8801021/diff/1001/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.debug.core/src/com/google/dart/tools/debug/core/DartDebugCorePlugin.java:80:
private static final String PREFS_DEFAULT_BROWSER = "defaultBrowser";
Why have 2 prefs?
Could browserPath = "" be considered the "default browser"?

http://codereview.chromium.org/8801021/diff/1001/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/actions/RunInBrowserAction.java
(right):

http://codereview.chromium.org/8801021/diff/1001/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/actions/RunInBrowserAction.java:210:
IEclipsePreferences prefs = InstanceScope.INSTANCE.getNode(DEBUG_PLUGIN_ID);
On 2011/12/05 19:58:27, devoncarew wrote:
> Is this code to get around the fact that the UI plugin doesn't depend on the
> debug plugin? Maybe it's time to move the RunInBrowserAction to the debug
> plugin, and clean this code up a bit.

+1

[keertip, 2011-12-05 21:30:26]

http://codereview.chromium.org/8801021/diff/1001/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.tools.debug.core/src/com/google/dart/tools/debug/core/DartDebugCorePlugin.java
(right):

http://codereview.chromium.org/8801021/diff/1001/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.debug.core/src/com/google/dart/tools/debug/core/DartDebugCorePlugin.java:80:
private static final String PREFS_DEFAULT_BROWSER = "defaultBrowser";
With 2 prefs, user selection is not lost, so they can just toggle button instead
of setting the selection every time.

On 2011/12/05 20:58:51, danrubel wrote:
> Why have 2 prefs?
> Could browserPath = "" be considered the "default browser"?

http://codereview.chromium.org/8801021/diff/1001/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.tools.debug.ui/src/com/google/dart/tools/debug/ui/internal/preferences/DebugPreferencePage.java
(right):

http://codereview.chromium.org/8801021/diff/1001/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.debug.ui/src/com/google/dart/tools/debug/ui/internal/preferences/DebugPreferencePage.java:232:
browserField.setEnabled(false);
On 2011/12/05 19:58:27, devoncarew wrote:
> Please pull all the enablement logic out into an updateEnablement() method,
then
> call that from the button listeners and the UI construction code instead of
> having the logic in several places.
> 
> Also, you don't need to override the widgetDefaultSelected() methods - they
> don't get called. I think there's a SelectionAdapter class you can use.

Done.

http://codereview.chromium.org/8801021/diff/1001/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/actions/RunInBrowserAction.java
(right):

http://codereview.chromium.org/8801021/diff/1001/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/actions/RunInBrowserAction.java:210:
IEclipsePreferences prefs = InstanceScope.INSTANCE.getNode(DEBUG_PLUGIN_ID);
On 2011/12/05 19:58:27, devoncarew wrote:
> Is this code to get around the fact that the UI plugin doesn't depend on the
> debug plugin? Maybe it's time to move the RunInBrowserAction to the debug
> plugin, and clean this code up a bit.

Done.

http://codereview.chromium.org/8801021/diff/1001/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/actions/RunInBrowserAction.java:316:
name = "chrome";
On 2011/12/05 19:58:27, devoncarew wrote:
> Is this necessary? Shouldn't we just look for whatever program the user
> selected?

Done.

[keertip, 2011-12-05 21:37:07]

PTAL - Made suggested changes, for now removed Run in Browser from Libraries
view context menu since the action was moved to the debug ui plugin.

[devoncarew, 2011-12-06 05:10:17]

lgtm

Thanks for taking this on Keerti! It's been one of the main requests from our
users -

[danrubel, 2011-12-06 13:01:57]

LGTM