Darin Fisher Assignee
	Description • 22 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 22 years ago

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...

	Darin Fisher Assignee
	Comment 2 • 21 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 21 years ago

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...
  
  

	Darin Fisher Assignee
	Comment 4 • 21 years ago

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

	Darin Fisher Assignee
	Comment 5 • 21 years ago

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."

	Darin Fisher Assignee
	Comment 6 • 21 years ago

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!

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 21 years ago

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!

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 21 years ago

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

sr=bzbarsky too, I guess. ;)

	Darin Fisher Assignee
	Comment 9 • 21 years ago

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