The nsIMIMEInfo implementation assumes that extensions are char* and cannot contain unicode characters. This seems like a bad assumption... Would it not make sense to at least make them PRUnichar* ?

mass move, v2. qa to me.

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

something like this... I still need to test this

how about something that actually compiles on windows...

Comment on attachment 154867 [details] [diff] [review] patch v2 >Index: uriloader/exthandler/nsExternalHelperAppService.cpp > NS_IMETHODIMP nsExternalAppHandler::OnStartRequest(nsIRequest >- // first, check to see if we've been canceled.... >- if (mCanceled) // then go cancel our underlying channel too >- return request->Cancel(NS_BINDING_ABORTED); >- Why? that code's correct, no? Or is it just that the situation can no longer arise because we don't put put up the dialog before this? >Index: uriloader/exthandler/win/nsOSHelperAppService.cpp >+ LONG err; >+ if (mIsNT) { [etc] Is this code worth factoring out somehow? >+ if (!aMIMEType.EqualsLiteral(APPLICATION_OCTET_STREAM)) { Should be LowerCaseEqualsLiteral, no? r=bzbarsky with that.

(In reply to comment #5) > Why? that code's correct, no? Or is it just that the situation can no longer > arise because we don't put put up the dialog before this? I'm not sure it could ever arise. But correct, we only put up that dialog later in OnStartRequest. > >Index: uriloader/exthandler/win/nsOSHelperAppService.cpp > > >+ LONG err; > >+ if (mIsNT) { > [etc] > > Is this code worth factoring out somehow? Hm.. duplicated in two places only... and with MZLU, it would go away (bug 239279). I don't think it's worth it. > >+ if (!aMIMEType.EqualsLiteral(APPLICATION_OCTET_STREAM)) { > > Should be LowerCaseEqualsLiteral, no? hm, right...

now with lowercasequalsliteral

Comment on attachment 155216 [details] [diff] [review] patch v3 >Index: netwerk/mime/public/nsIMIMEInfo.idl >- void setFileExtensions(in ACString aExtensions); >+ void setFileExtensions(in AUTF8String aExtensions); ok, binary compatible. no need to rev UUID of interface i guess. >Index: netwerk/mime/public/nsIMIMEService.idl >- nsIMIMEInfo getFromTypeAndExtension(in ACString aMIMEType, in ACString aFileExt); >+ nsIMIMEInfo getFromTypeAndExtension(in ACString aMIMEType, in AUTF8String aFileExt); same with this guy... >Index: uriloader/exthandler/nsExternalHelperAppService.cpp >+#include "nsIRefreshURI.h" // XXX needed to redirect according to Refresh: URI >+#include "nsIDocumentLoader.h" // XXX needed to get orig. channel and assoc. refresh uri why the XXX comments? do you plan on removing these dependencies at some point? (usually XXX denotes a hack or problem, etc.) >+static nsresult UnescapeFragment(const nsACString& aFragment, nsIURI* aURI, >+ nsAString& aResult) >+{ >+ // First, we need a charset >+ nsCOMPtr<nsIURL> url(do_QueryInterface(aURI)); >+ if (!url) >+ return NS_ERROR_NOT_AVAILABLE; >+ >+ nsCAutoString originCharset; >+ nsresult rv = url->GetOriginCharset(originCharset); >+ NS_ENSURE_SUCCESS(rv, rv); this QI seems unnecessary. GetOriginCharset is a method on nsIURI not nsIURL, so why the QI? is it that you only mean to unescape things that are actual URLs? the comments seem to indicate otherwise... sr=darin

(In reply to comment #8) > ok, binary compatible. no need to rev UUID of interface i guess. > same with this guy... right, that's what I was thinking. > why the XXX comments? do you plan on removing these dependencies at some > point? (usually XXX denotes a hack or problem, etc.) sure... at some point... ;) yes, I'd rather these dependencies were not there. > this QI seems unnecessary. GetOriginCharset is a method on nsIURI not nsIURL, I knew I would regret largely copying this code and not checking on that... I'll get rid of the QI.

checked in.