CCing OS/2 folks. This patch includes OS/2 part. And looks like there're some bugs. It took a non-root pref branch. But passes the branch path to Get*Pref() again. I don't change them in v1.0 because some users may use the strange pref name for customizing.

Comment on attachment 541657 [details] [diff] [review] Patch v1.0 >+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp >-static const char NEVER_ASK_FOR_SAVE_TO_DISK_PREF[] = "saveToDisk"; >-static const char NEVER_ASK_FOR_OPEN_FILE_PREF[] = "openFile"; >+static const char SAVE_TO_DISK_PREF[] = "browser.helperApps.neverAsk.saveToDisk"; >+static const char OPEN_FILE_PREF[] = "browser.helperApps.neverAsk.openFile"; Please keep the old macro names. >+++ b/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp >+ // XXX Is this intentional? I would think not. File a bug, please. >+++ b/uriloader/exthandler/os2/nsOSHelperAppService.cpp >- rv = prefBranch->GetComplexValue(aPrefName, >- NS_GET_IID(nsISupportsString), >- getter_AddRefs(prefFileName)); >+ NS_SUCCEEDED(Preferences::GetString(aPrefName, aFileLocation))) { Is that equivalent? >+ // XXX Is this intentionally? Again, I would think not, in both cases. >+++ b/uriloader/exthandler/unix/nsOSHelperAppService.cpp The changes to this file don't make sense. >+++ b/uriloader/prefetch/nsOfflineCacheUpdateService.cpp >+ static const char kPrefName[] = "offline-apps.allow_by_default"; >+ if (aPrefBranch) { >+ aPrefBranch->GetBoolPref(kPrefName, aAllowed); I believe this is not equivalent to the old code when the pref is not set. In general, this aPrefBranch thing seems to be an optimization that is not needed anymore. Just file a followup bug to rip it out?

>>+++ b/uriloader/exthandler/os2/nsOSHelperAppService.cpp >>- rv = prefBranch->GetComplexValue(aPrefName, >>- NS_GET_IID(nsISupportsString), >>- getter_AddRefs(prefFileName)); >>+ NS_SUCCEEDED(Preferences::GetString(aPrefName, aFileLocation))) { > > Is that equivalent? Yes. nsISupportsString returns UTF16 string which is converted from char pref value which was assumed as UTF8. >>+++ b/uriloader/prefetch/nsOfflineCacheUpdateService.cpp >>+ static const char kPrefName[] = "offline-apps.allow_by_default"; >>+ if (aPrefBranch) { >>+ aPrefBranch->GetBoolPref(kPrefName, aAllowed); > > I believe this is not equivalent to the old code when the pref is not set. |*aAllowed = PR_FALSE;| is done in top of the method. So, If GetBoolPref() is failed, *aAllowed isn't modified, i.e., it keeps PR_FALSE. The old code is redundant.

OK, makes sense. What about uriloader/exthandler/unix/nsOSHelperAppService.cpp ?

(In reply to comment #4) > OK, makes sense. What about > uriloader/exthandler/unix/nsOSHelperAppService.cpp ? I forgot to change in the file. I'm testing it now. http://tbpl.mozilla.org/?tree=Try&pusher=masayuki@d-toybox.com&rev=8b1741fe90b8

removing unnecessary change of v2.0. sorry for the spam.

Comment on attachment 542009 [details] [diff] [review] Patch v2.0.1 >--- a/uriloader/exthandler/nsExternalHelperAppService.cpp >+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp > NS_IMETHODIMP nsExternalHelperAppService::IsExposedProtocol(const char * aProtocolScheme, PRBool * aResult) > { >+ // check the per protocol setting first. it always takes precidence. While you're here, could you do s/precidence/precedence/?

> could you do s/precidence/precedence/? I'll do it after bz's review.

Comment on attachment 542009 [details] [diff] [review] Patch v2.0.1 r=me, but please do file those OS/2 bugs.

(In reply to comment #10) > r=me, but please do file those OS/2 bugs. filed bug 668725.

temporary backed out from inbound due to strange orange.

http://hg.mozilla.org/integration/mozilla-inbound/rev/7308d42e070d relanded.