185000 - Make Integration of mozilla and Java Plugin be better

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Pete Zha]

We want to make the integration of mozilla and java plugin be better. I will
upload a proposal later.
I file this in Plug-ins module because our proposal maybe relate some work in
Plug-ins module, not only OJI module. We need input from Plugin guys of mozilla
on this proposal.

Comment 1

[Pete Zha]

Attachment: The proposal of java integration work.

This proposal's target platform is Unix(Linux and Solaris).

Comment 2

[Pete Zha]

cc default module owner

Comment 3

[Pete Zha]

asa, scott,
This bug is related to OJI work. We have plan to make mozilla integrate with
java plugin better than now on Unix. Please review the proposal and give us some
comments. Thanks a lot!

Comment 4

[Asa Dotzler [:asa]]

adding a couple folks that might care to the cc:

Comment 5

[Pete Zha]

Attachment: The proposal of java integration work

This is a new version with minor changs:
1) change javaplugin.path to java.path
2) change format of java.path file
3) change to use symbol link to detect default java plugin

Comment 6

[Pete Zha]

My proposal on how to implement this in mozilla:
1) UI
We will create a config dialog by xul and js. Maybe we will put it in
preference->advanced. Just add a panel is OK I think.
2) Parse the config file
Write some code(maybe a component or service) to parse the config file. I will
try to do it in js first.
3) Save the configuration
Currently, our plan is to create a symbol link in ~/.mozilla/plugins directory
if the user choose a java plugin. We will not store the config in pref.js
because mozilla will not read user profile when load the plugin(Please let me
know if you think I was wrong. Thanks).

Target Milestones->1.4beta

Comment 7

[Pete Zha]

Attachment: Browser side java integration

Latest proposal.

Comment 8

[Pete Zha]

I'm not sure where I should put the java plugin config UI. I suggest the
preference->advanced(pref-advanced.xul). I want to add a panel in that area.
Does anyone have good suggestion?

Comment 9

[Judson Valeski]

Is this bug soley about changing JRE configuration/setup (including UI), or does
it also suggest that the OJI layer and model will be re-worked?

Comment 10

[Pete Zha]

This bug is for mozilla on unix only. We want to add UI in mozilla to find out
the java installation on the system and config mozilla to use it.
JRE side change will be made in future release. It will update the config
file(version file) when install/uninstall.

Comment 11

[Pete Zha]

Attachment: patch v1

First working patch include following changes:
1) Add a UI in pref-advanced.xul to switch java vm
2) Add two interface and their implementation
nsIJVMConfigManager - a manager to manage jvm config list and set current JVM
nsIJVMConfig - a interface to store JVM information
nsJVMConfigManagerUnix - Implementation of nsIJVMConfigManager on Unix
platform.

Comment 12

[Pete Zha]

Attachment: image of the pref-advanced window with this patch

Comment 13

[Pete Zha]

Comment on attachment 114416 [details]
image of the pref-advanced window with this patch

"*" means the browser is using this JVM
Note:Currently, I will first try to create symlink in <plugin dir>. If the user
don't have permission to write that directory, I will create symlink in
~/.mozilla/plugins

Comment 14

[Pete Zha]

Comment on attachment 114415 [details] [diff] [review]
patch v1

beard, please take a look at this patch. Thanks!

Comment 15

[Pete Zha]

Comment on attachment 114415 [details] [diff] [review]
patch v1

ask r= from joshua and ask sr= from [e-mail]

Comment 16

[Pete Zha]

Attachment: patch v2

delete following line in the patch v1
+
+// For font filter
+pref("font.foundry_filter", "linotype");

Comment 17

[Pete Zha]

Attachment: patch v3

Add following changes:
1) Search default java install location to find java installations
2) Change UI to two columns: Name(type + " " + version) and Home
3) Remove description since it's useless
4) Fix memory leak in previou patch

Comment 18

[Pete Zha]

Attachment: patch v4

modify several problems:
1) use PR_GetLibraryName instead use libjavaplugin_oji.so to match the library
name
2) Fix some intend problem and some usless varibles.
This patch is ready for review.

Comment 19

[Pete Zha]

Comment on attachment 116405 [details] [diff] [review]
patch v4

Joshua, Please review this patch. Thanks!

Comment 20

[Pete Zha]

Xiaobin,
I'm going to add new function in nsIJVMManager interface. Will this effect JPI?
Because I found that JPI will include the nsIJVMManager.h which is generated
from nsIJVMManager.idl.

Comment 21

[Pete Zha]

Attachment: patch v5

New patch with following changes:
1) Move nsIJVMConfigManager to new files: nsIJVMConfigManager.idl
nsJVMConfigManager.h, nsJVMConfigManager.cpp
2) Modify makefiles
3) Modify nsJVMConfigManager.cpp to register nsIJVMConfigManager as a component
and we can use it from service manager.

Comment 22

[Pete Zha]

Comment on attachment 117584 [details] [diff] [review]
patch v5

Joshua, beard:
Please review it. thanks!

Comment 23

[Joshua Xia]

looks good.

>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1

all new code should be written using the MPL tri-license.

Comment 24

[Joshua Xia]

Comment on attachment 117584 [details] [diff] [review]
patch v5

r=joshua.xia

Comment 25

[Pete Zha]

Thanks, joshua
There is another little change that we should do is use same gcc build plugin
with mozilla. JPI team will supply gcc3.2 build plugin in jre1.4.2.
I will make a new patch soon. The Xiaobin and you can review it.

Comment 26

[Pete Zha]

Attachment: patch v6

With following changes between patch v5 and v6:
1) Remove support for ns600 dir since the java plugin in that dir is for ns6
only.
2) Add support for gcc32 build java plugin. If mozilla is built by gcc32, we
will only pick up gcc32 built java plugin. This is because mozilla can't work
with java plugin which is compiled with different compiler as mozilla.

Comment 27

[Pete Zha]

Attachment: patch v7

Minor changes from patch v6
1) Just change to use compiler field to determine whether we should list in
java installation list.
2) Fix license problem to use MPL for new files.

Comment 28

[Pete Zha]

Comment on attachment 118403 [details] [diff] [review]
patch v7

XiaoBin, please review it. Thanks

Comment 29

[Pete Zha]

Attachment: Browser side java integration

Update the proposal for following changes:
1) modify versions file's format to add "compiler" attribute
2) add support for searching default java install location

Comment 30

[Gervase Markham [:gerv]]

Pete: incidentally, there's some good advice on defining file formats (a lot of
which you seem to be following) here:
http://www.catb.org/~esr/writings/taoup/html/ch05s02.html

Gerv

Comment 31

[Pete Zha]

Attachment: patch v8

New patch with following changes:
1) versions file format changed. mozilla.plugin.path field will support
multiple browser. For detail, Please see the proposal which I will upload here
soon.
2) Don't create symbol link in home directory since diferent version of gcc
compiled plugin will cause mozilla crash.

Comment 32

[Xiaobin Lu]

Several comments about last patch posted:

1. AppendJVMConfig's return value should be PRIntn, not PRBool (please double
check, even though PRIntn is equivalent to PRBool, there is no guarantee).

2. Possible Enhancement: better not to hardcode __GNUC__ =3 to the code, we can
define it somewhere, same as __GNUC_MINOR__

3. I noticed in your proposal, section 5.3, the path could be a path to jdk. If
that is case, for example, path= and
mozilla1.3.plugin.path=/plugin/i386/ns610-gcc32/libjavaplugin_oji.so, append the
later one to "path" won't resolve into a valid path
(/usr/local/java/j2sdk1.4.2/plugin/i386/ns610-gcc32/libjavaplugin_oji.so), the
path should be JRE path, not JDK. So in "ParseLine"function, between
"testPath.Assign(path)" and "testPath.Append(mozillaPluginPath)", you should
test whether path is jdk path or jre path. You have noticed this in
"AddDirectory" function.

4. Possible enhancement: Eliminate "GetAgentString(float* _retval)" or"
GetAgentVersion(nsCAutoString& _retval)" , use some conversion directly to
convert the string to float. Same for "AddDirectory" function, having two
function is fine, but that adds a little bit complexity to the code. 

5. Just a thought, if we have a clear mapping between Mozilla version and
"ns610" or "ns7", we can eliminate mozilla1.2(3).plugin.path. So we can have
some rule such as 

Mozilla 1.0 - Mozilla 1.2 use ns610.plugin.path
Mozilla 1.3 or later use ns7.plugin.path

But we can leave as it is now. 

Also please always fall back to use ns610.plugin.path (since it will be always
there, at least for 5 years) if ns7.plugin.path can not be found. (This is
because the JRE and Netscape/Mozilla release schedule difference).

6. There is some code overlapping between "GetMozillaPluginPath" and
"GetNSVersion" about how to determing the Netscape version, we can call
GetNSVersion from "GetMozillaPluginPath". Since having two places to have the
same logic will make code maintainence work difficult.

Pete, please "Accept" this bug. Don't leave the bug state in "New" too long.
Thanks!

Comment 33

[Pete Zha]

Attachment: patch v9

>1. AppendJVMConfig's return value should be PRIntn, not PRBool (please double
>check, even though PRIntn is equivalent to PRBool, there is no guarantee).
I have checked. The return value should be PRBool.

>2. Possible Enhancement: better not to hardcode __GNUC__ =3 to the code, we
can
>define it somewhere, same as __GNUC_MINOR__
OK, I will define a macro for this.

>3. I noticed in your proposal, section 5.3, the path could be a path to jdk.
If
>that is case, for example, path= and
>mozilla1.3.plugin.path=/plugin/i386/ns610-gcc32/libjavaplugin_oji.so, append
the
>later one to "path" won't resolve into a valid path
>(/usr/local/java/j2sdk1.4.2/plugin/i386/ns610-gcc32/libjavaplugin_oji.so), the

>path should be JRE path, not JDK. So in "ParseLine"function, between
>"testPath.Assign(path)" and "testPath.Append(mozillaPluginPath)", you should
>test whether path is jdk path or jre path. You have noticed this in
>"AddDirectory" function.
OK, I will determine the installation type.

>4. Possible enhancement: Eliminate "GetAgentString(float* _retval)" or"
>GetAgentVersion(nsCAutoString& _retval)" , use some conversion directly to
>convert the string to float. Same for "AddDirectory" function, having two
>function is fine, but that adds a little bit complexity to the code. 
Both of these functions are useful. I just overwite these function using same
name but different parameters. And the conversion from nsAString to float is
complex. So I call GetAgentVersion(nsAString& _retval) in
GetAgentVersion(float* _retval) and then use nsIWritableVariant to do the
conversion

>5. Just a thought, if we have a clear mapping between Mozilla version and
>"ns610" or "ns7", we can eliminate mozilla1.2(3).plugin.path. So we can have
>some rule such as 
>
>Mozilla 1.0 - Mozilla 1.2 use ns610.plugin.path
>Mozilla 1.3 or later use ns7.plugin.path
>
>But we can leave as it is now. 
Good idea, I will think about this later.

>Also please always fall back to use ns610.plugin.path (since it will be always

>there, at least for 5 years) if ns7.plugin.path can not be found. (This is
>because the JRE and Netscape/Mozilla release schedule difference).
OK.

