80787 - nsMimeInfoImpl/exthandler assumes extensions are char* (not Unicode)

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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* ?

Comment 1

[benc]

mass move, v2.
qa to me.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

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

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

how about something that actually compiles on windows...

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

(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...

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v3

now with lowercasequalsliteral

Comment 8

[Darin Fisher]

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

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

(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.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: final patch

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in.