Thanks for the review.
http://gwt-code-reviews.appspot.com/62808/diff/31/45 File xpcom/Preferences.h (right): http://gwt-code-reviews.appspot.com/62808/diff/31/45#newcode22 Line 22: #include "nsISupports.h" On 2009/08/28 16:30:34, jaimeyap wrote: > These headers get pulled in from the include path set in the make file. Some of > these headers seem only to exist in gecko-1.9.1/include. > Means that if you are building for gecko-1.9.0 that the build would fail (as it > does on my mac). It should be in plugin-sdks -- can you svn up to make sure it is current? On my Mac, where I built ff2, ff3, and ff35 without any problems, I have: $ find plugin-sdks -name nsIObserver.h plugin-sdks/gecko-sdks/gecko-1.8/include/nsIObserver.h plugin-sdks/gecko-sdks/gecko-1.9.0/include/nsIObserver.h plugin-sdks/gecko-sdks/gecko-1.9.1/include/nsIObserver.h This is with plugin-sdks at r6019 and no locally modified files. http://gwt-code-reviews.appspot.com/62808/diff/31/51 File xpcom/prebuilt/extension/content/options.xul (right): http://gwt-code-reviews.appspot.com/62808/diff/31/51#newcode45 Line 45: <listbox id="listbox" rows="5"> On 2009/08/28 16:30:34, jaimeyap wrote: > The id should probably say what this UI item is supposed to be representing. Not > simply the tag name. Ok, so something like accesslist_listbox? http://gwt-code-reviews.appspot.com/62808/diff/31/52 File xpcom/prebuilt/extension/content/prefScript.js (right): http://gwt-code-reviews.appspot.com/62808/diff/31/52#newcode12 Line 12: alert("Host name must not contain , or !"); On 2009/08/28 16:30:34, jaimeyap wrote: > wrap ',' and '!' these in single quotes so this message reads correctly. Other > wise it sounds like a threat. > Do not do it or !! Ok. http://gwt-code-reviews.appspot.com/62808/diff/31/52#newcode39 Line 39: prefs.splice(idx, 1); On 2009/08/28 16:30:34, jaimeyap wrote: > shouldnt this be prefs = prefs.splice(idx,1) No, splice is a mutating method. The return value is actually an array of the elements removed (except Navigator 4). http://gwt-code-reviews.appspot.com/62808/diff/31/52#newcode78 Line 78: prefs.QueryInterface(Components.interfaces.nsIPrefBranch2); On 2009/08/28 16:30:34, jaimeyap wrote: > shouldnt you be using the return of this queryinterface? The sample code I was following did not -- I assume it was simply checking to make sure it implemented that interface, though I don't think it would throw an exception anyway. I think these should be nsIPrefBranch anyway, since in the JS code we don't use the observer interface that nsIPrefBranch2 supplies. So would something like this be ok: prefs = prefs.QueryInterface(Components.interfaces.nsIPrefBranch); if (!prefs) { return []; } http://gwt-code-reviews.appspot.com/62808/diff/31/52#newcode91 Line 91: prefs.QueryInterface(Components.interfaces.nsIPrefBranch2); On 2009/08/28 16:30:34, jaimeyap wrote: > shouldnt you be using the return of this queryinterface? Same as above. http://gwt-code-reviews.appspot.com/62808 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