>6. There is some code overlapping between "GetMozillaPluginPath" and
>"GetNSVersion" about how to determing the Netscape version, we can call
>GetNSVersion from "GetMozillaPluginPath". Since having two places to have the
>same logic will make code maintainence work difficult.
I have moved these logic to a function named GetNSVersion(nsAString& _retval)
And changed GetNSVersion and GetArch to TestNSVersion and TestArch because I
think it's more reasonable

Comment 34

[Pete Zha]

Attachment: patch v10

minor changes

Comment 35

[Pete Zha]

Comment on attachment 119951 [details] [diff] [review]
patch v10

xiaobin has reviewed it and think it's ok.

Comment 36

[Pete Zha]

Attachment: Browser side java integration

Comment 37

[Pete Zha]

Attachment: screen shot

Comment 38

[Gervase Markham [:gerv]]

UI feedback:

- "JVM Configuration" is a bad title for that bit of UI, because you can't
configure your JVM, and most people don't know what a JVM is. If it allows you
to choose which version of Java to use, it should be titled "Select Java version".

- How is the version currently in use indicated? Is it that tiny text star on
the right? The right way to do this is to list all of the versions with radio
buttons to choose the one currently in use. If that's not possible, then it
should be a single-select field, with the highlighted JVM being used.

- I don't know where "jvm 1.4.2" comes from as the name, but it's not very
clear. The title of that column should be "Version" and the field should be
"1.4.2". 

- The "Home" column should be titled "Path". It may be JAVA_HOME, but to the
user, it's the path to an installation.

- The entire UI should not be present if only one Java version is installed.
Optionally, it could be replaced with "  (1.4.2)  " after the "use Java" text
further up the dialog.

Gerv

Comment 39

[Pete Zha]

>- "JVM Configuration" is a bad title for that bit of UI, because you can't
>configure your JVM, and most people don't know what a JVM is. If it allows you
>to choose which version of Java to use, it should be titled "Select Java >version".
Sounds reasonable. Since it's for select a java installation to use, How about
title it to "Select Java"

>- How is the version currently in use indicated? Is it that tiny text star on
>the right? The right way to do this is to list all of the versions with radio
>buttons to choose the one currently in use. If that's not possible, then it
>should be a single-select field, with the highlighted JVM being used.
Currently, I use a star('*') in the last column to indicate the current java
version.

>- I don't know where "jvm 1.4.2" comes from as the name, but it's not very
>clear. The title of that column should be "Version" and the field should be
>"1.4.2". 
jre means the type of the java installation. I display the type and the version
in the same column so I named the title as "Name"

>- The "Home" column should be titled "Path". It may be JAVA_HOME, but to the
>user, it's the path to an installation.
How about change it to "Java Home"?

>- The entire UI should not be present if only one Java version is installed.
>Optionally, it could be replaced with "  (1.4.2)  " after the "use Java" text
>further up the dialog.
Since the browser could be installed before user install java. So, we should
list java installation and let user to choose it. If there no java installation
on the system, I will not display this panel.

Thanks for the feedback. I will update the patch based on the discussion and
after beard review it.

Comment 40

[Gervase Markham [:gerv]]

>>- "JVM Configuration" is a bad title for that bit of UI, because you can't
>>configure your JVM, and most people don't know what a JVM is. If it allows you
>>to choose which version of Java to use, it should be titled "Select Java
>version".
>
> Sounds reasonable. Since it's for select a java installation to use, How about
> title it to "Select Java"

"Select Java" isn't really good English. I appreciate that, in theory, you could
have more than one copy of the same Java version installed, but I still think
"Select Java Version" is a better title.

>>- How is the version currently in use indicated? Is it that tiny text star on
>>the right? The right way to do this is to list all of the versions with radio
>>buttons to choose the one currently in use. If that's not possible, then it
>>should be a single-select field, with the highlighted JVM being used.
>
> Currently, I use a star('*') in the last column to indicate the current java
> version.

I'm afraid I really think that should be changed. This should be a single-select
list, or there should be radio buttons. These are the standard ways of choosing
one item from many.

>>- I don't know where "jvm 1.4.2" comes from as the name, but it's not very
>>clear. The title of that column should be "Version" and the field should be
>>"1.4.2". 
>
> jre means the type of the java installation. I display the type and the version
> in the same column so I named the title as "Name"

Why does the user care what type of installation it is? Do some types of
installation not work with Mozilla? If so, we shouldn't list them.

>>- The "Home" column should be titled "Path". It may be JAVA_HOME, but to the
>>user, it's the path to an installation.
> 
> How about change it to "Java Home"?

That's redundant with "Select Java Version". The concept of "Home", in terms of
a directory, means nothing to most users - in fact, it would be confusing, as it
would get confused with your browser's "Home", which is a URL.

>>- The entire UI should not be present if only one Java version is installed.
>>Optionally, it could be replaced with "  (1.4.2)  " after the "use Java" text
>>further up the dialog.
>
> Since the browser could be installed before user install java. So, we should
> list java installation and let user to choose it. 

I don't quite understand that sentence. If there is one Java on the system, we
should just use it (when "use Java" is ticked) without bothering the user. Also,
now I think about it, this entire UI should be disabled if "use Java" is unticked.

> If there no java installation
> on the system, I will not display this panel.

Good stuff.

Gerv

Comment 41

[timeless]

How can i tell the difference between:

IBM JRE 1.3.1
Sun JRE 1.3.1
Blackdown JRE 1.3.1?

Please don't expect the user to figure it out from the path.

Comment 42

[Gervase Markham [:gerv]]

If that's the problem, then the title should still be "Version", and the
contents should be "IBM 1.3.1" or "Sun 1.3.1" or "Blackdown 1.3.1". Or perhaps
"1.3.1 (Sun)" etc. 

The letters "JRE" mean nothing to most users, and don't really add anything for
experienced users.

Gerv

Comment 43

[Pete Zha]

timeless,
I think this patch can't support Java installation besides Sun Java. Because it
will use the "versions" file which created by Sun Java. Anyway, if other plugin
verdor need this feature in mozilla, they could register itself in "versions" file

Comment 44

[Patrick C. Beard]

Comment on attachment 119951 [details] [diff] [review]
patch v10

The bulk of these changes have nothing to do with existing OJI infrastructure.
I'd rather somebody more versed in XUL and preferences super-review this.

Comment 45

[Pete Zha]

beard,
Could you at least take a look at the service(nsIJVMConfigManager) part? Since
it will be in OJI module.

Comment 46

[Alec Flett]

Comment on attachment 119951 [details] [diff] [review]
patch v10

+    var nsIJVMConfig = Components.interfaces.nsIJVMConfig;
+    var currentJVMPluginPath = getCurrentJVMPluginPath();
+    var jvmConfigMgr = getJVMConfigMgr();
+    var jvmConfigList = getJVMConfigList();
+    var oldJVMConfig = null;


most of these should be "const" not "var"

there are a few more of these in the patch, its worth you taking a sweep
through.

+	   var strBundleService =
Components.classes["@mozilla.org/intl/stringbundle;1"].getService();
+	   strBundleService =
strBundleService.QueryInterface(Components.interfaces.nsIStringBundleService);
+	   var navbundle =
strBundleService.createBundle("chrome://navigator/locale/navigator.properties")
;
+	   var brandbundle =
strBundleService.createBundle("chrome://global/locale/brand.properties");

probably worth doing the XBL-based string bundle (something like <stringbundle
src="..."/> - look around at some of the other XUL)
+    <tree id="jvmConfigListObj" flex="1" style="width: 60em; height: 10em"

I'm not a huge fan of this inline style. what does this buy you that flex=1
wouldn't?

+protected:
+    nsAutoString mVersion;
+    nsAutoString mType;
+    nsAutoString mOS;
+    nsAutoString mArch;
+    nsAutoString mPath;
+    nsAutoString mMozillaPluginPath;
+    nsAutoString mDescription;

woah! member variables should be nsString unless they are going to be between
48 and 64 bytes long. (and if so, you need a comment to that effect)


Also, you shouldn't be using strings for paths -  you should be using nsIFile
and using pref's GetComplexValue to get the value of type nsIFile.


+    nsHashtable* mJVMConfigList;

use nsTHashtable, nsHashtable is deprecated. Also, make sure to declare it as a
concrete class rather than a pointer to a malloc'ed object.

+    nsCStringKey key(ToNewCString(path));

oh man! I know you're supposed to be using nsHashtable, but you're leaking a
string here every time! nsCStringKey creates a copy of the string. be careful
with ToNewCString()


+static PRBool PR_CALLBACK

you'll have to use PR_STATIC_CALLBACK(PRBool) here.

+NS_IMETHODIMP
+nsJVMConfigManagerUnix::GetJVMConfigList(nsISupportsArray **_retval)

You should be using nsIArray, not nsISupportsArray (which is deprecated)

does this need any kind of sorting through? I mean you're enumerating a
hashtable, so I think the order is going to be random.. is that the intent?

+PRBool
+nsJVMConfigManagerUnix::TestArch(nsAString& aPluginPath, nsAString& aArch)
+{
+    aArch.Assign(NS_LITERAL_STRING("i386"));
+    if (TestDir(aPluginPath, aArch)) return PR_TRUE;
+
+    aArch.Assign(NS_LITERAL_STRING("sparc"));
+    if (TestDir(aPluginPath, aArch)) return PR_TRUE;
+
+    return PR_FALSE;
+}

is this really safe? what if my home directory is mounted across sparc and i386
boxes - is my sparc box going to try to load the i386 plugin?

+    nsAutoString testPath;
+    testPath.Assign(aHomeDirName);
+    testPath.Append(NS_LITERAL_STRING("/jre"));

why not nsAutoString testPath(aHomeDirName); ?

this is why I think we need to be dealing with this stuff in terms of nsIFile -
all this path testing would be a lot cleaner and we wouldn't see all these path
separators all over the code.



Please go through this patch again. xiaobin did not do a very thorough job and
should have already caught things like the ToNewCString() thing. I'm clearing
his review.. whoever reviews this, I want to see THEIR comment in the bug
saying that they reviewed it - specifically because of the big problems I saw.

Comment 47

[Pete Zha]

Attachment: patch v11

changes:
1) UI changed. I will upload screenshot later
2) address alecf's review comments.
>most of these should be "const" not "var"
>there are a few more of these in the patch, its worth you taking a sweep
>through.
only oldJVMConfig is var. I have modified others

>probably worth doing the XBL-based string bundle (something like <stringbundle

>src="..."/> - look around at some of the other XUL)
done

>I'm not a huge fan of this inline style. what does this buy you that flex=1
>wouldn't?
this has been removed

>woah! member variables should be nsString unless they are going to be between
>48 and 64 bytes long. (and if so, you need a comment to that effect)
I changed them to nsString

>Also, you shouldn't be using strings for paths -  you should be using nsIFile
>and using pref's GetComplexValue to get the value of type nsIFile.
Yes, I changed to use nsIFile in this patch.

>use nsTHashtable, nsHashtable is deprecated. Also, make sure to declare it as
a
>concrete class rather than a pointer to a malloc'ed object.
I declare it as concrete class in this patch.
Since nsTHashtable is new and no other place is using this. Can I change to use
it later?

