Move devtools/shared/webconsole/network-monitor next to its actor

RESOLVED FIXED in Firefox 63

Status

enhancement
P2
normal
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

unspecified
Firefox 63
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(10 attachments)

46 bytes, text/x-phabricator-request
Honza
: review+
Details | Review
46 bytes, text/x-phabricator-request
Honza
: review+
Details | Review
46 bytes, text/x-phabricator-request
Honza
: review+
Details | Review
46 bytes, text/x-phabricator-request
Honza
: review+
Details | Review
46 bytes, text/x-phabricator-request
Honza
: review+
Details | Review
46 bytes, text/x-phabricator-request
Honza
: review+
Details | Review
46 bytes, text/x-phabricator-request
Honza
: review+
Details | Review
46 bytes, text/x-phabricator-request
Honza
: review+
Details | Review
46 bytes, text/x-phabricator-request
Honza
: review+
Details | Review
46 bytes, text/x-phabricator-request
Honza
: review+
Details | Review
Assignee

Description

10 months ago
devtools/shared/webconsole/network-monitor.js is only used from the actors and should rather be next to them rather than in shared folder:
  https://searchfox.org/mozilla-central/search?q=network-monitor%22&case=false&regexp=false&path=

Now the naming will be complex as everything is named network-something:
 * devtools/server/actors/network-monitor.js
   NetworkMonitorActor
 * devtools/server/actors/network-event.js
   NetworkEventActor
 * devtools/shared/webconsole/network-monitor.js
   StackTraceCollector, NetworkMonitor, ConsoleProgressListener

First thing would be to split network-monitor.js in three and move ConsoleProgressListener to actors/webconsole/listeners.js (we may split listeners.js if that helps).
Then, I think it would be helful to create a new folder: actors/network-monitor, and move StackTraceCollector in it, as well as NetworkMonitor. But we would have to rename it to make the distinction between NetworkMonitorActor and NetworkMonitor clearer. What about NetworkObserver, or NetworkListener, or... ?
Assignee

Comment 1

10 months ago
honza, what do you think about this renaming plan?
Flags: needinfo?(odvarko)
This is nice clean-up, thanks for working on it!

