This new API is used by MD5 and Assoc-Req headers check (bug 232030, bug 597684). The plan is to first land bug 599164 before those two bugs as those two are considered plugins for pipeline failure detection. But the patch for bug 599164 is dependent on this API, rebasing it would be hard.

Comment on attachment 540108 [details] [diff] [review] v1 (Extracted from bug 597684, attachment 526128 [details] [diff] [review], v5, comment 7, author: Patrick McManus) Review of attachment 540108 [details] [diff] [review]: ----------------------------------------------------------------- This has already been partially reviewed in bug 597684 comment 5, this is based on patch according to the review comments. ::: netwerk/protocol/http/nsAHttpTransaction.h @@ +107,5 @@ > + // Used to inform the connection that it is being used in a pipelined > + // context. That may influence the handling of some errors. > + // The value is the pipeline position. > + virtual nsresult SetPipelineUsed(PRInt32) = 0; > + virtual PRInt32 PipelineUsed() = 0; This has to be called PipelinePosition when working with indexes. I can help with merging this change to following patches. I don't think the first sentence of the comment is correct... Why this should inform just the connection? Anyone can read this or not? You should explicitly say that the index starts at 1 and not 0. But maybe even more detailed comment is appropriate. ::: netwerk/protocol/http/nsHttpConnectionInfo.cpp @@ +110,5 @@ > +PRBool > +nsHttpConnectionInfo::SetSupportsPipelining(PRBool support) > +{ > + if (!mBannedPipelining) > + mSupportsPipelining = support; New line after this line please. @@ +111,5 @@ > +nsHttpConnectionInfo::SetSupportsPipelining(PRBool support) > +{ > + if (!mBannedPipelining) > + mSupportsPipelining = support; > + return mSupportsPipelining; Shouldn't this rather return SupportsPipelining()? ::: netwerk/protocol/http/nsHttpPipeline.cpp @@ +126,5 @@ > + trans->SetPipelineUsed(qlen); > + else > + // do it for this case in case an idempotent cancellation > + // is being repeated and an old value needs to be cleared > + trans->SetPipelineUsed(0); Use PRUint32 for qlen (forgot to mention this earlier, sorry). Please remember that nsTArray::Length returns unsigned int (PRUint32), so keep on consistency with this, on all places. Note that I didn't understand well your comment on my question about this in the original bug comment 6: "we use that as feedback in order to "wade-in" to more aggressive pipelines - at first we want to see some simple short ones succeed before going in all the way. A lonely transaction doesn't make the cut." Patrick, can you please rephrase this a bit? It is still not clear to me why the first transaction should not have the index set. Or maybe I get this understanding while reviewing the following patches? @@ +151,5 @@ > + nsAHttpTransaction *trans = Response(0); > + if (trans) > + return trans->SetPipelineUsed(position); > + return NS_OK; > +} I am not sure whether this ever gets called. I think this should just be an empty method doing nothing or NS_NOTREACHED (if we still want to use this macro). According to the meaning of this method, we probably will never set a "position in a pipeline" for a pipeline. @@ +160,5 @@ > + nsAHttpTransaction *trans = Response(0); > + if (trans) > + return trans->PipelineUsed(); > + return 2; > +} As well here... Can you please explain why this method should return 2 if there is not any transaction response pending? ::: netwerk/protocol/http/nsHttpTransaction.h @@ +200,5 @@ > PRInt16 mPriority; > > PRUint16 mRestartCount; // the number of times this transaction has been restarted > PRUint8 mCaps; > + PRUint32 mIsPipelined; The name has to be mPipelinePosition or mPipelineDepth or whatever you think is more appropriate. But this is no longer a flag in a form "mIsSomething".

Comment on attachment 540108 [details] [diff] [review] v1 (Extracted from bug 597684, attachment 526128 [details] [diff] [review], v5, comment 7, author: Patrick McManus) Actual review in comment. Just marking the review has been done already.