>oh man! I know you're supposed to be using nsHashtable, but you're leaking a
>string here every time! nsCStringKey creates a copy of the string. be careful
>with ToNewCString()
I changed to use nsStringKey and remove the ToNewCString.

>you'll have to use PR_STATIC_CALLBACK(PRBool) here.
done

>You should be using nsIArray, not nsISupportsArray (which is deprecated)
done

>does this need any kind of sorting through? I mean you're enumerating a
>hashtable, so I think the order is going to be random.. is that the intent?
I don't sort it in current patch. I propose to sort it in another patch(bug).
Because I'm not sure what's the right way to sort it right now.

>is this really safe? what if my home directory is mounted across sparc and
i386
>boxes - is my sparc box going to try to load the i386 plugin?
I add #ifdef in this function.

alecf, Thanks a lot for the review. Please review this new patch again when you
have time.

Comment 48

[Pete Zha]

Attachment: screen shot

screen shot

Comment 49

[Alec Flett]

wow, that was quick - thanks for all the cleanup. I'll get right on this.

Comment 50

[Gervase Markham [:gerv]]

Pete,

The new UI is definitely an improvement on the old, so that's great. But were
you not going to put the JVM author in the "version" column, rather than the
mysterious letters "JRE"? 

Also, it's still true that the star-in-last-column thing isn't consistent with
any other UI we have in the product. It really does make sense for the selected
JVM to be the one used, and for the control to be restricted to a single
selection at all times. 

Lastly, your screenshot shows the UI with just one JVM in it. Do you plan to
make it so that the choice UI only appears when there is a choice to make? (With
the currently-used JVM Version next to the "Use Java" checkbox?)

Gerv

Comment 51

[Pete Zha]

Gerv,
Thanks for the suggestion!
>The new UI is definitely an improvement on the old, so that's great. But were
>you not going to put the JVM author in the "version" column, rather than the
>mysterious letters "JRE"? 
OK, I will remove the "JRE"

>Also, it's still true that the star-in-last-column thing isn't consistent with
>any other UI we have in the product. It really does make sense for the selected
>JVM to be the one used, and for the control to be restricted to a single
>selection at all times. 
How about a radio box? I will try to add a radio at the first column. If I
can't, I will make current one be selected. OK?

>Lastly, your screenshot shows the UI with just one JVM in it. Do you plan to
>make it so that the choice UI only appears when there is a choice to make? (With
>the currently-used JVM Version next to the "Use Java" checkbox?)
So, the logic will like this:
if ((list.length == 0) and (it's the one we are using)) {
  don't show the panel
} else {
  show the panel
}
Since maybe user is using a plugin which he manualy add the plugin dir and it's
not the one we found in config file and default location.
Is this OK for you?

Comment 52

[Pete Zha]

Attachment: screen shot

UI changed:
1) remove "JRE", only show the version in the first column
2) make current java version be selected when the panel be shown
3) only show the panel if we have choice. Please see comment 51
4) when "Enable Java" check box not selected, the panel will not be shown
patch will be ready soon

Comment 53

[Pete Zha]

Attachment: patch v12

patch with UI changed as comment 52

Comment 54

[Pete Zha]

Comment on attachment 120796 [details] [diff] [review]
patch v12

alecf, please review the new patch which has UI changs

Comment 55

[Alec Flett]

Comment on attachment 120796 [details] [diff] [review]
patch v12

+	 var oldValue = prefs.getBoolPref("plugin.expose_full_path");
+	 prefs.setBoolPref("plugin.expose_full_path", true);
+
+	 var result;
+	 try {
+	   result =
navigator.mimeTypes["application/x-java-vm"].enabledPlugin.filename;
+	 } catch (e) { result = ""; };
+
+	 prefs.setBoolPref("plugin.expose_full_path", oldValue);
+
+	 return result;
+      }
+

this is nasty. It doesn't look like nsIDOMPlugin is frozen - can we add a
"fullPath" attribute, so that we don't have to go pref-flipping to get this
value?

Index: modules/oji/public/nsIJVMConfigManager.idl

This file needs WAY more documentation.. mostly what type
nsIJVMConfigList.getJVMConfigList is

+    nsIMutableArray* array;
+    rv = NS_NewArray(&array);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    if (mJVMConfigList.Count() > 0)
+	 mJVMConfigList.Enumerate(AppendJVMConfig, (void *)array);
+    else
+	 array = nsnull;
+
+    *_retval = (nsIArray*)array;
+    NS_IF_ADDREF(*_retval);

you're leaking the array! Please use nsCOMPtr<>!

+    nsCAutoString globalName("java.global_java_version_file");
+    nsCAutoString privateName("java.private_java_version_file");
+

where are these used?

