Open Bug 1802181 Opened 2 years ago Updated 1 year ago

Switch NetworkObserver to use events instead of owner pattern

Categories

(DevTools :: Netmonitor, task, P3)

Product:
DevTools
Component:
Netmonitor
Type:
task
Points:
1

Tracking

(Not tracked)

People

(Reporter: jdescottes, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [webdriver:backlog])

Attachments

(8 obsolete files)

We already simplified the usage of owner in NetworkObserver, but ultimately, it would still be better if all the information was available as events emitted from the NetworkObserver, instead of having a setup with individual objects and owners.

All events should be emitted by the NetworkObserver itself, not by individual objects dynamically created for each request. This way consumers could setup their listeners as early as needed an avoid potential race conditions.

We should also consolidate the data collected by the NetworkObserver in the httpActivity object. The current code creates a temporary "networkEvent" payload which is used as the basis for creating the devtools specific network event actors and resources. This data should be available on the httpActivity, and all events from the NetworkObserver should emit the httpActivity they relate to.

We can potentially translate all the current add* methods expected on a network event owner, but we can probably simplify and reduce the number of events needed.

Julian, do we have to put this work on our next BiDi milestone or can it wait? What's your feeling?

Flags: needinfo?(jdescottes)

I think it makes sense in m6. I would land a first version of beforeRequestSent, responseStarted and responseCompleted in m5, to get things going. And then do this refactor in m6, to support further improvements for those events.

Flags: needinfo?(jdescottes)

Sounds good. Potentially already marking this bug for M6 then.

Whiteboard: [webdriver:m6]
Priority: P3 → P2

Setting 8 points as discussed during planning on Friday.

Points: --- → 8
Priority: P2 → P1
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

Depends on D168535

I should summarize the changes in the stack.

1/ The NetworkEvent "owner" interface now only expects two methods: addResponseStarted and addResponseCompleted. The creation of the request including parsing request headers and cookies now matches the creation of the networkEvent, and therefore the call to onNetworkEvent.

2/ The NetworkObserver is now the only one responsible for calling "owner" APIs, but since some information is only retrieved by the NetworkResponseListener, I exposed a few methods on the Observer which will be called by the ResponseListener when the content/security info/... is available

3/ We cannot consistently consider responses as completed when we fully retrieved the content. For throttled responses, the transaction will closed after that (and we need to wait for it in order to get correct timings). And retrieving the security info can also happen before or after the content has been retrieved. So we still have some notion of state, but now handled fully in the observer.

I can still switch to events, which was the original intent of this bug, but now that the API went from onNetworkEvent + 11 methods to onNetworkEvent + 2 methods, I think it's a lesser problem.

An immediate added value of this patch is that addResponseCompleted is the end step for all requests, and since this is what we use for BiDi, both DevTools and BiDi should now behave very similarly for the same requests.

There are still a lot of cleanups possible:

  • extract the DevTools timings logic to a separate helper
  • try to use nsITimedChannel to compute all event timings
  • simplify the duplicated information between networkEvent, httpActivity and various actors
  • add browser mochitests now that the API has been simplified

Depends on D168537

Depends on: 1815459
Depends on: 1815460
Depends on: 1815461
Points: 8 → 1

Comment on attachment 9315350 [details]
Bug 1802181 - [devtools] Remove unused arguments for network onEventUpdate

Revision D168535 was moved to bug 1815459. Setting attachment 9315350 [details] to obsolete.

Attachment #9315350 - Attachment is obsolete: true

Comment on attachment 9315351 [details]
Bug 1802181 - [devtools] Rename extraStringData to rawHeaders

Revision D168536 was moved to bug 1815459. Setting attachment 9315351 [details] to obsolete.

Attachment #9315351 - Attachment is obsolete: true

Comment on attachment 9315352 [details]
Bug 1802181 - [devtools] Remove addRequestHeaders and addRequestCookies

Revision D168537 was moved to bug 1815459. Setting attachment 9315352 [details] to obsolete.

Attachment #9315352 - Attachment is obsolete: true

Comment on attachment 9315977 [details]
Bug 1802181 - [devtools] Remove NetworkUtils.createNetworkEvent

Revision D168791 was moved to bug 1815459. Setting attachment 9315977 [details] to obsolete.

Attachment #9315977 - Attachment is obsolete: true

Comment on attachment 9315353 [details]
Bug 1802181 - [devtools] Merge addRequestPostData, addResponseHeaders, addResponseCookies into addResponseStart

Revision D168538 was moved to bug 1815460. Setting attachment 9315353 [details] to obsolete.

Attachment #9315353 - Attachment is obsolete: true

Comment on attachment 9315354 [details]
Bug 1802181 - [devtools] Merge addResponseCache, addEventTimings, addServerTimings and addSecurityInfo in addResponseContent

Revision D168539 was moved to bug 1815461. Setting attachment 9315354 [details] to obsolete.

Attachment #9315354 - Attachment is obsolete: true

Comment on attachment 9315355 [details]
Bug 1802181 - [devtools] Remove networkEvent receivedTypes in favor of a isComplete flag

Revision D168540 was moved to bug 1815461. Setting attachment 9315355 [details] to obsolete.

Attachment #9315355 - Attachment is obsolete: true

Comment on attachment 9315356 [details]
Bug 1802181 - [devtools] Rename event observer callbacks to addResponseStarted/Completed

Revision D168541 was moved to bug 1815461. Setting attachment 9315356 [details] to obsolete.

Attachment #9315356 - Attachment is obsolete: true
Whiteboard: [webdriver:m6] → [webdriver:m7]

This should not block other network work for BiDi and can be tackled later.

Assignee: jdescottes → nobody
Status: ASSIGNED → NEW
Priority: P1 → P3
Whiteboard: [webdriver:m7] → [webdriver:backlog]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: