Closed
Bug 192522
Opened 22 years ago
Closed 22 years ago
[FIXr]doing too much work to determine MIME type of .properties files
Categories
(Core :: Networking, defect, P2)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla1.4alpha
People
(Reporter: dbaron, Assigned: bzbarsky)
Details
Attachments
(1 file, 2 obsolete files)
4.33 KB,
patch
|
alecf
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
As discovered in bug 190951, we're doing more work than we ought to to figure out the MIME type of ".properties" files that we load. There's a patch on bug 190951 (attachment 113662 [details] [diff] [review]) that fixes this by hard-coding the extension. However, I'm worried this might not be appropriate for all uses -- after all, ".properties" isn't a standard extension and somebody might be using it somewhere else. However, the patch there may be reasonable nevertheless. Boris also suggested in bug 190951 comment 32 that there might be a solution where the caller loading the file could specify the MIME type to avoid the work. I'm splitting this off as a separate bug from bug 190951 because we have one solution to that bug checked in.
Assignee | ||
Comment 1•22 years ago
|
||
See http://groups.google.com/groups?dq=&hl=en&lr=&ie=UTF-8&oe=UTF-8&frame=right&th=76a883e3c07357fc&seekm=200301032103.QAA12242%40cathedral-seven.mit.edu#link1 for my proposed approach to solving this...
Comment 2•22 years ago
|
||
I like the proposal in the newsgroup. (quick summary was to provide an extra interface on the channel to allow the caller to force the mime type) give me the interface, and I'll stick the call in the string bundle code. Another possibility would be for specific protocols to override this. For example, I think hardcoding .properties for all chrome:// and res:/ protocols would be quite reasonable.
Assignee | ||
Comment 3•22 years ago
|
||
Something else I just realized.... if you want to _force_ the MIME type instead of hinting it, you could just call SetContentType right before calling Open/AsyncOpen. Channels have to support SetContentType anyway; none of our channel impls will try to detect the type if a type is already set, iirc. The reason we need the hinting stuff is that for http we don' want to force types. But that's not relevant for resource: urls, is it? So perhaps the .properties loading code should just SetContentType("text/plain")?
Assignee | ||
Comment 4•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Attachment #114142 -
Flags: superreview?(darin)
Attachment #114142 -
Flags: review?(alecf)
Assignee | ||
Updated•22 years ago
|
Summary: doing too much work to determine MIME type of .properties files → [FIX]doing too much work to determine MIME type of .properties files
Comment 5•22 years ago
|
||
Comment on attachment 114142 [details] [diff] [review] like this kick ass! r=alecf
Attachment #114142 -
Flags: review?(alecf) → review+
Comment 6•22 years ago
|
||
Comment on attachment 114142 [details] [diff] [review] like this >+ channel->SetContentType(NS_LITERAL_CSTRING("text/plain")); ... >+ rv = channel->Open(getter_AddRefs(in)); hmm... so this is a change in convention. and if we go this way we'd want to document this API change/requirement in nsIChannel.idl. an alternative solution would be to make the content-type determination happen inside GetContentType instead of inside EnsureStream. then, the string bundle code could Open the channel and _then_ set the content type. later on when nsFileChannel::GetContentType is called, it could check to see if it has already determined mContentType. this solution is consistent with the current API, since the content-type attribute is only supposed to be accessed after a channel has been opened. (yes, i know... the comments in nsIChannel.idl don't say this... there's a bug filed on me to cleanup the comments in nsIChannel.idl.)
Attachment #114142 -
Flags: superreview?(darin) → superreview-
Assignee | ||
Comment 7•22 years ago
|
||
Yeah, some documentation would be good. ;) I can definitely move the type-detection out of there. In fact, in this case we never ask for the type. So just fixing the file channel to detect type lazily would do the trick....
Priority: -- → P2
Target Milestone: --- → mozilla1.4alpha
Assignee | ||
Comment 8•22 years ago
|
||
Attachment #114142 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #114263 -
Flags: superreview?(darin)
Attachment #114263 -
Flags: review?(alecf)
Comment 9•22 years ago
|
||
Comment on attachment 114263 [details] [diff] [review] right then. So more like this. >Index: intl/strres/src/nsStringBundle.cpp >+ channel->SetContentType(NS_LITERAL_CSTRING("text/plain")); ... >+ rv = channel->Open(getter_AddRefs(in)); just move the call to SetContentType after the call to Open and i'm happy ;-) >Index: netwerk/protocol/file/src/nsFileChannel.cpp > if (mIsDir) { > rv = nsDirectoryIndexStream::Create(file, getter_AddRefs(mStream)); > } > else { > rv = NS_NewLocalFileInputStream(getter_AddRefs(mStream), file); > } nit: kill the braces (for consistency) > nsFileChannel::GetContentType(nsACString &aContentType) > { >+ NS_ENSURE_TRUE(mURL, NS_ERROR_NOT_INITIALIZED); NS_ASSERTION should be fine. no need to runtime check this. there shouldn't be a channel if there is no URL. otherwise, this is right on! sr=darin with these tweaks.
Attachment #114263 -
Flags: superreview?(darin)
Attachment #114263 -
Flags: superreview-
Attachment #114263 -
Flags: review?(alecf)
Assignee | ||
Comment 10•22 years ago
|
||
Attachment #114263 -
Attachment is obsolete: true
Assignee | ||
Comment 11•22 years ago
|
||
Comment on attachment 114285 [details] [diff] [review] I need more sleep... thank you, Darin. third time's the charm?
Attachment #114285 -
Flags: superreview?(darin)
Attachment #114285 -
Flags: review?(alecf)
Comment 12•22 years ago
|
||
Comment on attachment 114285 [details] [diff] [review] I need more sleep... thank you, Darin. sr=darin
Attachment #114285 -
Flags: superreview?(darin) → superreview+
Comment 13•22 years ago
|
||
Comment on attachment 114285 [details] [diff] [review] I need more sleep... thank you, Darin. even better. sr=alecf
Attachment #114285 -
Flags: review?(alecf) → review+
Assignee | ||
Updated•22 years ago
|
Summary: [FIX]doing too much work to determine MIME type of .properties files → [FIXr]doing too much work to determine MIME type of .properties files
Assignee | ||
Comment 14•22 years ago
|
||
Checked in for 1.4a.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•