(In reply to Alexandre Poirot [:ochameau] from comment #0)
> devtools/shared/webconsole/network-monitor.js is only used from the actors
> and should rather be next to them rather than in shared folder:
>  
> https://searchfox.org/mozilla-central/search?q=network-
> monitor%22&case=false&regexp=false&path=
> 
> Now the naming will be complex as everything is named network-something:
>  * devtools/server/actors/network-monitor.js
>    NetworkMonitorActor
>  * devtools/server/actors/network-event.js
>    NetworkEventActor
>  * devtools/shared/webconsole/network-monitor.js
>    StackTraceCollector, NetworkMonitor, ConsoleProgressListener
> 
> First thing would be to split network-monitor.js in three and move
> ConsoleProgressListener to actors/webconsole/listeners.js
All make total sense. We might split it into more files, see below.

> (we may split listeners.js if that helps).
I am usually in favor or splitting files (having one object/component per file), so I am saying yes. No need to have an extra dir for these new listener files of course.

> Then, I think it would be helpful to create a new folder:
> actors/network-monitor, and move StackTraceCollector in it, as well as
Yes, precisely

> NetworkMonitor. But we would have to rename it to make the distinction
> between NetworkMonitorActor and NetworkMonitor clearer. What about
> NetworkObserver, or NetworkListener, or... ?

So, I can image the following:

* ConsoleProgressListener -> actors/webconsole/console-progress-listener.js
* ConsoleServiceListener -> actors/webconsole/console-service-listener.js
* ConsoleAPIListener -> actors/webconsole/console-api-listener.js
* ConsoleReflowListener -> actors/webconsole/console-reflow-listener.js
* ContentProcessListener -> actors/webconsole/console-process-listener.js
* DocumentEventsListener -> actors/webconsole/document-events-listener.js
* StackTraceCollector -> server/actors/network-monitor/stack-trace-collector.js
* NetworkMonitorActor -> server/actors/network-monitor/network-monitor-actor.js

When splitting `devtools/shared/webconsole/network-monitor.js` we might also introduce:
* matchRequest() -> server/actors/network-monitor/utils.js
* ChannelEventSink -> server/actors/network-monitor/channel-event-sink.js
* NetworkResponseListener -> server/actors/network-monitor/network-response-listener.js
* NetworkMonitor -> NetworkObserver -> server/actors/network-monitor/network-observer.js
     * I am picking 'observer' since the impl is based on `Services.obs.addObserver` 

-

I think that file names usually follow the name of the object/component implemented inside.
So, this is why I am suggesting:
* NetworkMonitorActor -> server/actors/network-monitor/network-monitor-actor.js

... and *not* a shortcut like:
* NetworkMonitorActor -> server/actors/network-monitor/actor.js

Even if the dir and file name might look a bit repetitive.

Honza
Flags: needinfo?(odvarko)
Duplicate of this bug: 1483205
Assignee

Comment 15

9 months ago
I changed two things compared to comment 2:
* I've let matchRequest in network-observer as I found it weird to have just this method in utils.js
* I've let NetworkMonitorActor in server/actors/network-monitor.js. All actors are following this pattern (except inspector.js). See highlighters, webconsole, emulation, object.
Comment on attachment 9001917 [details]
Bug 1482070 - Move ConsoleProgressListener to its own file. r=Honza

Jan Honza Odvarko [:Honza] has approved the revision.
Attachment #9001917 - Flags: review+
Comment on attachment 9001918 [details]
Bug 1482070 - Move ConsoleServiceListener to its own file. r=Honza

Jan Honza Odvarko [:Honza] has approved the revision.
Attachment #9001918 - Flags: review+
Comment on attachment 9001919 [details]
Bug 1482070 - Move ConsoleAPIListener to its own file. r=Honza

Jan Honza Odvarko [:Honza] has approved the revision.
Attachment #9001919 - Flags: review+
Comment on attachment 9001920 [details]
Bug 1482070 - Move ConsoleReflowListener to its own file. r=Honza

Jan Honza Odvarko [:Honza] has approved the revision.
Attachment #9001920 - Flags: review+
Comment on attachment 9001921 [details]
Bug 1482070 - Move ContentProcessListener to its own file. r=Honza

Jan Honza Odvarko [:Honza] has approved the revision.
Attachment #9001921 - Flags: review+
Comment on attachment 9001922 [details]
Bug 1482070 - Move DocumentEventsListener to its own file. r=Honza

Jan Honza Odvarko [:Honza] has approved the revision.
Attachment #9001922 - Flags: review+
Comment on attachment 9001924 [details]
Bug 1482070 - Move ChannelEventSink to its own file. r=Honza

Jan Honza Odvarko [:Honza] has approved the revision.
Attachment #9001924 - Flags: review+
Comment on attachment 9001925 [details]
Bug 1482070 - Move StackTraceCollector to its own file. r=Honza

Jan Honza Odvarko [:Honza] has approved the revision.
Attachment #9001925 - Flags: review+
Comment on attachment 9001926 [details]
Bug 1482070 - Rename NetworkMonitor to NetworkObserver and move it next to the NetworkMonitorActor. r=Honza

Jan Honza Odvarko [:Honza] has approved the revision.
Attachment #9001926 - Flags: review+
Comment on attachment 9001927 [details]
Bug 1482070 - Move NetworkResponseListener to its own file. r=Honza

Jan Honza Odvarko [:Honza] has approved the revision.
Attachment #9001927 - Flags: review+

Comment 26

9 months ago
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6c757dc5f0b
Move ConsoleProgressListener to its own file. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/71e21bc8aa29
Move ConsoleServiceListener to its own file. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/985b16e17356
Move ConsoleAPIListener to its own file. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6cb4956e53f
Move ConsoleReflowListener to its own file. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b6d22251680
Move ContentProcessListener to its own file. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/f269b019428d
Move DocumentEventsListener to its own file. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc2fffaa4633
Move ChannelEventSink to its own file. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4b6ce31de8d
Move StackTraceCollector to its own file. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/6871a98b8b0b
Rename NetworkMonitor to NetworkObserver and move it next to the NetworkMonitorActor. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/68bff34cbddd
Move NetworkResponseListener to its own file. r=Honza
You need to log in before you can comment on or make changes to this bug.