283489 - Expose 64-bit content length on channels

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

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

I have a patch to make HTTP and FTP expose a 64-bit content length via
nsIPropertyBag2.

the patch also makes HTTP, FTP and File channels inherit nsHashPropertyBag.

They no longer implement nsIProperties.

docshell and xpinstall are fixed up accordingly.

Comment 1

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

Attachment: patch

this patch depends on bug 270224's patch

I also made nsFtpConnectionThread own a nsRefPtr<nsFTPChannel> rather than
nsCOMPtr<nsIFTPChannel>, simplifying some code.

Comment 2

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

so, question...

"content-length" is a quite generic thing. But the other properties we have in
the tree live in namespaces (docshell.*, test.*).

Should content-length too? Maybe general.content-length?

Comment 3

[Darin Fisher]

yeah, i wonder about that too.  there are two issues:

1) we want to keep property names simple (think: request.props["content-length"])
2) we want to give consumers the ability to store arbitrary properties on the
channel.

we want to avoid conflicts between necko defined properties and consumer defined
properties.

Comment 4

[Darin Fisher]

btw, the patch looks really great.  let's just sort out this issue, and i think
we'll be good to go.

Comment 5

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

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

ok, I'll go with just "content-length", to keep names simple.

user-defined properties should have some prefix.

I'll add a nsChannelProperties.h file containing the properties exposed by
necko.

I don't think IDL can have string constants, unfortunately...

Comment 6

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

Attachment: patch v2

Maybe nsChannelProperties.h should include the NS_LITERAL_STRING(..) so users
don't have to?

Comment 7

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

Attachment: interdiff v1/v2

Comment 8

[Darin Fisher]

To make this efficient we should probably do something like this for necko:
http://lxr.mozilla.org/seamonkey/source/extensions/webservices/soap/src/nsSOAPUtils.h#87

We might want to think about tricks that we could play to leverage something
like this throughout gecko when we link it all together statically or as part of
libxul.

Comment 9

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

(In reply to comment #8)
> To make this efficient we should probably do something like this for necko:
>
http://lxr.mozilla.org/seamonkey/source/extensions/webservices/soap/src/nsSOAPUtils.h#87

wouldn't be useful for channels outside of libnecko, is that a concern?

Comment 10

[Darin Fisher]

think in terms of libxul :-)

no, but seriously... if done right, we could control how these are used
depending on how we are building gecko.  it probably just takes some clever
macroing.

Comment 11

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

(In reply to comment #10)
> think in terms of libxul :-)

last I checked mailnews still had some channel implementations, and won't be
part of libxul :-) (not to mention extensions...)

> no, but seriously... if done right, we could control how these are used
> depending on how we are building gecko.  it probably just takes some clever
> macroing.

ah, so depending on whether (say) _IMPL_NS_NECKO is defined, this uses the
NS_LITERAL_STRING or such a class member? that'd work... would require spreading
some defining over the necko makefiles (not in the dirs that end up in
necko2.dll though :) )

Comment 12

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

of course the question is, does the cost nsDependentString c/dtor matter? (I'm
assuming here that the compiler supports 2-byte wchar_t, to my knowledge the
win/mac/linux compilers all do)

Comment 13

[Darin Fisher]

constructing a nsDependentString means initializing 4 words on the stack, and
then you have to push the address of that object when calling a function.  if
you already have the object, then you save on codesize.

i think we should do away with necko2 fwiw, and just merge the two libraries.

Comment 14

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

Attachment: patch v3

something like this, then?

Comment 15

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

Attachment: interdiff v2/v3 (excluding Makefile.in changes)

(this interdiff does not show all the makefile changes which only add a
-D_IMPL_NS_NECKO)

Comment 16

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

Attachment: interdiff v2/v3 (Makefile.in changes only)

Comment 17

[Darin Fisher]

I think you should move the nsNeckoStrings thingy into its own header file. 
Then include it in nsChannelProperties.h optionally.  The reason for this is
that we might have some wide strings that we want to use in necko that are not
channel properties.

Comment 18

[Darin Fisher]

nsNeckoStrings.cpp would be nice too instead of putting everything in
nsNetModule.cpp.  Otherwise, this looks pretty good.  How do you feel about
NS_CHANNEL_PROP_CONTENT_LENGTH instead of NS_CHANNEL_PROPERTY_CONTENT_LENGTH? 
Less typing, and it is still plenty readable in my opinion.

Nit: watch out for overly long lines.  Try to break at 80 chars.

Comment 19

[Darin Fisher]

Also, nsNetStrings might be better than nsNeckoStrings since we generally don't
refer to "necko" in code like that ;-)

Comment 20

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

(In reply to comment #18)
> NS_CHANNEL_PROP_CONTENT_LENGTH instead of NS_CHANNEL_PROPERTY_CONTENT_LENGTH? 
> Less typing, and it is still plenty readable in my opinion.

Good idea. I had problems fitting lines within 80 characters, and with no
obvious place to break them.

Comment 21

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

Attachment: patch v4

with those changes.

Things I'm unsure about:
- should I move the gNeckoStrings into a NS_InitNetStrings function,
implemented in nsNetStrings.cpp and called from nsNetModule?
- should nsNetStrings.h be exported? I'm doing that now, but it's really a
necko-internal header...

Comment 22

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

Attachment: interdiff v3/v4

it was a pain to generate this interdiff. I had to mangle the patch a bit, so
the shown context may not match current cvs. the actual changes should be
right, though.

Comment 23

[Darin Fisher]

Comment on attachment 178191 [details] [diff] [review]
patch v4

also, please change _IMPL_NS_NECKO to IMPL_NS_NET (see bug 266011)


> +    PRUint32                          mSetCacheToken            : 1;

looks like that is part of some other patch.


oh, and nsNeckoStartup should be nsNetStartup.


r=darin with those changes.

Comment 24

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

(In reply to comment #23)
> oh, and nsNeckoStartup should be nsNetStartup.

I was trying to be consistent with the existing nsNeckoShutdown; should I change
both?

Comment 25

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

Attachment: patch v5

this also changes nsNeckoShutdown to nsNetShutdown

Comment 26

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

Attachment: interdiff v4/v5

Comment 27

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

please ignore the xpinstall makefile change. that one depends on other changes
that are not checked in yet.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 178746 [details] [diff] [review]
patch v5

>Index: netwerk/base/public/nsIChannel.idl
>+     * Callers should prefer getting the "content-length" property
>+     * using nsIPropertyBag2 as 64-bit value, if available.

"as a 64-bit value by QIing the channel to nsIPropertyBag2, if that interface
is exposed by the channel".

>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

Note that nsHttpChannel::SetContentLength throws... Arguably, we should make
the 64-bit version throw as well, for consistency.  Otherwise we can easily get
mismatches between the two numbers even while in the 32-bit range.

sr=bzbarsky with those nits picked.

Comment 29

[Darin Fisher]

So, it sounds like we might want to have a way to lock properties as readonly.  Hmm.

Comment 30

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

I checked this patch in since freeze is tonight.

Bug 289162 filed on locking properties.

Comment 31

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

Attachment: fix leak regression

Comment 32

[Brendan Eich [:brendan]]

Comment on attachment 179870 [details] [diff] [review]
fix leak regression

a=[e-mail]

/be

Comment 33

[Darin Fisher]

> Created an attachment (id=179870) [edit]
> fix leak regression
 
We should probably get in the habit of putting non-virtual destructors in
private sections.

Comment 34

[Peter van der Woude [:Peter6]]

did this fix cause bug 289724 ? (Mac-only)