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)
Core Graveyard
File Handling
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.8alpha3
People
(Reporter: bzbarsky, Assigned: Biesinger)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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* ?
Updated•23 years ago
|
Target Milestone: --- → Future
Assignee | ||
Comment 2•20 years ago
|
||
*** Bug 221708 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•20 years ago
|
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)
Assignee | ||
Comment 3•20 years ago
|
||
something like this... I still need to test this
Assignee: file-handling → cbiesinger
Status: NEW → ASSIGNED
Assignee | ||
Updated•20 years ago
|
Target Milestone: Future → mozilla1.8alpha3
Assignee | ||
Comment 4•20 years ago
|
||
how about something that actually compiles on windows...
Attachment #154324 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #154867 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 5•20 years ago
|
||
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+
Assignee | ||
Comment 6•20 years ago
|
||
(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...
Assignee | ||
Comment 7•20 years ago
|
||
now with lowercasequalsliteral
Attachment #154867 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #155216 -
Flags: superreview?(darin)
Comment 8•20 years ago
|
||
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+
Assignee | ||
Comment 9•20 years ago
|
||
(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.
Assignee | ||
Comment 10•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #155216 -
Attachment is obsolete: true
Assignee | ||
Comment 11•20 years ago
|
||
checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•