Comment on attachment 323238 [details] [diff] [review] Patch for pref-search v0.1 >+ // build the engine list, we do this by hand instead of using >+ // the xul template builder because of bug #285076 >+ CreateEngineList(); So, it turns out there's a better workaround than creating the menu by hand: set the ref of the engineList after the overlay has loaded. Note that there's a bug in the internet search service (caused by a performance feature) in that the very first time you set the ref it will only build one menuitem. I can attach the patch that works around that in the search service or you can work around it here by calling engineList.builder.rebuild(). >+ // nothing is selected >+ if (!engineList.label) My preference would be for if (!engineList.selectedItem)

I didn't do detailed look, since I assume you'll tackle Neil's comment first. ;-) But while you're fixing the panel you should also fix the trunk regression (as compared to branch) of not showing the search icon in the closed search engine menulist (it's missing for both Classic and Modern, although Modern shows them in the dropdown, which we usually(?) don't do for Classic)...

Changes since v0.1: * Went back to using a template and setting ref/rebuilding as suggested by Neil * Fixed issue with missing icons for search engines - in Gecko 1.9 the attribute for an icon on a menuitem has changed from "src" to "image" - behaviour is back as it was for classic/modern under Gecko 1.8.x

Comment on attachment 325199 [details] [diff] [review] Patch for pref-search v0.2 >+ // Need to set ref attribute once overlay has loaded >+ engineList.setAttribute("ref", "NC:SearchEngineRoot"); >+ // Due to a bug in the internet search service, the first time >+ // ref is set only one menuitem is built, so force a rebuild >+ engineList.builder.rebuild(); >+ > //nothing is selected >+ if (!engineList.selectedItem) Whoops, this doesn't quite work - since the menulist starts off empty it has no selected item - you need to try to set it to the preference value (engineList.value's setter will try to set selectedItem, so you don't need to use getElementsByAttribute yourself in this case). sr=me with this fixed. > try > { >+ name = document.getElementById("browser.search.defaultenginename").value; >+ selectedItem = engineList.getElementsByAttribute("label", name).item(0); > } >+ catch(e) {} I don't think this needs try/catch any more. >- //select the first listed search engine >- engineList.selectedIndex = 1; Heh, I can't believe we only just fixed this ;-) >+ // select a locale-dependent predefined search engine >+ // in absence of a user default >+ engineList.selectedItem = selectedItem; >+ } >+ else >+ { >+ // select the first listed search engine >+ engineList.selectedIndex = 0; > } > >+ var pref = document.getElementById(engineList.getAttribute("preference")); >+ pref.value = engineList.selectedItem.value; Setting the selected item/index updates the value, so you just need pref.value = engineList.value; Or you could be sneaky and use selectedItem.doCommand(); as that updates both the menulist and the preference ;-)

Changes since v0.2: * Removed try/catch * Added attempt to set menulist from preference value Carrying forward sr= and requesting r=

Comment on attachment 325313 [details] [diff] [review] Patch for pref-search v0.2a (Checked in) >Index: pref-search.js >=================================================================== >+function Startup() >+{ >+ CreateEngineList(); >+} ... >+function CreateEngineList() > { AFAICT, this function is only called once, so you could just put the code into Startup. > //nothing is selected And spend a space here ;-) r=me with those. (My Classic Linux build crops the label of the selected search engine in the menulist, but this may just be either toolkit or my KDE theme.)

Comment on attachment 325313 [details] [diff] [review] Patch for pref-search v0.2a (Checked in) Checking into trunk pref-search.xul new revision: 1.39; previous revision: 1.38 pref-search.js new revision: 1.15; previous revision: 1.14 preferences.xul new revision: 1.20; previous revision: 1.19 preftree.xul new revision: 1.122; previous revision: 1.121 done

(In reply to comment #6) > (From update of attachment 325313 [details] [diff] [review]) > (My Classic Linux build crops the label of the selected search engine in the > menulist, but this may just be either toolkit or my KDE theme.) > This is due to display: none css in toolkit themes gnomestripe, winstripe and pmstripe (but not pinstripe): http://lxr.mozilla.org/seamonkey/source/toolkit/themes/gnomestripe/global/menu.css#179 http://lxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/menu.css#196 http://lxr.mozilla.org/seamonkey/source/toolkit/themes/pmstripe/global/menu.css#189 So would have to be raised as a bug against them (Bug 443516)