Closed Bug 80787 Opened 23 years ago Closed 20 years ago

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

Categories

(Core Graveyard :: File Handling, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.8alpha3

People

(Reporter: bzbarsky, Assigned: Biesinger)

References

Details

Attachments

(1 file, 3 obsolete files)

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* ?
Target Milestone: --- → Future
mass move, v2. qa to me.
QA Contact: tever → benc
*** Bug 221708 has been marked as a duplicate of this bug. ***
Assignee: valeski → file-handling
Blocks: 162116
Component: Networking → File Handling
QA Contact: benc → ian
Summary: nsMimeInfoImpl assumes extensions are char* (not Unicode) → nsMimeInfoImpl/exthandler assumes extensions are char* (not Unicode)
Attached patch patch (obsolete) (deleted) — Splinter Review
something like this... I still need to test this
Assignee: file-handling → cbiesinger
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla1.8alpha3
Attached patch patch v2 (obsolete) (deleted) — Splinter Review
how about something that actually compiles on windows...
Attachment #154324 - Attachment is obsolete: true
Attachment #154867 - Flags: review?(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.
Attachment #154867 - Flags: review?(bzbarsky) → review+
(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...
Attached patch patch v3 (obsolete) (deleted) — Splinter Review
now with lowercasequalsliteral
Attachment #154867 - Attachment is obsolete: true
Attachment #155216 - Flags: superreview?(darin)
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
Attachment #155216 - Flags: superreview?(darin) → superreview+
(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.
Attachment #155216 - Attachment is obsolete: true
checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: