The main "advanced" pref panel is still not ported to the new prefwindow, which makes its subpanels look like top-level panels.

This patch ports the advanced pref panel. I moved all JS functions into a own file called pref-advanced.js Since I don't see the javaPluginSettings and the systemPrefs groupboxes using Windows XP, somebody else with Linux has to have a look, if they are still working.

Comment on attachment 308974 [details] [diff] [review] Port advanced pref panel WIP Just some general comments to start with: 1) You have removed the listed contributor from the xul file, put it back and you should also add it to the js file. 2) Function names should be properly capitalised - e.g. SysPrefCheck function. 3) Some lines are over long and should be split over several lines with correct indentation - e.g. the var appShell line. 4) In If statements the code to be executed should not be on the same line as the statement itself. 5) We are moving away from using registerOKCallbackFunc 6) You have removed the stringbundles in the xul but still reference them in the JS 7) The javaCheck function takes an argument, setFocus, which is no longer used so it could be removed.

hrm, I had started to work into this myself, but I didn't get around to creating a diff yet... let's see if mine addresses Ian's comments - the function name thing is something I first started and then reverted in my locale WIP due to my unclearness with all that Java stuff in this panel...

Aqualon told me on IRC he's OK with me taking this over. Here's what I have in my tree right now for this. I still need to look what Aqualon's patch might have done in a better way than mine, and I didn't not yet do the switch to upper-cased function names as I couldn't figure out everything about those Java functions - I don't even know where the Java stuff outside the main checkbox would even show up, I think I've never seen that stuff myself...

Comment on attachment 309015 [details] [diff] [review] patch v1: try moving advanced panel to new prefwindow >Index: mozilla/suite/common/pref/pref-advanced.xul >=================================================================== >+ <preferences id="content_preferences"> Oops. Comparing my diff to Aqualon's, I spotted this obvious error. Fixed locally.

