Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 350377

Summary: [Browser] Default browser detection should use environment variables
Product: [Eclipse Project] Platform Reporter: Markus Keller <markus.kell.r>
Component: User AssistanceAssignee: Chris Goldthorpe <cgold>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: cgold, daniel_megert, john.arthorne, pwebster
Version: 3.7   
Target Milestone: 3.8 M1   
Hardware: PC   
OS: Windows 7   
Whiteboard:
Bug Depends on:    
Bug Blocks: 415065    
Attachments:
Description Flags
Fix
none
Fix 2
none
Patch to fix warnings none

Description Markus Keller CLA 2011-06-26 08:40:35 EDT 
Created attachment 198608 [details]
Fix

HEAD, follow-up to bug 258682

The fix for bug 258682 broke the PDE support for editing the plugin.xml and browsers.exsd. PDE only seems to understand elements that either have attributes or PCDATA, but not both.

Furthermore, the whole detection only works on en_US installs of Windows, since it hardcodes paths that are localized on that platform.

The attached patch fixes all these issues by using environment variables [1]. It also adds the "%ProgramFiles(x86)%" paths for 64-bit systems. I didn't add this for outdated browsers which should be removed anyway IMHO (Opera7, Netscape, mozilla.exe).

Note that I had to raise the BREE to 1.5, to make System#getenv(String) work.

Tested on Windows 7 (en_US, 64bit) and Windows XP (de_CH).


[1] See e.g. the table at the bottom of
http://msdn.microsoft.com/en-us/library/dd378457(v=VS.85).aspx
Comment 1 Chris Goldthorpe CLA 2011-06-27 16:28:36 EDT 
I like that fix! This is much better than the old way of searching where we looked on the C: and D: drives for the Program Files folder. I need to test this for a while before I commit, one thing I noticed on Windows 7 was that %ProgramFiles% expanded to "C:\Program Files (x86)", which means that it would not detect 64 bit versions of browsers. I also agree with pruning out the obsolete browsers.
Comment 2 Chris Goldthorpe CLA 2011-06-27 17:03:02 EDT 
An update on my last comment. System.getenv("ProgramFiles") is returning C:\Program Files (x86). In cmd.exe "set" with no arguments shows these environment variables

ProgramFiles=C:\Program Files
ProgramFiles(x86)=C:\Program Files (x86)
ProgramW6432=C:\Program Files

System.getenv("ProgramW6432") returns C:\Program Files

I'm not sure why System.getenv() would return a different value to what is returned from the "set" command.
Comment 3 Chris Goldthorpe CLA 2011-06-27 17:11:42 EDT 
John, I'd like to change the execution environment for org.eclipse.ui.browser from J2SE-1.4 to J2SE-1.5 for Eclipse 3.8/4.2. Are you going to be the author of the Juno Eclipse plan, if so can you include this change?
Comment 4 Markus Keller CLA 2011-06-28 07:21:58 EDT 
Hmm, %ProgramFiles% depends on whether you launch with a 32 or 64 bit VM:

public static void main(String[] args) {
	String[] vars= {"ProgramFiles", "ProgramFiles(x86)", "ProgramW6432"};
	for (String var : vars) {
		System.out.printf("%-20s %s\n", "%" + var +"%:", System.getenv(var));
	}
}

java version "1.6.0_24"
Java(TM) SE Runtime Environment (build 1.6.0_24-b07)
Java HotSpot(TM) Client VM (build 19.1-b02, mixed mode)
%ProgramFiles%:      C:\Program Files (x86)
%ProgramFiles(x86)%: C:\Program Files (x86)
%ProgramW6432%:      C:\Program Files

java version "1.6.0_22"
Java(TM) SE Runtime Environment (build 1.6.0_22-b04)
Java HotSpot(TM) 64-Bit Server VM (build 17.1-b03, mixed mode)
%ProgramFiles%:      C:\Program Files
%ProgramFiles(x86)%: C:\Program Files (x86)
%ProgramW6432%:      C:\Program Files
Comment 5 John Arthorne CLA 2011-06-28 08:27:17 EDT 
(In reply to comment #3)
> John, I'd like to change the execution environment for org.eclipse.ui.browser
> from J2SE-1.4 to J2SE-1.5 for Eclipse 3.8/4.2. Are you going to be the author
> of the Juno Eclipse plan, if so can you include this change?

+1. I have a program that automatically updates that list every time I do a plan update, so go ahead and update your BREE in the manifest.
Comment 6 Markus Keller CLA 2011-06-28 09:19:23 EDT 
Created attachment 198722 [details]
Fix 2

OK, looks like we need all 3:
%ProgramFiles% for backward compatibility
%ProgramFiles(x86)% for 32-bit apps on 64-bit installs
%ProgramW6432% for 64-bit apps

This patch adds that and removes outdated browsers on all platforms.
Comment 7 Chris Goldthorpe CLA 2011-06-28 13:14:01 EDT 
The new fix is good, I verified that on Win7 it detects IE, Firefox, Chrome and Opera correctly. I agree with the order of search you chose, which ensures that if running 64 bit Eclipse it will detect the 64 bit IE before the 32 bit version. I have committed Fix 2.

I still need to clean up the warnings for use of raw types. I will do that today so the nightly build is clean.
Comment 8 Markus Keller CLA 2011-06-28 13:42:57 EDT 
> I still need to clean up the warnings for use of raw types. I will do that
> today so the nightly build is clean.

Sorry, forgot about that. You can also suppress the warnings in the build by adding this line to the build.properties:

javacWarnings..jar=-raw,-unchecked


If you run into trouble because required bundles are not yet generified, I recommend enabling "Ignore unavoidable generic type problems" in the project's compiler options.
Comment 9 Markus Keller CLA 2011-06-28 13:43:54 EDT 
(In reply to comment #8)
Actually, it should be this:

javacWarnings..=-raw,-unchecked
Comment 10 Chris Goldthorpe CLA 2011-06-28 13:49:16 EDT 
Created attachment 198762 [details]
Patch to fix warnings
Comment 11 Chris Goldthorpe CLA 2011-06-28 13:50:57 EDT 
Patch to fix warnings has also been committed to HEAD. Fixed.
Comment 12 Chris Goldthorpe CLA 2011-06-28 14:25:13 EDT 
It was a good opportunity to clean up the code to use generics, most of them were easy to fix but there was one case where a list contained objects of different classes so I had to suppress warnings in the source in a few places.