Add methods on nsHttpConnectionMgr to accept PHttpTransactionParent

RESOLVED FIXED

Status

()

enhancement
P3
normal
RESOLVED FIXED
9 months ago
6 months ago

People

(Reporter: mayhemer, Assigned: junior)

Tracking

(Blocks 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 affected)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 attachment)

Reporter

Description

9 months ago
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)
Priority: -- → P3
Whiteboard: [necko-triaged]
Assignee

Updated

8 months ago
Assignee: nobody → juhsu
Assignee

Comment 1

8 months ago
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
Reporter

Comment 2

8 months ago
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.
Assignee

Comment 3

8 months ago
(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
Assignee

Comment 4

8 months ago
nsHttpHandler::InitiateTransaction has other client. Sorry handler.
Assignee

Comment 5

8 months ago
(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
Reporter

Comment 6

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

Honza Bambas (:mayhemer) has approved the revision.
Attachment #9013039 - Flags: review+
Reporter

Updated

8 months ago
Blocks: 1497232
Reporter

Updated

8 months ago
Blocks: 1497270
Reporter

Updated

8 months ago
No longer blocks: 1497270
Assignee

Comment 7

6 months ago
https://hg.mozilla.org/projects/larch/rev/17204458665390f8649443e04bf0a07b78007125
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.