(In reply to comment #2) > (From update of attachment 308974 [details] [diff] [review]) > Just some general comments to start with: > 1) You have removed the listed contributor from the xul file, put it back and > you should also add it to the js file. I don't know what policy there is for the list of contributors, but the only thing left from Mike's patch in bug 126600 will be the load of pref-advanced.dtd (the biggest part of it was already removed by bug 361682).

(In reply to comment #4) > Created an attachment (id=309015) [details] > patch v1: try moving advanced panel to new prefwindow The majority of my comments from Aqualon's patch still hold for this one. I'm not sure what the protocol is for when to remove a contributor from the boilerplate but Aqualon's argument does seem good. > > Aqualon told me on IRC he's OK with me taking this over. Here's what I have in > my tree right now for this. I still need to look what Aqualon's patch might > have done in a better way than mine, and I didn't not yet do the switch to > upper-cased function names as I couldn't figure out everything about those Java > functions - I don't even know where the Java stuff outside the main checkbox > would even show up, I think I've never seen that stuff myself... > The main difference between the two patches is the position of the declaration of const/var in the JS file (I prefer Aqualon's positioning). From examining the code I would say it should come into play when you have multiple JVMs installed (say both 1.5 and 6.0).

Comment on attachment 309015 [details] [diff] [review] patch v1: try moving advanced panel to new prefwindow r- for the moment

This patch should have all comments addressed except the registerOKCallbackFunc stuff, which I still need to figure out - we're setting something that looks to the user like a pref but internally isn't and I'm not yet sure how to solve this best. I still wonder though if we want/need to keep this JVM selection at all, the suite seems to be the only one still having this in the code, and with Sun moving to NPAPI instead of OJI, this OJI-centric stuff probably won't work in any case in the future.

Comment on attachment 309699 [details] [diff] [review] patch v2: everything but callback addressed Just a quick scan. ;-) >Index: mozilla/suite/common/pref/pref-advanced.xul >=================================================================== >+<!DOCTYPE overlay [ >+<!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd"> %brandDTD; > <!ENTITY % prefAdvancedDTD SYSTEM "chrome://communicator/locale/pref/pref-advanced.dtd"> %prefAdvancedDTD; You should indent the entities. >+ <stringbundle id="brandBundle" >+ src="chrome://branding/locale/brand.properties"/> This bundle is unused. >+ <groupbox id="advancedSettings" align="start"> >+ <caption label="&prefEnableJava.caption;"/> >+ <vbox align="start" id="contentEnablingBox"> >+ <checkbox id="advancedJavaAllow" label="&enbJavaCheck.label;" >+ accesskey="&enbJavaCheck.accesskey;" >+ preference="security.enable_java" >+ oncommand="JavaCheck()"/> >+ </vbox> >+ </groupbox> Having align=start on the groupbox is rather useless here. The entire <vbox> can go away, too. And the checkbox label should be on its own line. >+ <groupbox id="javaPluginSettings" align="start"> Useless align. >+ <groupbox id="systemPrefs"> >+ <caption id="systemPrefCaption" label="&systemPref.caption;"/> >+ <vbox id="systemPrefBox" align="start"> >+ <checkbox id="systemPrefCheck" label="&systemPrefCheck.label;" >+ accesskey="&systemPrefCheck.accesskey;" >+ preference="config.use_system_prefs"/> >+ <vbox class="indent" flex="1"> >+ <description>&systemPref.desc;</description> >+ </vbox> > </vbox> Boring <vbox> party, all can go home. ;-) IMO, the description should be reworded (especially the "check this item" part) and moved before the checkbox (label on its own line). >Index: mozilla/suite/common/pref/pref-advanced.js >=================================================================== This file is really in an awful state. (Yes, i know it's copied stuff.) I tried even to test that OJI stuff, but failed; despite having Java installed, my Linux system lacks the required(?) /etc/.java/versions / ~/.java/versions files pointed by the preferences. So just some basic nits. ;-) BTW: did you actually test that the OJI stuff works both with and without instant apply? >+const nsIJVMConfig = Components.interfaces.nsIJVMConfig; >+var jvmConfigMgr; >+var jvmConfigList = GetJVMConfigList(); >+const currentJVMPluginPath = GetCurrentJVMPluginPath(); >+var oldJVMConfig = null; Globals variables should have a g prefix, constants should be UPPERCASE or kPrefixed. (nsIJVMConfig follows an often used exceptional pattern, though.) >+function Startup() { >+ SysPrefCheck(); >+ InitJVMConfigList(); >+} Most pref panels put both brackets on their own line, and so should this, too. >+function SysPrefCheck() { >+ if ("@mozilla.org/system-preference-service;1" in Components.classes) >+ try { >+ var appShell = Components.classes["@mozilla.org/system-preference-service;1"] >+ .getService(Components.interfaces.nsIPrefBranch); >+ return; >+ } >+ catch(e) {} >+ document.getElementById("systemPrefs").hidden = true; >+} If you're using a try block anyway, you can drop the outer if, move the hidden line into the catch block and drop the return. Furthermore, the appShell variable is useless and even misleadingly named. >+function GetJVMConfigMgr() { This function is just called once and hence pretty useless. >+function GetJVMConfigList() { >+ if (!jvmConfigMgr) >+ jvmConfigMgr = GetJVMConfigMgr(); >+ return jvmConfigMgr && jvmConfigMgr.getJVMConfigList(); The reentrancy check is useless, we don't load panels multiple times anymore. GetJVMConfigMgr is only called here, so its code can be used to simplyfy the result computation. >+function GetCurrentJVMPluginPath() { >+ var result = ""; >+ if ("application/x-java-vm" in navigator.mimeTypes) >+ try { >+ var prefs = Components.classes["@mozilla.org/preferences-service;1"] >+ .getService(Components.interfaces.nsIPrefBranch); Tinkering with the pref service is not recommended with the new prefwindow, it just makes it harder to handle instant vs. non-instant mode. It's better use <preference> elements, cretaed dynamically if needed at all. >+const jvmConfigListView = { Uh. Do we really need a try here? Shouldn't a simple list box suffice? This seems pretty much overkill, especially with a custom tree builder. >+function SetVisible(visible) { This function is extremely badly named (and not only for grammar reasons). >+ var brandbundle = document.getElementById("brandBundle"); You don't use that anywhere. That said, the actual Java version selection is not stored in a pref, but done via a function call. So, in the case of non-instant apply, you need to register a dialogaccept handler - see pref-navigator.js for an example (look out for "dialogaccept" there).

> Uh. Do we really need a try here? s/try/tree/

(In reply to comment #10) > (From update of attachment 309699 [details] [diff] [review]) > >+function GetCurrentJVMPluginPath() { > >+ var result = ""; > >+ if ("application/x-java-vm" in navigator.mimeTypes) > >+ try { > >+ var prefs = Components.classes["@mozilla.org/preferences-service;1"] > >+ .getService(Components.interfaces.nsIPrefBranch); > > Tinkering with the pref service is not recommended with the new prefwindow, it > just makes it harder to handle instant vs. non-instant mode. It's better use > <preference> elements, cretaed dynamically if needed at all. > As discussed with Karsten on IRC, we do not know if is instant or non-instant mode, so we do need to call the pref service here as we are using it for a special reason. I have also tried using the legacy pref panel on a PC with multiple versions of JRE installed and nothing extra shows up so I think this code is probably already broken and will need to be fixed :S

Comment on attachment 309699 [details] [diff] [review] patch v2: everything but callback addressed As mentioned above, alot to work on here :S

<NeilAway> IanN: can you use .valueFromPreferences to force instant apply? <NeilAway> IanN: well, I think that has a setter too, if it helps

Here's another iteration of the patch, now killing the JVM selection altogether, as I'm not sure if we can get it working nowadays at all and how. I guess the upcoming new non-OJI Java plugin won't support this any more (this is OJI functionality, right) in any case. Interestingly, we now have two ways of turning off Java, with the plugins pane of the Add-ons manager and this pref panel - but then, this is the same as what Firefox 3 has as well.

Comment on attachment 319394 [details] [diff] [review] patch v3: just kill JVM selection I'd like Ian, Karsten and Neil to all agree on killing the JVM switching, so involving you all in the reviews ;-)

Comment on attachment 319394 [details] [diff] [review] patch v3: just kill JVM selection >+function SysPrefCheck() >+{ >+ try { >+ Components.classes["@mozilla.org/system-preference-service;1"] >+ .getService(Components.interfaces.nsIPrefBranch); >+ } >+ catch(e) { >+ document.getElementById("systemPrefs").hidden = true; >+ } >+} Nit: the old sys pref check had an if to avoid the try/catch when the component wasn't listed in classes. I guess you could avoid the try/catch altogether: document.getElementById("systemPrefs").hidden = !(("@mozilla.org/system-preference-service;1" in Components.classes) && (Components.classes["@mozilla.org/system-preference-service;1"].getService() instanceof Components.interfaces.nsIPrefBranch));

Comment on attachment 319394 [details] [diff] [review] patch v3: just kill JVM selection The only platforms that the code you are removing would work on is Linux/UNIX - as they are the only ones that implement jvm-config-mgr at the moment. Might be worth checking with the original contributor of that implementation - bug 210042 seems to be what stops it working. Adding a symbolic link for ns7-gcc32 to point to ns7-gcc29 is one way to show the pref code displaying the list of java versions. You also need write permissions to the plugin directory for switching JVMs to work.

Of course that should have been symbolically linking ns7-gcc32 to ns7 as the default for recent java plugins is to be built with gcc32, ns7-gcc29 is for older browsers.

Ian, why exactly should we keep that old unmaintained code alive anyway, esp. given the fact the probably by the time we release SeaMonkey 2 the new non-OJI Java plugin will be out?

(In reply to comment #20) > Ian, why exactly should we keep that old unmaintained code alive anyway, esp. > given the fact the probably by the time we release SeaMonkey 2 the new non-OJI > Java plugin will be out? > There will probably still need to be some sort of switching mechanism pref frontend on Linux/UNIX with the new NPRuntime code but as I cannot find a bug to rewrite the current backend it would be outside the scope of this bug though maybe spin off bug for release notes on how to switch between versions of the NPRuntime Java plugin (as well as the ones already included for OJI)

r=me with change Neil suggested

Comment on attachment 319394 [details] [diff] [review] patch v3: just kill JVM selection >Index: mozilla/suite/common/pref/pref-advanced.js >=================================================================== >+function SysPrefCheck() >+{ >+ try { Actually, I'm no friend of that brace-at-end-of-the-line style, so if you, too, feel this is ugly, you've got my consent to use the nice style variant. ;-) >Index: mozilla/suite/locales/en-US/chrome/common/pref/pref-advanced.dtd >=================================================================== >+<!ENTITY systemPref.desc "With this option, &brandShortName; inherits preferences from the system. The system settings will override the &brandShortName; preferences"> I'd start the second sentence with "These". In either case, the second sentence lacks the final full stop. r=me with the dot added.

Checked in with the nits fixed - thanks to Mnyromyr for making something readable from Neil's proposal in comment #17 for the version I checked in.