128508 - freeze nsIChannel

Status: VERIFIED FIXED

(Core :: Networking, defect, P3)

Description

[Darin Fisher]

freeze nsIChannel

Comment 1

[Darin Fisher]

Attachment: v1 patch

changed:

  nsIRequest::name	   (wstring -> AUTF8String)
  nsIChannel::contentType  (string  -> ACString)

added:

  nsACString nsIChannel::contentCharset

eliminated:

  string nsIHttpChannel::charset

while at it, i also changed nsIHttpChannel and nsIHttpProtocolHandler to use
ACString for all string values, and converted the htmlparser to store MIME
types as ASCII instead of UCS2 (ok'd by jst) + a few other _this is ASCII_
changes.

Comment 2

[Darin Fisher]

looking for code reviews... anyone interested?  or, should i just pick someone ;-)

Comment 3

[Bradley Baetz (:bbaetz)]

Yeah, um... Not to poke holes in this, but I though that strings weren't
freezing for 1.0. You can't @FREEZE this in that case.

See bug 125389.

You could freeze it, but there wouldn't be much point, since none of the methods
involving strings could be safely used.

Comment 4

[Bradley Baetz (:bbaetz)]

Actually, jag just told me that those are freezing for 1.0, so you can ignore
that last comment ;)

Comment 5

[Gagan]

Comment on attachment 74277 [details] [diff] [review]
v1 patch

r=gagan. Good job! 
I wonder if there is value in defining some (or even al) of the string
constants (like UNKNOWN_CONTENT_LENGTH) as really being
NS_LITERAL_CSTRING("application/x-unknown-content-type").  From just a cursory
look I couldn't find any place that used it in it's raw form. This might
cleanup the code even better. But it's just a suggestion.

Comment 6

[rpotts (gone)]

that is a BIG patch :-)

Here are some comments, NONE of them have to do with your changes :-)  They are
just about how 'nasty' the surrounding code is :-)

1. Now that we have NS_ParseContentTypeType() should all
nsIChannel::SetContentType() implementations use it?  Currently, some channels
do and some channels don't...  I'd vote that since the charset got promoted to
nsIChannel we should correctly support setting content-type AND charset.  This
way the charset is handled correctly AND stripped off of the content-type :-)

2. Setting the content-type to "text/plain;charset=US-ASCII" in
nsDataChannel.cpp seems VERY wrong and twisted !

3. Lots of the 'dummy channel' implementations stub out methods with NS_OK... 
Even if they have [out] parameters !!  Currently this isn't hurting us (?) but
it's just another live land mine that someone will step on one day...

-- rick

Comment 7

[rpotts (gone)]

Comment on attachment 74277 [details] [diff] [review]
v1 patch

sr=[e-mail]

very nice !!

Comment 8

[Bradley Baetz (:bbaetz)]

One thing I'd like on nsIChannel is a 'did this load succeed' attribute. the
response in OnStart just shows if data is being receieved, and http errors +
other proxy errors means that we can get valid data for a 404, which we don't
want to store in history, save, and so on.

Currently code has to QI to nsIHttpChannel, and manually interpret the response
code, and this seems like it could be extended.

Maybe we should leave this for the moment, though, since it snot really needed.
It would be nice though, I think, for a couple of bugs

Comment 9

[Darin Fisher]

bbaetz: yeah, perhaps an attribute like that would help... but, i'd like to land
the current patch first.  there'll most likely be a second pass for changes, and
we should consider some sort of "did i get the content i wanted" flag.  i'll be
making a general request on the newsgroup for comments on these interfaces. 
it'd therefore be good to get the current changes in so people can easily see
where we are.

Comment 10

[Darin Fisher]

rick:

1- agreed... i know i skipped nsViewSourceChannel because of the "; x-view-type"
foo, and i think i also skipped the dummy channels, since i think they're never
even QI's to nsIChannel (more on that later)... i may have also missed some
nsIChannel impls in mailnews... will double check and fix all that can be fixed.

2- perhaps we shouldn't assign any default charset... instead, we should perhaps
let charset detection kick-in.  or, maybe this would lead to some wierd
regression some where... ugh... i don't know if i want to risk such a change
right now.

3- i believe the dummy nsIChannel impls really only need to impl nsIRequest...
they impl nsIChannel for legacy reasons (i.e., loadGroup used to be an attribute
on nsIChannel).  maybe i can turn those into nsIRequest impls?? i hesitated to
do so for fear that someone was accessing the dummy channels via the load group :-(

at any rate, thanks much for the review :-)

Comment 11

[Darin Fisher]

as i mentioned to gagan via AIM, i like the idea of using NS_LITERAL_CSTRING's
for the mime types... but that can wait until we cleanup the mime service API.

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 74277 [details] [diff] [review]
v1 patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 13

[rpotts (gone)]

bbaetz:  Perhaps we need to better propagate 'protocol specific' failures back
to the 'status' attribute on nsIRequest.

Currently this indicates whether the request succeeded or not...  but we could
augment it with protocol specific error (or maybe success) codes indicating
things like 404 or redirect...

-- rick

Comment 14

[Bradley Baetz (:bbaetz)]

But then dochshell would have to be changed too, to dispaly the info. Of course,
we sould return 1 as a success code, and in theory nothing should break, since
everyone should be using the NS_SUCCEEDED macro, right?

Comment 15

[[not reading bugmail]]

not sure if this fits into networking, but I thought you might like to see bug
131256 about status line text updating issues.. that have regressed.

Comment 16

[Darin Fisher]

marking FIXED... bug 124465 will be used to track marking this interface FROZEN.

Comment 17

[benc]

VERIFIED:
http://lxr.mozilla.org/mozilla/source/netwerk/base/public/nsIChannel.idl#57