1402888 - nsIStreamTransportService::createInputTransport startOffset and readLimit is not used

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Andrea Marchesini [:baku]]

We can simplify nsIStreamTransportService removing startOffset and readLimit params.
If needed, SlicedInputStream can be used.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - CreateInputTransport

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - nsIInputStreamPump

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - CreateOutputTransport

If you prefer I can file a separate bug. CreateOutputTransport is not used.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8911836 [details] [diff] [review]
part 1 - CreateInputTransport

>     //
>     // OK, we need to use the stream transport service if
>     //
>     // (1) the stream is blocking
>     // (2) the stream does not support nsIAsyncInputStream
>     //
>+    if (mStreamOffset != UINT64_MAX || mStreamLength != UINT64_MAX) {
>+        mStream = new SlicedInputStream(mStream, mStreamOffset, mStreamLength);
>+        mStreamOffset = 0;
Why mStreamOffset = 0; here? It is set to 0 later in the method.

I don't understand 
if (mStreamOffset != UINT64_MAX || mStreamLength != UINT64_MAX) {
Why limit SlicedInputStream creation that way?


(I need to read the other patch too)

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8911836 [details] [diff] [review]
part 1 - CreateInputTransport

but ok, that stuff goes away

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8911836 [details] [diff] [review]
part 1 - CreateInputTransport

No, I still don't understand. Why we add the sliced stream here and then remove it.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8911839 [details] [diff] [review]
part 2 - nsIInputStreamPump

Yeah, you need to explain these patches still again, sorry.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d434da3de9ea
Remove nsIStreamTransportService::createInputStream startOffset and readLimit params, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/560296786c0c
Remove nsIInputStreamPump::Init() offset and limit params, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e992ffa004c
Remove nsIStreamTransportService::createOutputStream, r=smaug

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d434da3de9ea
https://hg.mozilla.org/mozilla-central/rev/560296786c0c
https://hg.mozilla.org/mozilla-central/rev/7e992ffa004c

Comment 10

[Jorg K (CEST = GMT+2)]

You busted TB with that. We DO use those parameters.
https://dxr.mozilla.org/comm-central/search?q=CreateInputTransport&redirect=false

I don't see how to work not having them.

Comment 11

[Jorg K (CEST = GMT+2)]

Or do I need to switch to SlicedInputStream?

Comment 12

[Jorg K (CEST = GMT+2)]

Hmm, SlicedInputStream() doesn't have the aCloseWhenDone parameter, which we also use.

Comment 13

[Jorg K (CEST = GMT+2)]

I forked nsIStreamTransportService into C-C, see bug 1403393.

Comment 14

[Andrea Marchesini [:baku]]

(In reply to Jorg K (GMT+2) from comment #12)
> Hmm, SlicedInputStream() doesn't have the aCloseWhenDone parameter, which we
> also use.

Probably it's better to add aCloseWhenDone parameter in SlicedInputStream instead.

Comment 15

[Jorg K (CEST = GMT+2)]

Up to you. Right now, C-C is busted and I can't allow it to remain busted until you implement that. I can back out my fork later.

Comment 16

[Jorg K (CEST = GMT+2)]

Turned out that we can use this:
-  rv = serv->CreateInputTransport(m_multipleMsgMoveCopyStream, int64_t(offset),
-                                  int64_t(size), false,
+  RefPtr<SlicedInputStream> slicedStream =
+    new SlicedInputStream(slicedStream, offset, uint64_t(size));
+  rv = serv->CreateInputTransport(m_multipleMsgMoveCopyStream, false,
                                   getter_AddRefs(m_transport));
Sorry about the noise.

Comment 17

[Andrea Marchesini [:baku]]

We have too many ways to deal with inputStream and our documentation is not good as it should.