193917 - incorporate changes from bz's comments in bug 176919

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Darin Fisher]

incorporate changes from bz's comments in bug 176919.  i've waited to
incorporate bz's remaining comments because i didn't want to have to justify
them to drivers.  i will land a patch with all of his comments addressed once
the tree opens for 1.4 alpha.  this is just a tracking bug.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I'm still trying to find time to review the remaining patches there, by the way.
 With any luck I can get that done early enough to land all the updates in alpha...

Comment 2

[Darin Fisher]

marking FIXED now that the patch for bug 210125 has landed.  bz: please correct
me if i'm wrong, but i think i hit all your review comments.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The following comments remain unaddressed:

bug 176919 comment 31:

  Comment on nsIPipe.idl
  Comment on nsInputStreamTee.cpp (SetSink should assert that sink is blocking)
  nsPipe still calls NS_INIT_ISUPPORTS()
  nsPipeInputStream::ReadSegments comments seem to not have been addressed
  Same for WriteSegments
  TestPipes.cpp comments not addressed
  nsEventQueueUtils.h comments not addressed.

I stopped reading at this point (didn't look at the other comments I made on
that bug), since it looks like you never actually checked which comments you did
or did not address...
  
  

Comment 4

[Darin Fisher]

dang.. i think i must have started at comment #37 and somehow overlooked comment
#31.  that's lame of me.

Comment 5

[Darin Fisher]

Attachment: v1 patch

ok, here we go.  this addresses most of the rest of the review comments.  i
believe i may have skipped only a very select few on the basis that "it could
go either way."

Comment 6

[Darin Fisher]

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

seeking r+sr=bz ... please give this a once over.  most changes are just new
comments, so it should be a quick review.  thx!

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

>Index: xpcom/io/nsIPipe.idl

>+ * @param nonBlockingInput
>+ *        true specifies non-blocking input stream behavior
>+ * @param nonBlockingInput
>+ *        true specifies non-blocking output stream behavior

Second one should be nonBlockingOutput

>+ * @param segmentSize
>+ *        specifies the segment size in bytes (pass 0 to use default value)
>+ * @param segmentCount
>+ *        specifies the max number of segments (pass 0 to use default value)

Do we need to mention what the default value is?  In particular whether it's
finite?

>+ * @param maxSize
>+ *        specifies the max size of the pipe (pass 0 to use default value)
>+ *        number of segments is maxSize / segmentSize, and maxSize must be a
>+ *        multiple of segmentSize.

Same here.

>+ * @param nonBlockingInput
>+ *        true specifies non-blocking input stream behavior
>+ * @param nonBlockingInput
>+ *        true specifies non-blocking output stream behavior

Again, nonBlockingOutput.

>Index: netwerk/base/public/nsITransport.idl
>+ * streams to the resource.  The name "transport" is meant to cannote the 
>+ * inherant

"connote" and "inherent"

>Index: netwerk/base/src/nsSocketTransportService2.cpp
>+    NS_ASSERTION(gSocketTransportService, "must not instantiate twice");

Shouldn't that test be reversed?  You want to assert if gSocketTransportService
is non-null....

r=me with those changes.  And thanks for the quick response!

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

sr=bzbarsky too, I guess. ;)

Comment 9

[Darin Fisher]

fixed-on-trunk... thx for being so thorough bz! ;)