+    if (hasPermission) {
+	 nsCAutoString srcPath;
+	mozPluginPath->GetNativePath(srcPath);

indentation

+	 if (currentJVMConfig && currentJVMConfig != oldJVMConfig) {
+	   jvmConfigMgr.setCurrentJVMConfig(currentJVMConfig);

I notice that setCurrentJVMConfig() can throw an exception if the system() call
fails.. shouldn't we throw up an error? For instance, what if the links that we
want to create are write-protected.


That said, why can't we store a pref to the actual path of the jvm, instead of
doing a symlink on disk, thus forcing the user to use a specific JVM in other
applications as well?

(for instance, what if the 1.4.2 JVM works better in mozilla but the 1.4.1 JVM
works better in  Opera? Are we breaking that?)

+    PRBool hasPermission = PR_FALSE;
+    pluginDir->IsWritable(&hasPermission);

oh wait! we fail silently if we don't have permission (we return NS_OK) seems
like an error should be thrown there too...

I'm just not happy about this symlinking

+    nsCAutoString compiler_line(ToNewCString(compiler));

another leaking ToNewCString() - please look over your patch for these, I see a
few.

+    nsJVMConfig* config = (nsJVMConfig*)mJVMConfigList.Get(&key);

please use NS_STATIC_CAST or NS_REINTERPRET_CAST (please look over your patch
for other C-style casts, there are a number of these)

you can do nsCAutoString key("mozilla") to save the assignment. Again, look
over the patch for more of these.

+    PRInt16 rvOffset = userAgent.Find("rv:");
+
+    if (rvOffset >= 0)
+	 _retval.Assign(Substring(userAgent, rvOffset + 3, 3));

why are you using PRInt16? Find() returns a PRInt32. Again, look over your
patch, I see lots of these, with FindChar() and so forth.

+    nsCAutoString defaultLocationName;
+
+#ifdef SUN_OS
+    defaultLocationName.Assign("java.default_java_location_solaris");
+#else
+    defaultLocationName.Assign("java.default_java_location_others");
+#endif

why are you using nsCAutoString? You just need the raw string.

+    nsAutoString empty;
+
+    nsStringKey key(aHomeDirName);
+    nsJVMConfig* config = (nsJVMConfig*)mJVMConfigList.Get(&key);
+    if (!config) {
+	 config = new nsJVMConfig(empty, type, empty, arch, path,
+				  mozPluginPath, empty);

instead of |empty| just pass in nsCString()

Index: modules/oji/src/nsJVMConfigManager.cpp
===================================================================

+#ifdef XP_UNIX
+// Implementation of nsJVMConfigManagerUnix


generally we like per-platform classes to be in their own file, so we don't
have to #ifdef out the whole file.

That said, it looks like a huge part of this could be shared by on all
platforms - perhaps you could make a "base" class and then derive per-platform
classes from it.

I'm also seeing a lot of:
+    PRBool TestNSVersion(nsILocalFile* aArchPath, nsAString& aNSVersion);
+
+    PRBool TestDir(nsILocalFile* aBaseDir, nsAString& aSubDir);
+

that could be made static - please look through the classes for other methods
which could be made static...

PLEASE look over your patch before you post it.   Please read
http://www.mozilla.org/projects/xpcom/string-guide.html
for details on how to use the string classes. 

as for the nsHashtable issue.. Given the above issues, I'm fine with sticking
with nsHashtable for now... there are bigger fish to fry.

Comment 56

[Gervase Markham [:gerv]]

Radios would also be fine; in that case, you would want to change it so that
rows are not selectable (highlightable.) Otherwise, you could have one row
selected with the radio and one with the highlight - and what would that mean?

Your list logic should be:

if ((list.length == 0) || (list.length == 1 && it's the one we are using)) {
  don't show the panel
} else {
  show the panel
}

although I don't know how it would be possible to have a list of length 1 and
have us not using that one.

Gerv

Comment 57

[Gervase Markham [:gerv]]

One more thing:

> 4) when "Enable Java" check box not selected, the panel will not be shown

If "Enable Java" is unchecked, and you would otherwise be showing the panel
(i.e. more than 1 JVM) then the panel should be _disabled_, not hidden.
Otherwise, ticking and unticking the checkbox will cause the panel to appear and
disappear, which would be ugly.

Gerv

Comment 58

[Pete Zha]

>although I don't know how it would be possible to have a list of length 1 and
>have us not using that one.
This could happen. If the user is using a java installation which not be
installed in default location and it didn't register itself into versions
file(For some old version of JRE, but new version of JRE will register itself
after install).

Comment 59

[Pete Zha]

Attachment: patch v13

>this is nasty. It doesn't look like nsIDOMPlugin is frozen - can we add a
>"fullPath" attribute, so that we don't have to go pref-flipping to get this
>value?
Yes, it's ugly. Can we resolve it in another bug? I will file a bug soon.

>Index: modules/oji/public/nsIJVMConfigManager.idl
>This file needs WAY more documentation.. mostly what type
>nsIJVMConfigList.getJVMConfigList is
I added some description in the idl file, please review it and correct me if
I'm wrong. Thanks!

>you're leaking the array! Please use nsCOMPtr<>!
Thanks for catching this. I'm changing to use nsComptr<>

>+    nsCAutoString globalName("java.global_java_version_file");
>+    nsCAutoString privateName("java.private_java_version_file");
>+
>where are these used?
removed

>+    if (hasPermission) {
>+	 nsCAutoString srcPath;
>+	mozPluginPath->GetNativePath(srcPath);
>indentation
fixed

>+	 if (currentJVMConfig && currentJVMConfig != oldJVMConfig) {
>+	   jvmConfigMgr.setCurrentJVMConfig(currentJVMConfig);
>I notice that setCurrentJVMConfig() can throw an exception if the system()
call
>fails.. shouldn't we throw up an error? For instance, what if the links that
we
>want to create are write-protected.
Yes, I'm catching the exception and let user know it failed.

>That said, why can't we store a pref to the actual path of the jvm, instead of

>doing a symlink on disk, thus forcing the user to use a specific JVM in other
>applications as well?
>(for instance, what if the 1.4.2 JVM works better in mozilla but the 1.4.1 JVM

>works better in  Opera? Are we breaking that?)
I think we can't store it in pref for two reasons:
1) The plugin is loaded before we load user's pref.
2) We shall store it in browser specific location since incompatible plugin
will cause mozilla crash on startup. For example, gcc3.2 built mozilla can't
work with gcc2.9x built java plugin.

>+    PRBool hasPermission = PR_FALSE;
>+    pluginDir->IsWritable(&hasPermission);
>oh wait! we fail silently if we don't have permission (we return NS_OK) seems
>like an error should be thrown there too...
I will return NS_ERROR_FAILURE if we don't have permission to write.

>I'm just not happy about this symlinking
Sorry for this. But it's the only solution right now. I will think about it and
get another better solution later.

>+    nsCAutoString compiler_line(ToNewCString(compiler));
>another leaking ToNewCString() - please look over your patch for these, I see
a
>few.
I cleaned up all the ToNewCString in this patch.

>+    nsJVMConfig* config = (nsJVMConfig*)mJVMConfigList.Get(&key);
>please use NS_STATIC_CAST or NS_REINTERPRET_CAST (please look over your patch
>for other C-style casts, there are a number of these)
fixed

>you can do nsCAutoString key("mozilla") to save the assignment. Again, look
>over the patch for more of these.
OK

>why are you using PRInt16? Find() returns a PRInt32. Again, look over your
>patch, I see lots of these, with FindChar() and so forth.
OK, I'm changing them to PRInt32

>why are you using nsCAutoString? You just need the raw string.
ok, changed to use char*

>instead of |empty| just pass in nsCString()
fixed

>generally we like per-platform classes to be in their own file, so we don't
>have to #ifdef out the whole file.
I separate them into different files: nsIJVMConfigManagerUnix.h/cpp

>That said, it looks like a huge part of this could be shared by on all
>platforms - perhaps you could make a "base" class and then derive per-platform

>classes from it.
Currently, I don't see other platform has chance to share code with Unix.
Because on windows, we could get java installation information from registry,
So this should be different with on Unix.

>that could be made static - please look through the classes for other methods
>which could be made static...
Yes, I changed them to static

>PLEASE look over your patch before you post it.   Please read
>http://www.mozilla.org/projects/xpcom/string-guide.html
>for details on how to use the string classes. 
Thanks! I really learn a lot about string from this patch.

Last problem. I'm using TARGET_MD_ARCH in Makefile.in to determine whether we
are on Unix. I'm not sure whether this is right because I didn't see any other
code is using this macro.

Gerv,
I can't disable a tree easier. I tried to set it's attribute "disabled" to true
but it still can be seelcted. Do you know any other place can disable a tree?
Thanks a lot!

Comment 60

[Pete Zha]

Comment on attachment 120811 [details] [diff] [review]
patch v13

alecf, Thanks for the great review. I learned a lot from your comments. Please
review this new one when you have time. Thanks again!

Comment 61

[Alec Flett]

Comment on attachment 120811 [details] [diff] [review]
patch v13

+    if (hasPermission) {
+	 nsCAutoString srcPath;
+	 mozPluginPath->GetNativePath(srcPath);
+
+	 nsCAutoString destPath;
+	 pluginDir->GetNativePath(destPath);
+
+	 char cmd[256];
+	 PR_snprintf(cmd, 256, "ln -sf %s %s",
+		     srcPath.get(), destPath.get());

I was thinking about this one actually - why don't we just call symlink()
instead of calling out to the system and running an external program?

+nsJVMConfigManagerUnix::ParseStream(nsILineInputStream* aStream)
+{
+    

a lot of the functions, especially this one, need more comments to explain what
they're doing - i.e. what's the format of each line? Is each line the same?
What assumptions are being made? Look at all the methods you have without ANY
comments.

+	     buffer.Assign(NS_LITERAL_STRING(""));

buffer.Truncate();

+    nsJVMConfig* config = (nsJVMConfig*)mJVMConfigList.Get(&key);

NS_STATIC_CAST please


+	 nsCOMPtr<nsIFile> entry;
+	 rv = entries->GetNext(getter_AddRefs(entry));
+	 NS_ENSURE_SUCCESS(rv, rv);
+

woah, aren't you getting assertions at runtime here? you should be passing in
an nsCOMPtr<nsISupports> and then calling do_QueryInterface() to get a nsIFile.

+#if (NS_COMPILER_GNUC32)
+    aNSVersion.Assign(versionStr);
+    aNSVersion.Append(NS_LITERAL_STRING("-gcc32"));
+    if (TestDir(aArchPath, aNSVersion)) return PR_TRUE;
+#else
+    aNSVersion.Assign(versionStr);
+    if (TestDir(aArchPath, aNSVersion)) return PR_TRUE;
+#endif

shouldn't this be:

+    aNSVersion.Assign(versionStr);
+#if (NS_COMPILER_GNUC32)
    aNSVersion.Append(NS_LITERAL_STRING("-gcc32"));
+#endif
+    if (TestDir(aArchPath, aNSVersion)) return PR_TRUE;

this pattern repeats all over this file

I'm sorry to have to keep going around on this but you need to read over your
own patch - there are a lot of obvious things here, and there have been for the
last few patches. These could have been caught much easier by you instead of
me.

Comment 62

[Alec Flett]

maybe I wasn't being clear in my earlier comments: there may be other things
that I haven't yet discovered in these patches. After fixing all of the above
comments, make a patch and read through it yourself before requesting another
review. spend some time pretending you are reviewing someone else's code. when
you are certain that the patch is in top form, then ask me for a review. If you
need to ask someone else for a review before coming to me, please do. I don't
mean to be harsh its just that I've been through 4 rounds with this bug so far
and found reasonable problems every time.

Comment 63

[Pete Zha]

>I was thinking about this one actually - why don't we just call symlink()
>instead of calling out to the system and running an external program?
Because I want to replace the symlink which is existing. I tried symlink and
seems it can't replace the symlink. If the dest file exists, the function will
return an error code. So, If I use it, I need to remove that file first and then
call symlink(). Do you think it's necessary?

Comment 64

[Mike Shaver (:shaver emeritus)]

Yes, or better yet use nsILocalFile's methods to unlink and then symlink.

Calling system() to ln is not the right thing.  (What do you want to do if the
path name exists, but is a directory?  Is that what ln will do for you?)

Seriously, though: if you're creating the symlink just to store the path name, I
think you should look into a way to load the profile at that point, or delay
initialization of the plugin until the profile is loaded.  This seems pretty gross.

Comment 65

[Pete Zha]

http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileUnix.cpp#837
We will copy the file when calling copyToFollowingLinks of nsILocalFile. So I
can't use that function. I will remove file and then use symlink directly.

Comment 66

[Pete Zha]

Attachment: patch v14

alecf,
>I was thinking about this one actually - why don't we just call symlink()
>instead of calling out to the system and running an external program?
OK, I'm now using the symlink to create the link. But before this, I delete the
old file first.

>a lot of the functions, especially this one, need more comments to explain
what
>they're doing - i.e. what's the format of each line? Is each line the same?
>What assumptions are being made? Look at all the methods you have without ANY
>comments.
I added some comments in the source file. I'm not sure if it's enough but I
think some detail information we can find in the proposal. I'm going to find a
place to store the proposal and link it in the source code. Do you think it's
ok?
>+	     buffer.Assign(NS_LITERAL_STRING(""));
>buffer.Truncate();
Fixed, And I found another place should use Truncate

>+    nsJVMConfig* config = (nsJVMConfig*)mJVMConfigList.Get(&key);
>NS_STATIC_CAST please
Fixed. And also I found another place should also fix this. Sorry for the
mistake.

>woah, aren't you getting assertions at runtime here? you should be passing in
>an nsCOMPtr<nsISupports> and then calling do_QueryInterface() to get a
nsIFile.
Fixed

>this pattern repeats all over this file
Yes, I checked other place and fixed them.

>I'm sorry to have to keep going around on this but you need to read over your
>own patch
No problem. Actually that's my fault... Please go head to review my new patch
if you have time. I did go through this new patch myself this time.

Comment 67

[Pete Zha]

Comment on attachment 121261 [details] [diff] [review]
patch v14

>+    char* temp = PR_GetLibraryName(nsnull, javaLibNameXPIDLValue.get());
>+    nsCAutoString pluginFileName(temp);
>+    testPath->AppendNative(pluginFileName);
Sorry, I should call PR_FreeLibraryName(temp) here to free it. I just read more
example code and found this problem.Alec, am I right?

Comment 68

[Alec Flett]

Comment on attachment 121261 [details] [diff] [review]
patch v14

ok, this is getting better and better... but you still need way more comments!
I think it is fine to link to a proposal that you describe, but even if the
proposal describes the file format, you need to comment in the file as to what
the code is trying to do - thanks for putting the comment near the ParseLine
declaration, but you also need it in the source.. for instance:

+PRBool
+nsJVMConfigManagerUnix::GetValueFromLine(nsAString& aLine, const char* aKey,
+					  nsAString& _retval)
+{
+    _retval.Truncate();
+    
+    nsAutoString line(aLine);
+    PRInt32 keyOffset = line.Find(aKey);
+
// make sure we have the key
+    NS_ENSURE_TRUE(keyOffset >= 0, PR_FALSE);
+
+    PRInt32 equalsOffset = aLine.FindChar('=', keyOffset);
+    NS_ENSURE_TRUE(equalsOffset >= 0, PR_FALSE);
+
// the keys are delimited with '|' so we know the value ends before the first
'|'

+    PRInt32 lineOffset = aLine.FindChar('|', equalsOffset);
+    lineOffset = lineOffset >= 0 ? lineOffset : aLine.Length();
+

// seperate out the value, removing the leading/trailing spaces
+    nsAutoString buffer(Substring(aLine,
+				   equalsOffset + 1,
+				   lineOffset - equalsOffset -1));
+    buffer.Trim(" ");
+    _retval = buffer;
+    return PR_TRUE;
+}

see, in commenting this function, it got me thinking:
- 'buffer' probably needs a more descriptive name, like 'value'
- what if the key given is only a substring of a key

for instance, what if I ask for the key 'type' - your code will match  either
'archtype=foo' or 'typename=foo' - is this what you indented (and if so, you
need to document that: "pass in a key or a substring of a key") - personally it
sounds like a bug, and a possible security exploit depending on how this data
is used!

a few more nits... in GetMozillaPluginPath():
+    if (_retval.IsEmpty()) {
+	 key.Assign("ns610.plugin.path");
+	 GetValueFromLine(aLine, key.get(), _retval);
+    }

why not just say GetValueFromLine(aLine, "ns610.plugin.path", _retval);

also you're propagating the error result 'rv' from GetNSVersion(), which always
succeededs (because of the NS_ENSURE_SUCCESS(rv, rv) right after it) - so the
"return rv;" at the end is really just "return NS_OK" - again if this was your
intent, document it 'always return success') but if it was not, please fix.. I
see other places like this in your code as well.

oh, this: 
+    nsAutoString defaultLocation;
+    defaultLocation.Assign(NS_ConvertUTF8toUCS2(defaultLocationXPIDLValue));

should probably be
NS_ConvertUTF8toUCS2 defaultLocation(defaultLocationXPIDLValue));

- NS_Convert* stuff are actually classes derived from nsAutoString -- I know
that isn't very obvious...

Comment 69

[Pete Zha]

Attachment: patch v15

>ok, this is getting better and better... but you still need way more comments!

I added more comments in the source file as I can.

>- 'buffer' probably needs a more descriptive name, like 'value'
Yes, it's much better than 'buffer'

>- what if the key given is only a substring of a key
Yes, you thinking is reasonable. Currently, the keys in the file format here is
fixed and it will not cause this problem.So, I think it's not a big problem
right now. I can fix this issue once the file format changed and could cause
this problem. Thanks for catching this!

>why not just say GetValueFromLine(aLine, "ns610.plugin.path", _retval);
Yes, It's more reasonable. Sorry for didn't catching this!

>also you're propagating the error result 'rv' from GetNSVersion(), which
always
>succeededs (because of the NS_ENSURE_SUCCESS(rv, rv) right after it) - so the
>"return rv;" at the end is really just "return NS_OK" - again if this was your

>intent, document it 'always return success') but if it was not, please fix.. I

>see other places like this in your code as well.
I just want to reuse the rv. But you are right, just return NS_OK is more
clear. I have modified the patch to return NS_OK if necessary.

>should probably be
>NS_ConvertUTF8toUCS2 defaultLocation(defaultLocationXPIDLValue));
>- NS_Convert* stuff are actually classes derived from nsAutoString -- I know
>that isn't very obvious...
Fixed.

Comment 70

[Roland Mainz]

nit:
-- snip --
+    nsCOMPtr<nsIPref> prefs(do_GetService(kPrefCID));
+    NS_ENSURE_TRUE(prefs, NS_ERROR_FAILURE);
-- snip --
caillon and others currently try to switch all consumers of the |nsIPrefs| API
over to the newer |nsIPrefsBranch| API as preparation to get rid of |nsIPrefs|.
Please avoid adding new code which uses |nsIPrefs| - use |nsIPrefsBranch| if
possible...

Comment 71

[Pete Zha]

Comment on attachment 121382 [details] [diff] [review]
patch v15

alecf, hope you will satisfy with this patch.
Thanks Roland,
I will change it in next patch. Do you know is there a bug for this?

Comment 72

[Roland Mainz]

Pete Zha wrote:
> I will change it in next patch. Do you know is there a bug for this?

See bug 175193 ("nsIPref is deprecated, and should be removed") ...

Comment 73

[Alec Flett]

Comment on attachment 121382 [details] [diff] [review]
patch v15

What happened to the option to add an attribute to nsIDOMPlugin? I never agreed
to waiting on that...

aside from that this is fine, though I haven't really evaluated the user
experience here. I thought we were going to do radio buttons or someting else
that gerv or someone was discussing?

Comment 74

[Pete Zha]

>What happened to the option to add an attribute to nsIDOMPlugin? I never agreed
>to waiting on that...
OK, I will file a bug or for this. Or I can modify patch in this patch.

>aside from that this is fine, though I haven't really evaluated the user
>experience here. I thought we were going to do radio buttons or someting else
>that gerv or someone was discussing?
In this patch, I'm using the selection to show the current java vm we are 
using. Radio button is OK, but I didn't find how to use radio button in tree 
control.

Anyway, next patch is coming.

Comment 75

[Alec Flett]

Comment on attachment 121382 [details] [diff] [review]
patch v15

ok, just add some comments in the place where you enable/disable the full-path
thing for plugins, and change the "switchJVM" string to

"The newly selected Java version will take effect when you restart %S"
(the old string wasn't gramatically correct)

sr=alecf with those changes.

Comment 76

[Pete Zha]

Attachment: patch v16

patch with following changes:
1) add comments around enable/disable pref to get full-path thing
2) change witchJVM to "The newly selected Java version will take effect when
you restart %S"
3) change to use nsIPrefBranch instead of nsIPref

