1485355 - Add methods on nsHttpConnectionMgr to accept PHttpTransactionParent

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P3)

Description

[Honza Bambas (:mayhemer)]

There are following four (if I don't miss anything) methods that operate on nsHttpTransaction objects:

AddTransaction
RescheduleTransaction
UpdateClassOfServiceOnTransaction
CancelTransaction

We can either add a companion method to each of those accepting PHttpTransactionParent that will do the necessary IPC or simply use methods with the same semantic directly on the PHttpTransactionParent object (we will want those anyway, no need to have conn manager IPC somewhere on PNecko or so)

Comment 1

[Junior [inactive]]

Proposed Implementation Detail (naming might change):

Case 1: no socket process, the underlying object is nsHttpTransaction
gHandler->InitiateTransaction(nsATransactionShell*)
mConnMgr->AddTransaction(nsATransactionShell*)
aTransactionShell->DispatchAdd(nsConnectionMgr* )
mConnMgr->PostAddTransaction()
  PostEvent(&nsHttpConnectionMgr::OnMsgNewTransaction) 

Case 2: with socket process, the underlying object is HttpTransactionParent
gHandler->InitiateTransaction(nsATransactionShell*)
mConnMgr->AddTransaction(nsATransactionShell*)
aTransactionShell->DispatchAdd(nsConnectionMgr* )
HttpTransactionParent::SendAdd()
  IPC to socket process

Several advantages:
1. hide the dispatching logic under ConnMgr and transaction objects
2. no need to dynamic cast to let connMgr/handler/channel know the underlying type

Comment 2

[Honza Bambas (:mayhemer)]

I like it!  Just a very small simplification I can think of:

gHandler->InitiateTransaction(nsATransactionShell*)
aTransactionShell->DispatchAdd(this->mConnManager)
mConnMgr->PostAddTransaction()
  PostEvent(&nsHttpConnectionMgr::OnMsgNewTransaction) 

gHandler->InitiateTransaction(nsATransactionShell*)
aTransactionShell->DispatchAdd(this->mConnManager)
HttpTransactionParent::SendAdd()
  IPC to socket process


Or, if mConnMgr is public (would have to look) just do a direct call on the shell (bypass handler):
aTransactionShell->DispatchAdd(gHttpHandler h)  // for nsHttpTrans
{
  h->mConnMgr->Post(this);
}

aTransactionShell->DispatchAdd(gHttpHandler h)  // for TransParent
{
  SendAdd();
}


But this is entirely up to you, whichever way is fine with me.

Comment 3

[Junior [inactive]]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> I like it!  Just a very small simplification I can think of:
> 
> gHandler->InitiateTransaction(nsATransactionShell*)
> aTransactionShell->DispatchAdd(this->mConnManager)
> mConnMgr->PostAddTransaction()
>   PostEvent(&nsHttpConnectionMgr::OnMsgNewTransaction) 
> 
> gHandler->InitiateTransaction(nsATransactionShell*)
> aTransactionShell->DispatchAdd(this->mConnManager)
> HttpTransactionParent::SendAdd()
>   IPC to socket process
> 
> 
> Or, if mConnMgr is public (would have to look) just do a direct call on the
> shell (bypass handler):
> aTransactionShell->DispatchAdd(gHttpHandler h)  // for nsHttpTrans
> {
>   h->mConnMgr->Post(this);
> }
> 
> aTransactionShell->DispatchAdd(gHttpHandler h)  // for TransParent
> {
>   SendAdd();
> }
> 
> 
> But this is entirely up to you, whichever way is fine with me.

Thanks for your good idea. Let's bypass handler.

I was confused before of why we need to tell gHandler all the scheduling things.
The reason is that nsHttpHander did the job of ConnMgr before [1].
Now it's time to disburden nsHttpHandler after 15 years :p

[1] https://github.com/mozilla/gecko-dev/commit/685a7af4683700b762547b1b16a2f454c5750e63

Comment 4

[Junior [inactive]]

nsHttpHandler::InitiateTransaction has other client. Sorry handler.

Comment 5

[Junior [inactive]]

Attachment: Bug 1485355 - implement IPC handles of nsHttpTransaction

(a) able to dispatch Reschedule/Cancel/UpdateClassOfService to socket process
(b) able to send priority to socket priority during init
(c) encapsulate the pump init logic under nsAHttpTransactionShell

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 9013039 [details]
Bug 1485355 - implement IPC handles of nsHttpTransaction

Honza Bambas (:mayhemer) has approved the revision.

Comment 7

[Junior [inactive]]

https://hg.mozilla.org/projects/larch/rev/17204458665390f8649443e04bf0a07b78007125