192522 - [FIXr]doing too much work to determine MIME type of .properties files

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

[Alec Flett]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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")?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: like this

Comment 5

[Alec Flett]

Comment on attachment 114142 [details] [diff] [review]
like this

kick ass!
r=alecf

Comment 6

[Darin Fisher]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: right then. So more like this.

Comment 9

[Darin Fisher]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: I need more sleep... thank you, Darin.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 114285 [details] [diff] [review]
I need more sleep... thank you, Darin.

third time's the charm?

Comment 12

[Darin Fisher]

Comment on attachment 114285 [details] [diff] [review]
I need more sleep... thank you, Darin.

sr=darin

Comment 13

[Alec Flett]

Comment on attachment 114285 [details] [diff] [review]
I need more sleep... thank you, Darin.

even better.
sr=alecf

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.4a.