Comment 77

[Pete Zha]

Attachment: diff between patch v15 and patch v16

Comment 78

[Pete Zha]

Comment on attachment 122821 [details] [diff] [review]
patch v16

Joshua, please review it. Thanks!

Alecf, please give sr= on this patch if you feel ok.

Comment 79

[Gervase Markham [:gerv]]

Where is that switchJVM string used? I hope it's not a pop-up which appears when
you change the selected JVM :-)

If changing JVM requires a restart, then the text for the control which selects
the JVM should say:
"Java version (change requires restart)"

Gerv

Comment 80

[Pete Zha]

>Where is that switchJVM string used? I hope it's not a pop-up which >appears when
>you change the selected JVM :-)
It will popup once the user click "OK" button of preference dialog.

This sentence is similar with you change the theme.

Comment 81

[Gervase Markham [:gerv]]

> It will popup once the user click "OK" button of preference dialog.

That is also not good UI; if the user needs to know this information, it should
be in the dialog where they can read it. I understand that themes uses it; but
now that you point it out, I can say that it sucks there too :-)

Obviously, the best fix would be to make it not require a restart. However, if
we can't do that, and if you need to regenerate this patch for any other reason,
could you please remove the popup, and change the text above the selection
widget to:

Select Java version (change requires restart)

?

Thanks for putting up with all these requests :-) The UI now is much better
because of your patience.

Gerv

Comment 82

[timeless]

I'll look at the patch over the weekend after i get back from my vacation, but
one quick note; this won't do what you want:
NS_ENSURE_SUCCESS(rv || rv == NS_ERROR_FILE_NOT_FOUND, rv);
consider this:
NS_ENSURE_TRUE(NS_SUCCEEDED(rv) || rv == NS_ERROR_FILE_NOT_FOUND, rv);

Comment 83

[Pete Zha]

Gerv, oh, I think remove the popup is better for me. Thanks for your suggestion.

timeless, Thanks a lot for catching this.

Comment 84

[Pete Zha]

Attachment: patch v17

changes:
1) remove the popup. Only alert when failed to change the java version.
2) fix the problem which timeless mentioned.

Comment 85

[Pete Zha]

Attachment: patch for i18n

This patch is for i18n changes only.

Comment 86

[Alec Flett]

Comment on attachment 122984 [details] [diff] [review]
patch v17

thanks for switching to nsIPrefBranch & friends.
sr=alecf

Comment 87

[Pete Zha]

Comment on attachment 122985 [details] [diff] [review]
patch for i18n

I want to land this into 1.4 since we need this feature in Sun's next release
of mozilla which maybe will based on mozilla1.4. This is i18n related only and
it's a part of patch v17, so I just carry r/sr here.

Comment 88

[Christopher Blizzard (:blizzard)]

Comment on attachment 122985 [details] [diff] [review]
patch for i18n

a=blizzard for 1.4 on this patch

Comment 89

[Pete Zha]

Comment on attachment 122985 [details] [diff] [review]
patch for i18n

Thanks, blizzard.
I just checked in this patch.

Comment 90

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 122984 [details] [diff] [review]
patch v17

Sorry for being late here, but there are a couple issues I'd like to point out.
 (I have a few other nits, such as more grammar nits, and some code size nits
but I will let them slide since they are already in the tree.  I would like to
see the following addressed though).



>Index: modules/oji/public/nsIJVMConfigManager.idl

>+ * The Initial Developer of the Original Code is Sun Microsystem.

I think your company is called "Sun Microsystems" (with an s) so for legal
reasons you might want to fix this here and in the several other places this
occurs.  And for future reference, use the boilerplate in which you only have
to type the company name once per file.  See
http://www.mozilla.org/MPL/boilerplate-1.1/mpl-c .  The only things you need to
touch are the 3 places with ____________, and the contributors section.


>Index: modules/oji/src/nsJVMConfigManagerUnix.h

>+    // The table to store the config list.
>+    nsHashtable mJVMConfigList;

FYI, nsHashtable is deprecated in favor of the new templatized hashtables...


>Index: modules/oji/src/nsJVMConfigManagerUnix.cpp

>+#define NS_COMPILER_GNUC32 __GNUC__ == 3 && __GNUC_MINOR__ == 2

Um, this is not really cool.  gcc3.3 is already out for example and it should
share the same code path since it is ABI compatible with gcc3.2.  You probably
want to do something like what xptcall did here (although you may want to use a
different __GXX_ABI_VERSION):
http://lxr.mozilla.org/seamonkey/source/xpcom/reflect/xptcall/src/md/unix/xptci
nvoke_unixish_x86.cpp#110

Anyway, could/should this be a configure check?


