173094 - Freeze nsIUploadChannel

Status: VERIFIED FIXED

(Core :: Networking, defect, P2)

Description

[Doug Turner (:dougt)]

Comment 1

[Darin Fisher]

-> me

Comment 2

[Chris Nalls]

Mrking Topembed- as part of Topembed triage

Comment 3

[Darin Fisher]

will get this done for 1.3 alpha

Comment 4

[Darin Fisher]

correcting summary.

Comment 5

[Darin Fisher]

Attachment: v1 patch

ok, this patch makes the following changes:

1) revises nsIUploadChannel.idl comments for freezing (adds @status FROZEN)
2) moves nsIUploadChannel.idl into the SDK
3) makes aContentType parameter |ACString| instead of |string|.

Comment 6

[Bradley Baetz (:bbaetz)]

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

>Index: docshell/base/nsDocShell.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v
>retrieving revision 1.492
>diff -u -r1.492 nsDocShell.cpp
>--- docshell/base/nsDocShell.cpp	21 Oct 2002 02:05:59 -0000	1.492
>+++ docshell/base/nsDocShell.cpp	12 Nov 2002 21:47:33 -0000
>@@ -5199,7 +5199,7 @@
>             NS_ASSERTION(uploadChannel, "http must support nsIUploadChannel");
> 
>             // we really need to have a content type associated with this stream!!
>-            uploadChannel->SetUploadStream(aPostData,nsnull, -1);
>+            uploadChannel->SetUploadStream(aPostData, nsCString(), -1);

NS_LITERAL_CSTRING(""), please, here and everywhere else - its more efficient
(although not by too much for an empty string, I think, but its still better
than nsCString())

Comment 7

[Darin Fisher]

sure... but, what we really need is NS_EMPTY_CSTRING().

Comment 8

[Bradley Baetz (:bbaetz)]

you could propose the #define, I suppose. I'm not sure what it would buy you,
though - its only a couple of characters shorter, and would have the same effect.

Comment 9

[Darin Fisher]

the point is not that it is shorter.  the point is that it will help people do
the right thing, and it will make it easy to substitute some other
implementation for an empty string if that is ever appropriate.

Comment 10

[Darin Fisher]

Attachment: v1.1 patch

revised per comments from bbaetz and dougt.

(1) nsCString -> NS_LITERAL_CSTRING("")
(2) removed nsIUploadChannel::setUploadFile since it is unused

and i also added extra documentation about what kind of stream may be passed to
setUploadStream.  i.e., it must implement readSegments, nsISeekableStream, and
threadsafe addref/release.

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 106068 [details] [diff] [review]
v1.1 patch

please write up a blub on exactly what these changes consist of and email it to
me.  I will forward it to the api announcement list.

remove the nsIFile interface declaration from the idl.

Instead of stating "the stream just", change the langauge to only imply that
most necko implementations require these three items.

Lets run this by rpotts since he and I discussed adding the SetFile to this
interface.

Other than that, r=dougt

Comment 12

[rpotts (gone)]

Comment on attachment 106068 [details] [diff] [review]
v1.1 patch

I'm still a little queasy about changing the |const char *| parameters to
|const nsACString&|

It seems like it just makes things more complex...

but i'm just old fashioned ;-)

Comment 13

[Darin Fisher]

dougt: will do

rpotts: yeah... i'm always torn too, though my thinking in this case is that not
specifying a content-type should really be less common than specifying a
content-type, so the ease of specifying NULL is somewhat less significant. 
however, today most callers don't specify a content-type because they are most
often issuing POST requests (the old code required the content-type header to be
manually added to the post data stream).  those consumers should really be
cleaned up IMO.  plus, i think it best to stay consistent with other necko
interfaces, which use A?String for the most part.

Comment 14

[timeless]

Comment on attachment 106068 [details] [diff] [review]
v1.1 patch

>+ * @status FROZEN
please don't freeze while making changes.

> interface nsIUploadChannel : nsISupports
> {

imo, this history stuff doesn't look right in an interface (I think that
reasons for interfaces can be placed in cvs logs or bugs):
>+     * History here is that we need to support both streams that already have
>+     * header (e.g., Content-Type and Content-Length) information prepended to
>+     * the stream (by plugins) as well as clients (composer, uploading
>+     * application) that want to upload data streams without any
 knowledgle of
^ you're persisting a spelling error!
>+     * protocol specifications.  For this reason, we have a special meaning
>+     * for the aContentType parameter (see below).

>+    void setUploadStream(in nsIInputStream aStream,
>+                         in ACString aContentType,
>+                         in long aContentLength);    
>+    readonly attribute nsIInputStream uploadStream;

Considering that you've removed the file version, why not do something like
this:

attribute nsIInputStream uploadStream;
/**
 * Specifying a content type will override native type associated with
uploadStream. 
 * If the implementation needs a mime type and neither the stream nor this flag
include
 * one, it may use the mime service to select one.
 */
attribute ACString uploadStreamContentType;
/**
 * Default: -1. Set this to specify a length.
 */
attribute long uploadStreamLength;
> };

Please note that you should not use 'nsnull' in interfaces (the current file
does).

I'd like to mark 'needswork', but i can't...

Comment 15

[Darin Fisher]

> >+ * @status FROZEN
> please don't freeze while making changes.

why?  this interface is ready.  sure the comments aren't perfect, but nobody
said you can't rev comments on a frozen interface.

> imo, this history stuff doesn't look right in an interface (I think that
> reasons for interfaces can be placed in cvs logs or bugs):
> >+   * History here is that we need to support both streams that already have
> >+   * header (e.g., Content-Type and Content-Length) information prepended to
> >+   * the stream (by plugins) as well as clients (composer, uploading
> >+   * application) that want to upload data streams without any

i believe it is useful as it helps users of this interface understand the
interface and hopefully use it correctly.

>  knowledgle of
> ^ you're persisting a spelling error!

again, comments are NOT persistent when an interface is frozen.  but, thank you
for pointing out this typo.

> Considering that you've removed the file version, why not do something like
> this:

no, it does not make sense to set these attributes independently.

> Please note that you should not use 'nsnull' in interfaces (the current file
> does).

indeed.  thanks for noticing.

i will correct the comment errors before landing the patch.

Comment 16

[Darin Fisher]

fixed-on-trunk

doug: i'll be sending you mail as requested.

Comment 17

[benc]

VERIFIED/lxr:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=Makefile.in&root=/cvsroot&subdir=mozilla/netwerk/base/public&command=DIFF_FRAMESET&rev1=1.77&rev2=1.78

Comment 18

[benc]

http://lxr.mozilla.org/mozilla/source/netwerk/base/public/nsIUploadChannel.idl#49

is actually the only file I'm going to directly verify.

If I need to look at every file in this patch, please let me know.