218645 - string bundles: use setcontenttype after ::open

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P2)

Description

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

125   rv = NS_NewChannel(getter_AddRefs(channel), uri);
126   if (NS_FAILED(rv)) return rv;
127 
128   nsCOMPtr<nsIInputStream> in;
129   rv = channel->Open(getter_AddRefs(in));
130   if (NS_FAILED(rv)) return rv;
131 
132   // It's a string bundle.  We know what MIME type it is!
133   channel->SetContentType(NS_LITERAL_CSTRING("text/plain"));

would it make more sense to do the SetContentType before the ->Open call?

Comment 1

[Boris Zbarsky [:bzbarsky]]

This was done this way on purpose -- this is not a hint but an override of
whatever type the channel may decide to return.

Comment 2

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

hm, but why does this code care about the content type at all, why doesn't it
just never call GetContentType?

Comment 3

[Boris Zbarsky [:bzbarsky]]

It never does.  However, some channels will determine the content type eagerly
(i.e. no matter whether GetContentType is called).  In particular, we were
hitting mime.types on Unix for every single stringbundle load before this code
was added.

Comment 4

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

yes, that is why I suspected the SetContentType should be before the Open.
Are you saying that content-type determination happens after ::Open is called?

Comment 5

[Boris Zbarsky [:bzbarsky]]

That's correct.  Getting contentType before Open is called has undefined
behavior, per nsIChannel.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Now I remember what the deal was here -- we had no type hints when this
code was written, so I actually changed file channels to lazily determine the
content type in addition to adding this setcontenttype call.

Thinking about it more, I agree that we should setcontenttype before calling
open (and changing the comment to say "we expect it to be" instead of "is" at
the end).  r=me for that change.

Comment 7

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

Attachment: patch

Comment 8

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

-> me

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 131249 [details] [diff] [review]
patch

r=bzbarsky

Comment 10

[Darin Fisher]

Comment on attachment 131249 [details] [diff] [review]
patch

looks good.  sr=darin

Comment 11

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

Checking in intl/strres/src/nsStringBundle.cpp;
/cvsroot/mozilla/intl/strres/src/nsStringBundle.cpp,v  <--  nsStringBundle.cpp
new revision: 1.139; previous revision: 1.138
done