>+nsresult
>+nsJVMConfigManagerUnix::ParseLine(nsAString& aLine)
>+{
>+    nsresult rv = NS_OK;
>+
>+#if (NS_COMPILER_GNUC32)
>+    nsAutoString compiler;
>+    GetValueFromLine(aLine, "compiler", compiler);
>+
>+    NS_ENSURE_TRUE(compiler.Find("gcc32") >= 0, NS_OK);

Again, what about newer gccs?  Also, you should check against kNotFound, not 0.


....

>+    PRInt32 rvOffset = userAgent.Find("rv:");
>+
>+    if (rvOffset >= 0)

Can you change this to rvOffset != kNotFound ?


>+PRBool
>+nsJVMConfigManagerUnix::GetValueFromLine(nsAString& aLine, const char* aKey,
>+                                         nsAString& _retval)

There are a bunch of Find()s in this method that could use |kNotFound|s as
well... It makes the code much more readable and is less error-prone.

Comment 91

[Pete Zha]

Attachment: patch v18

patch address Christopher's comments.
>FYI, nsHashtable is deprecated in favor of the new templatized hashtables...
Since nsTHashtable is new, I would like to fix it later.

>Anyway, could/should this be a configure check?
I'm not sure. But currently this solution is ok I think.

>(I have a few other nits, such as more grammar nits, and some code size nits
>but I will let them slide since they are already in the tree.
Can you point out the problem? I can fix it later.

Christopher,
Thanks a lot for your comments! Can you review it when you have time?

Comment 92

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 123871 [details] [diff] [review]
patch v18

Oh I think I misread, and thought that this specific patch (or the previous
incarnation of it) landed already.  I guess it hasn't, upon further reading. 
In that case, and since you asked for them, I'll offer the other nits I spoke
about as well, and take a closer look here :)

I would also like to thank you for also changing the license to tri-license.  I
just found out that Sun has permitted that, and was going to comment on that,
but found you had already done that in your patch -- I'm glad that you took the
extra step.

>Since nsTHashtable is new, I would like to fix it later.

I can live with that.  I just wanted you to be aware of it.  Make sure you use
it next time.

And now for my comments (I took a longer look this time around):


>Index: xpfe/components/prefwindow/resources/content/pref-advanced.xul

>+    const jvmConfigMgr = getJVMConfigMgr();
>+    const jvmConfigList = getJVMConfigList();

Because of the two previous lines, you will end up getting the
nsIJVMConfigManager service at least twice in success cases.  Your
getJVMConfigMgr() function below should check to see if there is already a
cached version of itself.  Specifically, see what xpfe does for getBrowser()
etc.  You probably want a global variable that is set by that method.  E.g.,

  function getJVMConfigMgr() {
    if (gJVMConfigMgr) {
      return gJVMConfigMgr;
    }

    try { ... } catch (e) { ... }; // from your existing code.
  }


And I also have to ask, why the need for the try/catch?  Will that ever throw
an exception?  You get other services in this manner without a try/catch.  I'm
not so sure it needs it, really.

>+    function getJVMConfigList() {
>+      try {
>+        return getJVMConfigMgr().getJVMConfigList();
>+      } catch (e) {
>+        return null;
>+      }
>+    }

This would probably be cleaner written with ifs and returns.  The try/catch
seems IMO unnecessary.


>Index: modules/oji/public/nsIJVMConfigManager.idl

>+/**
>+ * This interface is a manager can get information of java installations
>+ */
>+[scriptable, uuid(ca29fff1-a677-493c-9d80-3dc60432212b)]
>+interface nsIJVMConfigManager : nsISupports {

"This interface is a manager that can get information about Java
installations." is corrected grammar.


>+    // This function return a list of existing java installation.

"This function returns a list of existing Java installations."

>+    nsIArray getJVMConfigList();
>+
>+    // This function will let the browser use specific java installation
>+    void setCurrentJVMConfig(in nsIJVMConfig jvmConfig);

"This function will let the browser use a specific Java installation."

Although for the last one, I think a more appropriate comment would be:

"This function tells the browser to use a specific Java installation."


>Index: modules/oji/src/nsJVMConfigManagerUnix.h


>+    // Parse a stream which may contains several
>+    // java installation informations


"Parse a stream for information about Java installation(s)." perhaps?


>+    nsresult ParseStream(nsILineInputStream* aStream);
>+
>+    // Parse a line which contains a java installation information
>+    // and append it to the list
>+    // A line looks like this:
>+    // "version=1.4.2 | type=jre | os=linux | arch=i386 | ......"

"Parse a line for information about a Java installation, and ensure it's
JVMConfig is in our JVMConfigList."



>+    nsresult ParseLine(nsAString& aLine);
>+
>+    // Search java installation in default location

"Search for Java installations in the default location."

>+    nsresult SearchDefault();
>+
>+    // Search a specific directory to see if there are java installations

Wanna capitalize Java and add a period?

>+    nsresult SearchDirectory(nsAString& aDirName);
>+
>+    // Add a directory to the java configuration list if
>+    // it contains a java installation

Same here.

>+    nsresult AddDirectory(nsIFile* aHomeDir);
>+    
>+    nsresult AddDirectory(nsAString& aHomeDirName);
>+
>+    // Get a value by specifix key from the line. A line should like this:

"specific"

"A line should look like this:"



>+
>+    // Test if arch directory is existing

"Test if the arch directory exists."

>+    static PRBool TestArch(nsILocalFile* aPluginPath, nsAString& aArch);
>+
>+    // Test if NS version directory is existing

"Test if the NS version directory exists."

>+    static PRBool TestNSVersion(nsILocalFile* aArchPath, nsAString& aNSVersion);
>+
>+    // Test if a specific node in the base directory is existing

And apply the last two comments to this as well.


>Index: modules/oji/src/nsJVMConfigManagerUnix.cpp


>+#define NS_COMPILER_GNUC3 (__GXX_ABI_VERSION) && \
>+                          (__GXX_ABI_VERSION >= 100) /* G++ V3 ABI */

Please use defined(__GXX_ABI_VERSION) rather than just (__GXX_ABI_VERSION). 
Also note that version 100 is the 3.0 ABI.  You may (or may not) want to update
that to 102 which is the 3.2 ABI.



>+PR_STATIC_CALLBACK(PRBool)
>+FreeJVMConfig(nsHashKey *aKey, void *aData, void* aClosure)
>+{
>+    nsJVMConfig* config = NS_STATIC_CAST(nsJVMConfig *, aData);
>+    NS_ENSURE_TRUE(aKey && config, PR_FALSE);
>+
>+    NS_IF_RELEASE(config);


I'm not exactly sure why you are failing to free the JVMConfig if there is no
key....

>+
>+    return PR_TRUE;
>+}
>+


>+NS_IMETHODIMP
>+nsJVMConfigManagerUnix::GetJVMConfigList(nsIArray **_retval)
>+{
>+    NS_ENSURE_ARG_POINTER(_retval);
>+
>+    ClearJVMConfigList();
>+    InitJVMConfigList();
>+
>+    nsresult rv = NS_OK;
>+
>+    nsCOMPtr<nsIMutableArray> array;
>+    rv = NS_NewArray(getter_AddRefs(array));


  nsresult rv = NS_NewArray(...);


>+NS_IMETHODIMP
>+nsJVMConfigManagerUnix::InitJVMConfigList()
>+{
>+    nsresult rv = NS_OK;

Same as previous comment.

>+
>+    nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
>+    NS_ENSURE_TRUE(prefs, NS_ERROR_FAILURE);
>+
>+    nsCOMPtr<nsILocalFile> globalFile;
>+    prefs->GetComplexValue("java.global_java_version_file",
>+                           NS_GET_IID(nsILocalFile),
>+                           getter_AddRefs(globalFile));
>+
>+    nsCOMPtr<nsILocalFile> privateFile;
>+    prefs->GetComplexValue("java.private_java_version_file",
>+                           NS_GET_IID(nsILocalFile),
>+                           getter_AddRefs(privateFile));

In the case where getting the global file fails and would cause you to return
early below, you probably don't want to get the private file's location from
prefs.	Maybe move this down a bit?


>+    nsCOMPtr<nsILineInputStream> globalStream;
>+    rv = GetLineInputStream(globalFile, getter_AddRefs(globalStream));
>+    NS_ENSURE_TRUE(NS_SUCCEEDED(rv) || rv == NS_ERROR_FILE_NOT_FOUND, rv);
>+
>+    nsCOMPtr<nsILineInputStream> privateStream;
>+    rv = GetLineInputStream(privateFile, getter_AddRefs(privateStream));
>+    NS_ENSURE_TRUE(NS_SUCCEEDED(rv) || rv == NS_ERROR_FILE_NOT_FOUND, rv);
>+
>+    rv = InitJVMConfigList(globalStream, privateStream);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Search java installation in default install location

"Search for a Java installation in the default install location."

>+    return SearchDefault();
>+}
>+
>+NS_IMETHODIMP
>+nsJVMConfigManagerUnix::SetCurrentJVMConfig(nsIJVMConfig* aJVMConfig)
>+{
>+    NS_ENSURE_ARG_POINTER(aJVMConfig);
>+
>+    nsresult rv = NS_OK;

Again, just declare this where you really use it.  I see this several other
times.	I won't comment further on them, but please fix them.

>+
>+    nsCOMPtr<nsIFile> srcFile;
>+    rv = aJVMConfig->GetMozillaPluginPath(getter_AddRefs(srcFile));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCOMPtr<nsIFile> pluginDir;
>+    rv = NS_GetSpecialDirectory(NS_APP_PLUGINS_DIR, getter_AddRefs(pluginDir));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    PRBool hasPermission = PR_FALSE;
>+    pluginDir->IsWritable(&hasPermission);
>+    if (hasPermission) {

Could this be turned into:

  if (!hasPermission) {
    return NS_ERROR_FAILURE;
  }

  ...


>+nsresult
>+nsJVMConfigManagerUnix::ParseStream(nsILineInputStream* aStream)
>+{
>+    NS_ENSURE_ARG_POINTER(aStream);
>+
>+    nsresult rv = NS_OK;
>+
>+    PRBool notEOF = PR_TRUE;
>+
>+    nsAutoString lineBuffer;
>+    while (notEOF) {

You know this will always iterate at least once.  How about do{}while(); ?


>+nsresult
>+nsJVMConfigManagerUnix::ParseLine(nsAString& aLine)
>+{
>+    nsresult rv = NS_OK;
>+
>+#if (NS_COMPILER_GNUC3)
>+    nsAutoString compiler;
>+    GetValueFromLine(aLine, "compiler", compiler);
>+
>+    NS_ENSURE_TRUE(compiler.Find("gcc32") != kNotFound, NS_OK);

Does this need to be adapted for other gcc3.x series compilers as well?  If
not, let's hope that gcc never comes out with a 32.0 ;-)



>+    if (!config) {
>+        config = new nsJVMConfig(version, type, os, arch, path,
>+                                 mozPluginPath, description);
>+        NS_ENSURE_TRUE(config, NS_ERROR_OUT_OF_MEMORY);
>+        mJVMConfigList.Put(&key, NS_STATIC_CAST(void *, config));
>+        NS_IF_ADDREF(config);

You know that config exists here.  NS_ADDREF will suffice.

>+    }
>+    
>+    return NS_OK;
>+}
>+

>+nsresult
>+nsJVMConfigManagerUnix::GetAgentVersion(float* _retval)
>+{

...

>+    rv = p->GetAsFloat(_retval);
>+    return rv;

How about just |return p->GetAsFloat(..);|

>+}
>+



>+nsresult
>+nsJVMConfigManagerUnix::SearchDefault()
>+{
>+#ifdef SUN_OS
>+    char* defaultLocationName = "java.default_java_location_solaris";
>+#else
>+    char* defaultLocationName = "java.default_java_location_others";
>+#endif

Minor nit: Care to make those chars const?

>+
>+    nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
>+    NS_ENSURE_TRUE(prefs, NS_ERROR_FAILURE);
>+
>+    nsXPIDLCString defaultLocationXPIDLValue;
>+    prefs->GetCharPref(defaultLocationName,
>+                        getter_Copies(defaultLocationXPIDLValue));

Weird spacing above.  Please fix that.


>+nsresult
>+nsJVMConfigManagerUnix::AddDirectory(nsAString& aHomeDirName)

...

>+    if (!config) {
>+        config = new nsJVMConfig(version, type, nsString(), arch, path,
>+                                 mozPluginPath, nsString());
>+        NS_ENSURE_TRUE(config, NS_ERROR_OUT_OF_MEMORY);
>+        mJVMConfigList.Put(&key, NS_STATIC_CAST(void *, config));
>+        NS_IF_ADDREF(config);

NS_ADDREF

>+    }
>+
>+    return NS_OK;
>+}
>+
>+PRBool
>+nsJVMConfigManagerUnix::TestArch(nsILocalFile* aPluginPath, nsAString& aArch)
>+{
>+#ifdef SUN_OS
>+    aArch.Assign(NS_LITERAL_STRING("sparc"));
>+#else
>+    aArch.Assign(NS_LITERAL_STRING("i386"));
>+#endif
>+    return TestExists(aPluginPath, aArch);
>+}
>+
>+PRBool
>+nsJVMConfigManagerUnix::TestNSVersion(nsILocalFile* aArchPath,
>+                                      nsAString& aNSVersion)
>+{
>+    nsAutoString versionStr;
>+    nsresult rv = GetNSVersion(versionStr);
>+    NS_ENSURE_SUCCESS(rv, PR_FALSE);
>+
>+    aNSVersion.Assign(versionStr);
>+#if (NS_COMPILER_GNUC3)
>+    aNSVersion.Append(NS_LITERAL_STRING("-gcc32"));

Not all NS_COMPILER_GNUC3 compilers will be gcc3.2 (Your current macro will
grab the 3.x, 4.x, and beyond series of gcc).

Comment 93

[Mike Shaver (:shaver emeritus)]

> "Parse a line for information about a Java installation, and ensure it's
> JVMConfig is in our JVMConfigList."

"its JVMConfig"

> "Test if the arch directory exists."

We always test, because we don't know if the arch directory exists.  (Compare:
"frotz the foo if the arch directory exists.")  Suggestion:

"Check for existing arch directory."

Comment 94

[Christopher Aillon (sabbatical, not receiving bugmail)]

Rightio.  Thanks, Mike for catching my errors.  :-)

Comment 95

[Roland Mainz]

Nit:
-- snip --
+#ifdef SUN_OS
+    aArch.Assign(NS_LITERAL_STRING("sparc"));
+#else
+    aArch.Assign(NS_LITERAL_STRING("i386"));
+#endif
-- snip --
Where does the |SUN_OS| come from ? IMHO Sun's OS (="Solaris") runs on SPARC and
x86 (and more platforms may come in the future and were used in the past - like
"PPC").
What about using the matching compiler CPP symbols for it (assuming they are
allowed by mozilla.org policy, otherwise you have to use |TARGET_CPU| in
Makefile.in and set -D flags manually based on that value):
__sparc (SPARC)
__sparcv9 (SPARC with -xarch=v9|v9a|v9b)
__i386 (x86)

Comment 96

[Pete Zha]

Roland, I'm going to use the SPARC macro. Is it OK?
#ifdef SPARC
...

Alecf,
>Not all NS_COMPILER_GNUC3 compilers will be gcc3.2 (Your current macro will
>grab the 3.x, 4.x, and beyond series of gcc).
Now, we will put new gcc(after gcc3.2) compiled plugin into gcc32 directory.
Seems that directory's name is not very sensitive, but currently we don't know
the gcc's direction, So we want fix it to more sensitive name based on gcc's
changes in the future. Later, I will make a map to store the compatibility
information like Xiaobin suggested early in this bug.

Comment 97

[Pete Zha]

Attachment: patch v19

patch with you guys comments.
Please review it that I can check it in. Thanks!

Comment 98

[Pete Zha]

Attachment: diff between v18 and v19

Comment 99

[timeless]

Comment on attachment 124528 [details] [diff] [review]
patch v19

>Index: xpfe/components/prefwindow/resources/content/pref-advanced.xul
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/components/prefwindow/resources/content/pref-advanced.xul,v
>retrieving revision 1.77
>diff -u -r1.77 pref-advanced.xul
>--- xpfe/components/prefwindow/resources/content/pref-advanced.xul	28 Feb 2003 07:00:38 -0000	1.77
>+          if (column == "name")
>+            return jvmConfig.version;
else after v return ^
>+          else if (column == "home")
>+            return jvmConfig.path.path;
>Index: modules/oji/public/nsIJVMConfigManager.idl
>+/**
>+ *This interface is a manager that can get information about Java
>+ *installations." is corrected grammar.
		  ^^^ um, you weren't supposed to copy this part.
>+ */
please use: 
/**
 * First line...
 * second line...
 * Note the space between the asterisks and the text.
 */

>+[scriptable, uuid(3e333e20-b190-42d8-b993-d5fa435e46c4)]
>+interface nsIJVMConfig : nsISupports {
...
>+};
>+
>+/**
>+ * This interface is a manager can get information of java installations
     ^^ wasn't this the line with the bad grammar?
>+ */
>+[scriptable, uuid(ca29fff1-a677-493c-9d80-3dc60432212b)]
>+interface nsIJVMConfigManager : nsISupports {
>+    // This function returns a list of existing Java installations.
please use /** ... * comments ... */ instead of // comments
>+    nsIArray getJVMConfigList();
>+
>+    // This function tells the browser to use a specific Java installation.
<same>
>+    void setCurrentJVMConfig(in nsIJVMConfig jvmConfig);
>+};

>Index: modules/oji/src/nsCJVMManagerFactory.cpp
>@@ -40,12 +40,20 @@
...
> /*
>  * Note:  In revision 1.17 of this file (and earlier) there was a
>  * commented out implementation of nsCJVMManagerFactory, a hand-crafted
>  * implementation of nsIFactory.
>  */
This comment ^^ needs to be updated
>+#ifdef XP_UNIX
>+NS_GENERIC_FACTORY_CONSTRUCTOR(nsJVMConfigManagerUnix)
>+#endif
> 
> // The list of components we register
> static const nsModuleComponentInfo components[] = 
>@@ -61,6 +69,13 @@
>       "@mozilla.org/oji/jvm-auth-tools;1", 
>       nsJVMAuthTools::Create
>     },
>+#ifdef XP_UNIX
>+    { "JVM Config Manager",
>+      NS_JVMCONFIGMANAGER_CID,
>+      "@mozilla.org/oji/jvm-config-mgr;1",
>+      nsJVMConfigManagerUnixConstructor
>+    },
>+#endif
> };
> 
> NS_IMPL_NSGETMODULE(nsCJVMManagerModule, components);
>Index: modules/oji/src/nsJVMConfigManager.cpp
>+nsJVMConfig::nsJVMConfig(const nsAString& aVersion, const nsAString& aType,
>+                         const nsAString& aOS, const nsAString& aArch,
>+                         nsIFile* aPath, nsIFile* aMozillaPluginPath,
>+                         const nsAString& aDescription) :
please put the initializers one to a line:
>+mVersion(aVersion), mType(aType), mOS(aOS), mArch(aArch), mPath(aPath),
>+mMozillaPluginPath(aMozillaPluginPath), mDescription(aDescription)
>+{
>+}
>Index: modules/oji/src/nsJVMConfigManagerUnix.cpp

>+    if (mJVMConfigList.Count() > 0)
>+        mJVMConfigList.Enumerate(AppendJVMConfig,
>+                                 NS_STATIC_CAST(void *, array));
>+    else
>+        array = nsnull;
>+    *_retval = NS_STATIC_CAST(nsIArray *, array);
>+    NS_IF_ADDREF(*_retval);

i'd rather you not cast (nsIArray *) (0) and then if (0) ->AddRef

Instead:
+    if (mJVMConfigList.Count() > 0) {
+	 nsresult rv = mJVMConfigList.Enumerate(AppendJVMConfig,
+						NS_STATIC_CAST(void *, array));
+	 *_retval = NS_STATIC_CAST(nsIArray *, array);
+	 NS_ADDREF(*_retval); 
+    } else
+	 array = nsnull;

Comment 100

[Pete Zha]

>>+            return jvmConfig.version;
>else after v return ^
What's the problem here? It's in JS. I don't think it will cause any problem if
there is a else after return. Or I misundstood your meaning?

>This comment ^^ needs to be updated
This comment is talking about something in version 1.17. I don't think I need to
change it.

>please put the initializers one to a line:
>>+mVersion(aVersion), mType(aType), mOS(aOS), mArch(aArch), mPath(aPath),
>>+mMozillaPluginPath(aMozillaPluginPath), mDescription(aDescription)
>>+{
I used two lines because the line will be too long(more than 80 chars) if I put
them in one line. Is there any important reason to do that?

Other parts I will modified them in another patch. Thanks for the review.

Comment 101

[Pete Zha]

Attachment: patch v20

fix timeless's comments

Comment 102

[Pete Zha]

Alecf,

Could you take a look at the last patch again since there are some changes after
you give sr= on it. I'd like to let you give sr= on it before I check in. Or you
could tell me whether I can carry the sr= to the last patch.

Thanks
Pete

Comment 103

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 125489 [details] [diff] [review]
patch v20

One last round of comments from me Pete.  No need for a new patch, just make
sure you make the changes in your tree before you check it in.

>Index: xpfe/components/prefwindow/resources/content/pref-advanced.xul

>+    function getCurrentJVMPluginPath() {
>+      var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                    .getService(Components.interfaces.nsIPrefBranch);
>+
>+      if (prefs) {
>+        // Since we don't have fullPath attribute in nsIDOMPlugin,

Add an "a" in between "have" and "fullPath".

>+        // so we have to set the set pref "plugin.expose_full_path"

Please remove the "so" on the above line.

>+        // to true and then get it. Related bug is 204476
>+        var oldValue = prefs.getBoolPref("plugin.expose_full_path");


Also, it would be a good idea to not set the pref to itself twice.  In other
words, if the pref is _already_ set to true, you should not set it in either
place below.  Please check "oldValue" and only set the pref if it is false.


>+        prefs.setBoolPref("plugin.expose_full_path", true);
>+
>+        var result;
>+        try {
>+          result = navigator.mimeTypes["application/x-java-vm"]
>+                     .enabledPlugin.filename;
>+        } catch (e) { result = ""; };
>+
>+        // Set the pref to orignial value.

Typo.  "original"

>+        prefs.setBoolPref("plugin.expose_full_path", oldValue);

And of course, make sure you add the right check here as well (only set it if
the oldValue is false)

>+        return result;
>+      }
>+
>+      return "";
>+    }

Comment 104

[Pete Zha]

Thanks, Christopher. I will address your comments when checkin.

Comment 105

[Alec Flett]

Comment on attachment 125489 [details] [diff] [review]
patch v20

sr=alecf - yay!

Comment 106

[Pete Zha]

checked in trunk

Comment 107

[Jon Levell]

I'll file an additional bug if I'm not being stupid, but I haven't tested this
patch, only seen the screenshots - sorry for the spam if I'm missing something?

Wouldn't it make sense to have a "browse" button that pops up a file requester
so the user can select the location of a a JRE that's not in a standard place or
from Sun?

If Mozilla can't find the JRE (e.g. according to comment 43 because it's not
from Sun?!?) then the user is not helped by this patch and maybe confused?

Comment 108

[Peter Lubczynski]

*** Bug 209872 has been marked as a duplicate of this bug. ***

Comment 109

[Peter Lubczynski]

this isn't working for me. I've got today's 8am linux build, I see the prefs in
the unix js file but I don't see any new UI under the advanced prefs tab despite
having "Enable Java" checked.

I've got two JRE's on my Mandrake 9.1 system. I got the tar/gz of the SDK and
put them in:
/usr/local/j2sdk1.4.1_02
/usr/local/j2sdk1.4.2

There is no file named "versions" in these directories nor in /etc/.java or in
~/.java.

So I did a:
ln -s /usr/local/j2sdk1.4.2 /usr/java and still doesn't appear. How do I use
this feature?

Also, question: will this make installing the plugin for the right compiler
easier? Since trunk daily builds are now all built with gcc3, will the right
plugin be automatically selected and incompatibile ones filtered out?

Comment 110

[Pete Zha]

>If Mozilla can't find the JRE (e.g. according to comment 43 because it's not
>from Sun?!?) then the user is not helped by this patch and maybe confused?
Yes and No. If other java plugin supply versions file, we could use that file to
find it's library.

I know why this don't work for you. Currently, I don't fallback to use the
plugin in ns610-gcc32 directory. Because for mozilla1.3 or later, we should use
library in ns7-gcc32 directory. I will file another bug to do the fallback.
Currently, you may try to rename or copy the ns610-gcc32 directory to ns7-gcc32
to see the effect.

Comment 111

[Pete Zha]

bug 210042 has been filed for the fallback

Comment 112

[Neil Durant]

Are there any plans for this kind of functionality to be added to the Windows
version of Mozilla in the near future?  Even if the UI side wasn't available,
the underlying framework for specifying a JRE to use would be extremely useful
to work we're doing in our project!

Comment 113

[seikamaco]

Comment on attachment 120046 [details]
Browser side java integration

><html>
><head>
>	<meta HTTP-EQUIV="CONTENT-TYPE" CONTENT="text/html; charset=iso-8859-1">
>	<title>Browser side Java Integration proposal</title>
>	<meta NAME="GENERATOR" CONTENT="StarOffice 6.0  (Solaris Sparc)">
>	<meta NAME="CREATED" CONTENT="20030102;10473500">
>	<meta NAME="CHANGED" CONTENT="20030108;13572500">
></head>
><body LANG="en-US">
><h1 align=CENTER><b>Browser side Java integration proposal.</b></h1>
><pre width=100%>
>
><b>Purpose</b>
>This proposal specifies how the Browser is integrated with Java. The purpose of these changes is to make Browser works better with Java.
>Please refer to following document for Java side proposal.
><a href="http://web-east.east/~sbohne/mantis/madhatter-java-install.html">http://web-east.east/~sbohne/mantis/madhatter-java-install.html</a>
>
><b>1. Detect exists Java installation</b>
>
><b>1.1. JRE has already been installed on system by root(system-wide). Any user on this system can use it.</b>
>
>- Following config files should be created/updated by JRE.
>/etc/mime.types - store the mime type of special extension
>/etc/mailcap - store information how to deal with the special mime type
>/etc/.java/versions - store information of all JRE/JDK installed on system
>
>- Browser:
>
>  Preference. Browser should have a preference associated with it. So the user/administrator could customize the location of these file.
>
>pref("helpers.global_mime_types_file", "/etc/mime.types");
>pref("helpers.global_mailcap_file", "/etc/mailcap");
>pref("helpers.global_plugin_path_file", "/etc/.java/versions");
>
>  Runtime. Browser should provide a UI(in preference dialog) to let user choose which java plugin the he/she wants to use. This dialog can be popup when the user first time start Browser and we didn't let user choose plugin ate install time(Since packadd/rpm install can't popup dialog to let user choose). Of course, the user can open this dialog to config plugin at anytime he wants when he is using Browser.
>+----------------------------------------------------+
>|Preference - Java                                 |X|
>+----------------------------------------------------+
>|                                                    |
>|  @ Use Existing Java Plugin:                       |
>|  +---------------------------------------------+   |
>|  |/usr/java/j2sdk1.2.1/jre/plugin...         |^|   |
>|  |/usr/java/j2sdk1.3.1/jre/plugin...         |=|   |
>|  |/usr/java/j2sdk1.4.1/jre/plugin...         | |   |
>|  |...                                        |v|   |
>|  +---------------------------------------------+   |
>|                                                    |
>|  O Do not use Java Plugin                          |
>|                                                    |
>|                             +------+  +--------+   |
>|                             |  OK  |  | Cancel |   |
>|                             +------+  +--------+   |
>+----------------------------------------------------+
>
>
><b>1.2. JRE has already been installed by non-root user on private location. Other user can't use it.</b>
>
>- Following config files should be created/updated by JRE.
>~/.mime.types
>~/.mailcap
>~/.java/versions
>
>- Browser:
>
>  Preference. Browser also should have preference associated with it.
>
>pref("helpers.private_mime_types_file", "~/.mime.types");
>pref("helpers.private_mailcap_file", "~/.mailcap");
>pref("helpers.private_plugin_path_file", "~/.java/versions");
>
>  Browser will read both(system-wide and user-specific) config files and try to use user-specific config information to override the system-wide one if needed. So,
> Final config = system-wide config + user-specific config
> If there is overlap, we will use user-specific config.
>
><b>1.3. JRE has been bundled with browser's installer</b>
>
>WebStar(Java based Installer):
>+----------------------------------------------------+
>|Browser Installation (Java)                       |X|
>+----------------------------------------------------+
>|                                                    |
>|  @ Install and use Bundled Java (JRE1.4.2)         |
>|  +---------------------------------------------+   |
>|  |/home/pete/java/j2re1.4.2                    |   |
>|  +---------------------------------------------+   |
>|                                                    |
>|  O Do not install Java                             |
>|                                                    |
>|                              +------+   +------+   |
>|                              | Back |   | Next |   |
>|                              +------+   +------+   |
>+----------------------------------------------------+
>- Since we will bundle the latest JRE, we will not let user select JRE from this installer. The user could select other JRE after install browser. (Preference-&gt;Advanced)
>
>- User could change the directory he wants to install the bundled JRE.
>
><b>1.4. JRE has not been installed(on both system-wide or user specify location or plugins directory).</b>
>
>- Browser can't find/recognize any config files in system directory and user's home directory and can't find plugin in browser's plugins directory.
>
>- Browser will redirect user to Netscape/Sun website to download the JRE.
>
>- After download and install the JRE, the browser can find config file just like 1) or 2)
>
><b>2. When JRE is upgraded</b>
>
><b>2.1 system-wide JRE is upgraded</b>
>
>- JRE installer should update the system-wide config file.
>
>- User can choose the new version of plugin from preference dialog as mentioned in 1.1.
>
><b>2.2. User private JRE is upgraded</b>
>
>- JRE installer should update the config files in user's home directory
>
>- User can choose the new version of plugin from preference dialog as mentioned in 1.1.
>
><b>2.3. Notify.</b>
>
>- Browser should detect the upgrade of JRE and prompt the user to use the newer version when the user is restarted after new JRE is installed.
>To achieve this, We can detect whether "versions" file has been updated or compare the plugin path to find out whtther there is a new path is added.
>
><b>3. When the browser is upgraded</b>
>
>- How to do the version check.
>A simply way is to add a config file at plugins' directory, Browser can parse that file and check the version.
>
>Because this issue is too broad and uncertain for now, especially compatiblity may means not only browser version, but also compiler used to build the browser. We will not address it in current proposal.
>
><b>4. Issues</b>
>
><b>4.1. Exception handle:</b>
>
>- If browser fails to locate plugin according to config files(such as, JPI library corrupted or is removed). We need to let the user know it and don't load the plugin. Otherwise browser will meet problem when running.
>
><b>4.2. Backward compatibility with JRE from other vendors.</b>
>
>- We will search plugins directoty first. So we will not break the old way.
>
><b>4.3. Backward compatibility with other platforms.</b>
>
>- We should not break browser's java plugin integration on other platforms include Windows and Mac.
>
><b>4.4. How about if user don't want to use plugin?</b>
>
>- We can supply a UI in browser installer to disable the java plugin:
>We can remember this setting in browser so the java plugin dialog will not bother the user anymore. But if the user click the plugin area, we can redirect the user to download it and ask whether he/she wants to enable jave plugin.
>
><b>4.5. Java Web Start</b>
>
>- We can deal with it as a help application and define it's mime type in mime.types file and mailcap file.
>
>- Java Web Start need to update mime.types file and mailcap when install.
>
>- Java Web Start need to provide a config tools to update mime.types and mailcap files.
>
>- Browser will trade it as a help application. By default, the Browser will use the latest version of Java Web Start which is configured in mailcap file. But the user also can select different Java Web Start from preference dialog himself.
>
><b>4.6. Patch check</b>
>
>- Currently, Solaris installer( for browser) doesn't check system patches for JRE. We need to do this checking before install browser which has JRE bundled.
>
><b>4.7. Search Java Installation in default location</b>
>
>We should look in the default instal location to search java installation.
>
>- Solaris: /usr/j2se/jre
>- Linux: /usr/java/j2re<version> or /usr/java/j2sdk<version>/jre
>
><b>4.8. Compiler's version problem</b>
>
>Since gcc3.2 build mozilla can't work with gcc2.9x build java plugin. We should use java plugin which compiled by same version of compiler of gcc. For example, If mozilla is built by gcc3.2, we only list the java plugin which compiled by gcc3.x.
>
>- In the versions file, the java installation will add a attribute to each entry named "compiler" to tell it is built by which compiler. Please see the file format below.
></pre>
><pre>
><b>5. File format</b>
>
><b>5.1. mailcap</b>
>
>See <a href="http://archive.ncsa.uiuc.edu/SDG/Software/Mosaic/Docs/rfc1524.txt">http://archive.ncsa.uiuc.edu/SDG/Software/Mosaic/Docs/rfc1524.txt</a>
>
><b>5.2. mime.types</b>
>
>See <a href="http://developer.netscape.com/docs/manuals/enterprise/40/nsapi/0c_mime.htm#1017292">"http://developer.netscape.com/docs/manuals/enterprise/40/nsapi/0c_mime.htm#1017292</a>
>
><b>5.3. versions</b>
>This is an ASCII text file containing JRE/JDK entries.
>
>We considered using an XML format for this file and determined that it was not practical because
>parsing XML from a Bourne shell script is not easy to implement.  Also, programs that parse the
>versions file may not have easy access to an XML parser.
>==============================================================
>File-version: <file-version>
>
>version=&lt;version&gt; | \
>type={jre|jdk} | \
>os=&lt;operating system&gt; | \
>arch=&lt;processor architecture&gt; | \
>path={&lt;jre-home&gt;|&lt;jdk-home&gt;} | \
>compiler={&lt;compiler version&gt;} | \
>{mozilla|other-browser}{version}.plugin.path=&lt;relative-path&gt;" | \
>description="&lt;description&gt;"
>==============================================================
>For example:
>If you have JRE 1.4.1 and JDK1.4.2 installed. The content of this file may looks like following:
>==============================================================
>File-version: 1.0
>
>version=1.4.2 | \
>type=jdk | \
>os=linux | \
>arch=i386 | \
>path=/usr/local/java/j2sdk1.4.2 | \
>compiler=gcc32 | \
>mozilla1.3.plugin.path=/plugin/i386/ns610-gcc32/libjavaplugin_oji.so | \
>ns7.plugin.path=/plugin/i386/ns7-gcc32/libjavaplugin_oji.so | \
>description="J2SE v1.4.2 SDK"
>
>version=1.4.1 | \
>type=jre | \
>os=linux | \
>arch=i386 | \
>path=/usr/local/java/j2re1.4.1 | \
>compiler=gcc29 | \
>mozilla1.3.plugin.path=/plugin/i386/ns610/libjavaplugin_oji.so | \
>ns7.plugin.path=/plugin/i386/ns7/libjavaplugin_oji.so | \
>description="J2SE v1.4.1 Runtime Environment"
>==============================================================
>*Note:
>- The description should be enclosed by double quotes.  The pipe character is not allowed in the
>  description.
>- There should not be whitespace between each entry.  Whitespace above is included for clarity.
>- There should not be empty line(s) between each line.  Empty lines above are included for
>  clarity.
>- &lt;plugin.path&gt; is a relative path based on &lt;path&gt;.  &lt;plugin.path&gt; may differ between JREs.
>- The entries are actually a single line, they are shown above as multiple lines for clarity.
>- The file is sorted by version in descending order.
>- Applications will concatenate the system (/etc/.java/versions) and current user
>  (~/.java/versions) files to get a list of all accessible JREs.
>- File-version field will refer to version number of the versions file format.
>- Since one java installation will support multiple browser, each browser should pick up correct plugin file.
></pre>
><hr>
><br>
><div align="right">
>Maintained by <a href="mailto:[e-mail]">Pete Zha</a>
><br>
><script language="JAVASCRIPT">
><!--
>document.write("Last Updated at " + document.lastModified);
>//-->
></script> </div>
></div>
>
></body>
></html>