Implement a network monitor

RESOLVED FIXED in Firefox 23

Status

defect
P1
normal
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: vporof, Assigned: vporof)

Tracking

({doc-bug-filed})

unspecified
Firefox 23
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 23+)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(2 attachments, 5 obsolete attachments)

Assignee

Description

6 years ago
No description provided.
Assignee

Comment 1

6 years ago
Posted patch v1 (obsolete) — Splinter Review
In the interest of landing this before Rob returns, let the reviews commence.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #730427 - Flags: review?(mihai.sucan)
Attachment #730427 - Flags: review?(dcamp)
Assignee

Comment 2

6 years ago
Posted patch v1.1 (obsolete) — Splinter Review
Removed some retarded css and added better support for application/json responses. Sorry about the double r?.
Attachment #730427 - Attachment is obsolete: true
Attachment #730427 - Flags: review?(mihai.sucan)
Attachment #730427 - Flags: review?(dcamp)
Attachment #730725 - Flags: review?(mihai.sucan)
Attachment #730725 - Flags: review?(dcamp)
(can we get UX involved in this?)
Comment on attachment 730725 [details] [diff] [review]
v1.1

Review of attachment 730725 [details] [diff] [review]:
-----------------------------------------------------------------

So far:

::: browser/devtools/shared/widgets/ViewHelpers.jsm
@@ +140,5 @@
> +   */
> +  togglePane: function VH_togglePane(aFlags, aPane, aTarget = aPane,
> +                                     aCollapsedTooltip = "",
> +                                     aExpandedTooltip = "") {
> +    // Hiding is always handled via margins, not the hidden attribute.

This is a kinda opaque API.  Maybe the DOM nodes belong as an options object rather than a series of positional, optional arguments?  Maybe the collection of things that make up a "pane" (node, target, collapsed tooltips) should be stored as an object?

This isn't new code, so I won't hold up review over it.

::: browser/themes/linux/devtools/netmonitor.css
@@ +113,5 @@
> +      transparent 25%,
> +      rgba(255,255,255,0.02) 25%,
> +      rgba(255,255,255,0.02) 75%,
> +      transparent 75%);
> +/* FIXME: this is so incredibly fucking slow, need to do it in a different way. */

Did you mean to leave this dead code in here?  Would prefer a pointer to a bug to improve things, with the commented-out code there if it might be useful.
Comment on attachment 730725 [details] [diff] [review]
v1.1

Review of attachment 730725 [details] [diff] [review]:
-----------------------------------------------------------------

Some more:

::: browser/devtools/netmonitor/netmonitor-controller.js
@@ +17,5 @@
> +Cu.import("resource:///modules/devtools/ViewHelpers.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "Promise",
> +  "resource://gre/modules/commonjs/sdk/core/promise.js");
> +

Is it actually useful to lazy load Promise?  Surely it has already been loaded by this point...

@@ +45,5 @@
> +   */
> +  startupNetMonitor: function NC_startupNetMonitor() {
> +    if (this._isInitialized) {
> +      return;
> +    }

Rather than a second call to startupNetMonitor returning undefined, could it just return the same initialization promise?  Both callers could then be notified when the net monitor is initialized.

If you're confident there will ever only be one, correct, startupNetMonitor call, does this need to exist at all?

@@ +65,5 @@
> +   * @return object
> +   *         A promise that is resolved when the monitor finishes shutdown.
> +   */
> +  shutdownNetMonitor: function NC__shutdownDebugger() {
> +    if (this._isDestroyed || !NetMonitorView._isInitialized) {

If I call shutdownNetMonitor() before NetMonitorView is initialized won't I end up with an initialized, not-shut-down NetMonitorView with this if statement?

@@ +85,5 @@
> +    return deferred.promise;
> +  },
> +
> +  /**
> +   * Initializes a debugger client and connects it to the debugger server,

This comment isn't really true anymore, is it?

@@ +140,5 @@
> +        return;
> +      }
> +      this.tabClient = aTabClient;
> +
> +      aClient.attachConsole(aTabGrip.consoleActor, LISTENERS, (aResponse, aWebConsoleClient) => {

Does this share well with the web console?  As far as I can tell they're sharing a web console actor - does this mean they'll fight over which listeners are active?

@@ +218,5 @@
> +
> +  /**
> +   * Remove events emitted by the current tab target.
> +   */
> +  disconnect: function TEH_disconnect() {

fwiw, spidermonkey does a decent job of inferring function names now, so we can ditch the "TEH_disconnect" and similar throughout the code if you want.

::: browser/devtools/shared/widgets/ViewHelpers.jsm
@@ +140,5 @@
> +   */
> +  togglePane: function VH_togglePane(aFlags, aPane, aTarget = aPane,
> +                                     aCollapsedTooltip = "",
> +                                     aExpandedTooltip = "") {
> +    // Hiding is always handled via margins, not the hidden attribute.

This is a kinda opaque API.  Maybe the DOM nodes belong as an options object rather than a series of positional, optional arguments?  Maybe the collection of things that make up a "pane" (node, target, collapsed tooltips) should be stored as an object?

This isn't new code, so I won't hold up review over it.

::: browser/themes/linux/devtools/netmonitor.css
@@ +113,5 @@
> +      transparent 25%,
> +      rgba(255,255,255,0.02) 25%,
> +      rgba(255,255,255,0.02) 75%,
> +      transparent 75%);
> +/* FIXME: this is so incredibly fucking slow, need to do it in a different way. */

Did you mean to leave this dead code in here?  Would prefer a pointer to a bug to improve things, with the commented-out code there if it might be useful.
Assignee

Comment 6

6 years ago
(In reply to Dave Camp (:dcamp) from comment #5)
> 
> Did you mean to leave this dead code in here?  Would prefer a pointer to a
> bug to improve things, with the commented-out code there if it might be
> useful.

Yeah, haven't decided what to do about it yet.. If I'll fix this in a followup, I'll remove the commented-out code.
Assignee

Comment 7

6 years ago
(In reply to Dave Camp (:dcamp) from comment #5)
> 
> Is it actually useful to lazy load Promise?  Surely it has already been
> loaded by this point...
> 

Yeah, it's probably already loaded. I'll switch to a plain import.

> @@ +45,5 @@
> > +   */
> > +  startupNetMonitor: function NC_startupNetMonitor() {
> > +    if (this._isInitialized) {
> > +      return;
> > +    }
> 
> Rather than a second call to startupNetMonitor returning undefined, could it
> just return the same initialization promise?  Both callers could then be
> notified when the net monitor is initialized.
> 

Good idea.

> If you're confident there will ever only be one, correct, startupNetMonitor
> call, does this need to exist at all?
> 

I'm paranoid.

> @@ +65,5 @@
> > +   * @return object
> > +   *         A promise that is resolved when the monitor finishes shutdown.
> > +   */
> > +  shutdownNetMonitor: function NC__shutdownDebugger() {
> > +    if (this._isDestroyed || !NetMonitorView._isInitialized) {
> 
> If I call shutdownNetMonitor() before NetMonitorView is initialized won't I
> end up with an initialized, not-shut-down NetMonitorView with this if
> statement?
> 

That's the point. You shouldn't be allowed to shut down the netmonitor before you finished initializing it. This potentially avoids a huge class of dumb mistakes. Remember the debugger?

> 
> @@ +140,5 @@
> > +        return;
> > +      }
> > +      this.tabClient = aTabClient;
> > +
> > +      aClient.attachConsole(aTabGrip.consoleActor, LISTENERS, (aResponse, aWebConsoleClient) => {
> 
> Does this share well with the web console?  As far as I can tell they're
> sharing a web console actor - does this mean they'll fight over which
> listeners are active?
> 

Apparently yes. Argh.

> @@ +218,5 @@
> > +
> > +  /**
> > +   * Remove events emitted by the current tab target.
> > +   */
> > +  disconnect: function TEH_disconnect() {
> 
> fwiw, spidermonkey does a decent job of inferring function names now, so we
> can ditch the "TEH_disconnect" and similar throughout the code if you want.
> 

I'm having mixed feelings about this.

> ::: browser/devtools/shared/widgets/ViewHelpers.jsm
> @@ +140,5 @@
> > +   */
> > +  togglePane: function VH_togglePane(aFlags, aPane, aTarget = aPane,
> > +                                     aCollapsedTooltip = "",
> > +                                     aExpandedTooltip = "") {
> > +    // Hiding is always handled via margins, not the hidden attribute.
> 
> This is a kinda opaque API.  Maybe the DOM nodes belong as an options object
> rather than a series of positional, optional arguments?  Maybe the
> collection of things that make up a "pane" (node, target, collapsed
> tooltips) should be stored as an object?
> 

Agreed. Followup? Webconsole's also probably going to use this after (during) bug 808370, so we can discuss more about it then.
Assignee

Comment 8

6 years ago
Posted patch v1.2 (obsolete) — Splinter Review
Addressed comments and simplified togglePane's API.
Attachment #730725 - Attachment is obsolete: true
Attachment #730725 - Flags: review?(mihai.sucan)
Attachment #730725 - Flags: review?(dcamp)
Attachment #731122 - Flags: review?(mihai.sucan)
Attachment #731122 - Flags: review?(dcamp)
(In reply to Victor Porof [:vp] from comment #7)
> Agreed. Followup? Webconsole's also probably going to use this after
> (during) bug 808370, so we can discuss more about it then.

That won't happen during bug 808370, unfortunately. That patch is done, waiting on unsafeDereference and some greener try runs.

Do you plan to land this patch before the aurora merge? Do you have tests on the way? Afaik new features need tests to land.
(In reply to Victor Porof [:vp] from comment #7)

> > @@ +65,5 @@
> > > +   * @return object
> > > +   *         A promise that is resolved when the monitor finishes shutdown.
> > > +   */
> > > +  shutdownNetMonitor: function NC__shutdownDebugger() {
> > > +    if (this._isDestroyed || !NetMonitorView._isInitialized) {
> > 
> > If I call shutdownNetMonitor() before NetMonitorView is initialized won't I
> > end up with an initialized, not-shut-down NetMonitorView with this if
> > statement?
> > 
> 
> That's the point. You shouldn't be allowed to shut down the netmonitor
> before you finished initializing it. This potentially avoids a huge class of
> dumb mistakes. Remember the debugger?

Yeah, I'm saying this is still prone to dumb mistakes.  You can call shutdownNetMonitor(), get no indication that anything went wrong, and have a fully-constructed and active network monitor when you're done.

That's assuming that NetMonitorView's initialization doesn't happen immediately, which it does.  But since NMV's API implies that initialization *could* be async, I think it's worth either throwing an error from this function or making it work correctly if called mid-initialization.

> 
> > @@ +218,5 @@
> > > +
> > > +  /**
> > > +   * Remove events emitted by the current tab target.
> > > +   */
> > > +  disconnect: function TEH_disconnect() {
> > 
> > fwiw, spidermonkey does a decent job of inferring function names now, so we
> > can ditch the "TEH_disconnect" and similar throughout the code if you want.
> > 
> 
> I'm having mixed feelings about this.

That's fine, just letting you know in case you care.

> > This is a kinda opaque API.  Maybe the DOM nodes belong as an options object
> > rather than a series of positional, optional arguments?  Maybe the
> > collection of things that make up a "pane" (node, target, collapsed
> > tooltips) should be stored as an object?
> > 
> 
> Agreed. Followup? Webconsole's also probably going to use this after
> (during) bug 808370, so we can discuss more about it then.

Followup is fine.
(In reply to Mihai Sucan [:msucan] from comment #9)

> Do you plan to land this patch before the aurora merge? Do you have tests on
> the way? Afaik new features need tests to land.

I don't see landing before the aurora merge being too likely.  Tests are necessary, I asked Victor to post the patch itself for review while he wrote tests to streamline the process.
Comment on attachment 731122 [details] [diff] [review]
v1.2

Review of attachment 731122 [details] [diff] [review]:
-----------------------------------------------------------------

Nice patch.  Using feedback+ instead of r+ so that this doesn't land without tests, but I like it.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +564,5 @@
> +      // another scaled node. In this case, apply a reversed transformation.
> +      let revScaleX = "scaleX(" + (1 / scale) + ")";
> +
> +      // Scale the waterfall background to accurately represent timing steps.
> +      // FIXME: this is so incredibly fucking slow, need to do it in a different way.

Either fix or make sure there's a bug filed.  "incredibly fucking slow" is not building my confidence here...
Attachment #731122 - Flags: review?(dcamp) → feedback+
Assignee

Comment 13

6 years ago
(In reply to Mihai Sucan [:msucan] from comment #9)
> 
> That won't happen during bug 808370, unfortunately. That patch is done,
> waiting on unsafeDereference and some greener try runs.
> 

Ok, I'm fine either way.

> Do you plan to land this patch before the aurora merge? Do you have tests on
> the way? Afaik new features need tests to land.

Most definitely not before the merge. Sometime after that would be best to get a full cycle of tinkering and playing with it. Tests are on the way.


> 
> Yeah, I'm saying this is still prone to dumb mistakes.
> 

Won't you be instantly aware of these dumb mistakes though? Most of the times that happened in tests, and now if a test won't be able to close the network monitor, I assert that'd be pretty... evident.

> That's assuming that NetMonitorView's initialization doesn't happen
> immediately, which it does.  But since NMV's API implies that initialization
> *could* be async, I think it's worth either throwing an error from this
> function or making it work correctly if called mid-initialization.
> 

However, that's hood point.

(In reply to Dave Camp (:dcamp) from comment #12)
> Comment on attachment 731122 [details] [diff] [review]
> v1.2
> 
> Review of attachment 731122 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice patch.  Using feedback+ instead of r+ so that this doesn't land without
> tests, but I like it.
> 
> ::: browser/devtools/netmonitor/netmonitor-view.js
> @@ +564,5 @@
> > +      // another scaled node. In this case, apply a reversed transformation.
> > +      let revScaleX = "scaleX(" + (1 / scale) + ")";
> > +
> > +      // Scale the waterfall background to accurately represent timing steps.
> > +      // FIXME: this is so incredibly fucking slow, need to do it in a different way.
> 
> Either fix or make sure there's a bug filed.

Obviously.
Blocks: 581352
Comment on attachment 731122 [details] [diff] [review]
v1.2

Review of attachment 731122 [details] [diff] [review]:
-----------------------------------------------------------------

General comments:

- this looks really cool. Awesome work!
- you should show "refresh the page to see network requests" (or something) when the tool is first opened. It looks like initialization failed. I actually checked the error console before figuring out it's just an empty network pane.
- allow keyboard navigation inside the list of network requests. This quickly tripped me off - I wanted to press Up/Down/Home/End/Page Up/Down to go through the list of network requests.
- please allow sorting by column (type, size, time, name, etc) and resizing the columns.
- do you need "filter headers" input? The sidebar looks rather crowded...
- at the bottom of the sidebar you have "Headers size:" and you show the size of the request and response headers. This seems to be too wide for my screen and slightly superfluous. Why not reuse the "Request headers" and the "Response headers" heading? (the scope in variables view parlance). Put the size in parenthesis:

   Request headers (1.3 KB)
     header1: value1
     ...
   Response headers (1.4 KB) 
     header1: value1
     ...
- or... do we actually need to display the size?

- timings look incomplete with that background. I'm not sure what to suggest in terms of UI, but that was my first impression. Can you add page timings as well? Like onload, content loaded, etc.

- I'd like some consistency between the various sidebars we have. In bug 808370 I'm adding a sidebar to the web console that reuses the sidebar implementation from the Inspector. Here you reuse the debugger's sidebar implementation. Do we have a bug about consistency across tools?

- Any tests incoming?

For some of the stuff above you probably want to open followup bugs.

More comments below. I see dcamp already covered the stuff about initialization and promises.

Giving this patch r+, with the comments below addressed and followup bug reports for what can't be fixed here. Also, we should have some tests, unless Dave agrees with this feature to land without any tests. Thank you!

::: browser/devtools/framework/Target.jsm
@@ +250,5 @@
>          DebuggerServer.addBrowserActors();
>        }
>  
>        this._client = new DebuggerClient(DebuggerServer.connectPipe());
> +

stray change...

::: browser/devtools/netmonitor/netmonitor-controller.js
@@ +52,5 @@
> +   *
> +   * @return object
> +   *         A promise that is resolved when the monitor finishes shutdown.
> +   */
> +  shutdownNetMonitor: function NC__shutdownNetMonitor() {

This is inconsistent: first time you execute it you get a promise, while subsequent calls return undefined. You should return the same promise such that any subsequent calls will get the same promise, and all callers will be notified when the promise is resolved.

@@ +80,5 @@
> +   *
> +   * @return object
> +   *         A promise that is resolved when the monitor finishes connecting.
> +   */
> +  connect: function NC_connect() {

You might want to always return the same promise here, while the connection is active.

@@ +117,5 @@
> +   *        A function to invoke once the client attached to the console client.
> +   */
> +  _startMonitoringTab: function NC__startMonitoringTab(aClient, aTabGrip, aCallback) {
> +    if (!aClient) {
> +      Cu.reportError("No client found!");

Is this an expected error? When can this error occur? If this is not expected, then I suggest removing this check - let the code throw exceptions when completely unexpected behavior happens.

@@ +124,5 @@
> +    this.client = aClient;
> +
> +    aClient.attachTab(aTabGrip.actor, (aResponse, aTabClient) => {
> +      if (!aTabClient) {
> +        Cu.reportError("No tab client found!");

Same concern as above.

@@ +131,5 @@
> +      this.tabClient = aTabClient;
> +
> +      aClient.attachConsole(aTabGrip.consoleActor, LISTENERS, (aResponse, aWebConsoleClient) => {
> +        if (!aWebConsoleClient) {
> +          Cu.reportError("Couldn't attach to console: " + aResponse.error);

Ditto and other similar occurrences.

@@ +250,5 @@
> +
> +/**
> + * Functions handling target network events.
> + */
> +function NetworkEventsHandler() {

This is very nice code!

How did you find using the network event actors? The API and the protocol design.

@@ +412,5 @@
> +    });
> +  },
> +
> +  /**
> +   * Handles additional information received for a "eventTimings" packet.

Do you mean "responseContent"?

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +340,5 @@
> +          // The information in the packet is empty, it can be safely ignored.
> +          continue;
> +        }
> +
> +        switch (key) {

This looks quite hacky and possibly easy to break whenever packets change, since you iterate over the properties available in each networkEventUpdate packet. That's not ideal. Why not use packet.updateType as the basis for this switch block? I'd like to see this fixed.

::: browser/devtools/netmonitor/netmonitor.xul
@@ +22,5 @@
> +      <toolbar id="requests-menu-toolbar"
> +               class="devtools-toolbar"
> +               align="center">
> +        <label id="requests-menu-status-and-method-label"
> +               class="plain requests-menu-header requests-menu-status-and-method"

Interesting to see all labels have the |plain| class.

::: browser/devtools/shared/widgets/SideMenuWidget.jsm
@@ +80,5 @@
>     *         The element associated with the displayed item.
>     */
>    insertItemAt: function SMW_insertItemAt(aIndex, aContents, aTooltip = "", aGroup = "") {
>      this.ensureSelectionIsVisible(true, true); // Don't worry, it's delayed.
> +

stray change

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +1865,5 @@
>     *             - { value: { type: "object", class: "Object" } }
>     *             - { get: { type: "object", class: "Function" },
>     *                 set: { type: "undefined" } }
> +   * @param boolean aRelaxed
> +   *        True if name duplicates should be allowed.

How does this work? this._store is a map and you add properties by name, so you end up overriding existing stored properties. Am I missing something?

@@ +1929,5 @@
>     *        The raw object you want to display.
>     * @param object aOptions [optional]
>     *        Additional options for adding the properties. Supported options:
>     *        - sorted: true to sort all the properties before adding them
> +   *        - expanded: true to exand all the properties after adding them

typo: s/exand/expand/

::: browser/devtools/shared/widgets/ViewHelpers.jsm
@@ +103,5 @@
> +   *        A character set.
> +   * @return string
> +   *         A unicode string.
> +   */
> +  convertToUnicode: function SU_convertToUnicode(aString, aCharset) {

This looks like a duplicate of NetworkHelper.convertToUnicode(). Can you reuse that? hg locate NetworkHelper.jsm.
Attachment #731122 - Flags: review?(mihai.sucan) → review+
Comment on attachment 731122 [details] [diff] [review]
v1.2

Switching to f+, until we have tests (for consistency with Dave).
Attachment #731122 - Flags: review+ → feedback+
Assignee

Comment 16

6 years ago
(In reply to Mihai Sucan [:msucan] from comment #14)
> Comment on attachment 731122 [details] [diff] [review]
> v1.2
> 
> Review of attachment 731122 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> General comments:
> 
> - you should show "refresh the page to see network requests" (or something)
> when the tool is first opened. It looks like initialization failed. I
> actually checked the error console before figuring out it's just an empty
> network pane.

Followup?

> - allow keyboard navigation inside the list of network requests. This
> quickly tripped me off - I wanted to press Up/Down/Home/End/Page Up/Down to
> go through the list of network requests.

Agreed, definitely followup.

> - please allow sorting by column (type, size, time, name, etc) and resizing
> the columns.

I'm not sure about resizing the columns. I'd like to design that to have a more "menu" feel, not a "table" feel.

> - do you need "filter headers" input? The sidebar looks rather crowded...

I don't know. Probably not?

> - at the bottom of the sidebar you have "Headers size:" and you show the
> size of the request and response headers. This seems to be too wide for my
> screen and slightly superfluous. Why not reuse the "Request headers" and the
> "Response headers" heading? (the scope in variables view parlance). Put the
> size in parenthesis:

That's a great idea!

> - or... do we actually need to display the size?

I think it could potentially be useful information.

> - timings look incomplete with that background. I'm not sure what to suggest
> in terms of UI, but that was my first impression. Can you add page timings
> as well? Like onload, content loaded, etc.
> 

Obviously. We need timing delimitation marks (see REQUESTS_WATERFALL_BACKGROUND_PATTERN) but I need to make sure I'm not slowing things down. Onload and DOMContentReady event lines certainly need to be shown as well, but it'll happen in a followup.

> - I'd like some consistency between the various sidebars we have. In bug
> 808370 I'm adding a sidebar to the web console that reuses the sidebar
> implementation from the Inspector. Here you reuse the debugger's sidebar
> implementation. Do we have a bug about consistency across tools?
> 

How am I reusing debugger's sidebar implementation? It's a tabbox with tabpanels, just like your work in 808370. If anything, It's completely different from the debugger :) Maybe you mean the pane collapse/expand mechanism? That should work with anything.

> - Any tests incoming?
> 

Yes!

> More comments below. I see dcamp already covered the stuff about
> initialization and promises.
> 
> Giving this patch r+, with the comments below addressed and followup bug
> reports for what can't be fixed here. Also, we should have some tests,
> unless Dave agrees with this feature to land without any tests. Thank you!
> 

This shouldn't land without tests, no!

> 
> @@ +117,5 @@
> > +   *        A function to invoke once the client attached to the console client.
> > +   */
> > +  _startMonitoringTab: function NC__startMonitoringTab(aClient, aTabGrip, aCallback) {
> > +    if (!aClient) {
> > +      Cu.reportError("No client found!");
> 
> Is this an expected error? When can this error occur? If this is not
> expected, then I suggest removing this check - let the code throw exceptions
> when completely unexpected behavior happens.
> 
> @@ +124,5 @@
> > +    this.client = aClient;
> > +
> > +    aClient.attachTab(aTabGrip.actor, (aResponse, aTabClient) => {
> > +      if (!aTabClient) {
> > +        Cu.reportError("No tab client found!");
> 
> Same concern as above.
> 
> @@ +131,5 @@
> > +      this.tabClient = aTabClient;
> > +
> > +      aClient.attachConsole(aTabGrip.consoleActor, LISTENERS, (aResponse, aWebConsoleClient) => {
> > +        if (!aWebConsoleClient) {
> > +          Cu.reportError("Couldn't attach to console: " + aResponse.error);
> 
> Ditto and other similar occurrences.
> 

It's mirroring the debugger implementation, mostly just mechanical copy pasting. There's not specific reason I can think of for these errors to happen, but there's no harm in logging, no?

> @@ +250,5 @@
> > +
> > +/**
> > + * Functions handling target network events.
> > + */
> > +function NetworkEventsHandler() {
> 
> This is very nice code!
> 
> How did you find using the network event actors? The API and the protocol
> design.
>

It was quite delicious :) Knowing when to ask for request updates was a bit tricky to understand, but obvious after that.

> @@ +412,5 @@
> > +    });
> > +  },
> > +
> > +  /**
> > +   * Handles additional information received for a "eventTimings" packet.
> 
> Do you mean "responseContent"?
> 

Evidently :)

> ::: browser/devtools/netmonitor/netmonitor-view.js
> @@ +340,5 @@
> > +          // The information in the packet is empty, it can be safely ignored.
> > +          continue;
> > +        }
> > +
> > +        switch (key) {
> 
> This looks quite hacky and possibly easy to break whenever packets change,
> since you iterate over the properties available in each networkEventUpdate
> packet. That's not ideal. Why not use packet.updateType as the basis for
> this switch block? I'd like to see this fixed.
> 

That's not what happens.

These properties are part of the object literal passed when calling updateRequest from the controller. See _onNetworkEventUpdate and all its friends.

> ::: browser/devtools/netmonitor/netmonitor.xul
> @@ +22,5 @@
> > +      <toolbar id="requests-menu-toolbar"
> > +               class="devtools-toolbar"
> > +               align="center">
> > +        <label id="requests-menu-status-and-method-label"
> > +               class="plain requests-menu-header requests-menu-status-and-method"
> 
> Interesting to see all labels have the |plain| class.
> 

That's to override all the awkward margins and padding characteristic to a label. The plain class is a default style class: https://developer.mozilla.org/en-US/docs/XUL/label#s-plain

> 
> ::: browser/devtools/shared/widgets/VariablesView.jsm
> @@ +1865,5 @@
> >     *             - { value: { type: "object", class: "Object" } }
> >     *             - { get: { type: "object", class: "Function" },
> >     *                 set: { type: "undefined" } }
> > +   * @param boolean aRelaxed
> > +   *        True if name duplicates should be allowed.
> 
> How does this work? this._store is a map and you add properties by name, so
> you end up overriding existing stored properties. Am I missing something?
> 

This works poorly, but we're stretching the "variables view" metaphor a bit with the network monitor, so I'm not worried about it.

> 
> ::: browser/devtools/shared/widgets/ViewHelpers.jsm
> @@ +103,5 @@
> > +   *        A character set.
> > +   * @return string
> > +   *         A unicode string.
> > +   */
> > +  convertToUnicode: function SU_convertToUnicode(aString, aCharset) {
> 
> This looks like a duplicate of NetworkHelper.convertToUnicode(). Can you
> reuse that? hg locate NetworkHelper.jsm.

Uuuuh!
(In reply to Victor Porof [:vp] from comment #16)
> (In reply to Mihai Sucan [:msucan] from comment #14)
> > Comment on attachment 731122 [details] [diff] [review]
> > v1.2
> > 
> > Review of attachment 731122 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > General comments:
> > 
> > - you should show "refresh the page to see network requests" (or something)
> > when the tool is first opened. It looks like initialization failed. I
> > actually checked the error console before figuring out it's just an empty
> > network pane.
> 
> Followup?

Sure.

> > - allow keyboard navigation inside the list of network requests. This
> > quickly tripped me off - I wanted to press Up/Down/Home/End/Page Up/Down to
> > go through the list of network requests.
> 
> Agreed, definitely followup.

Yep.

> > - please allow sorting by column (type, size, time, name, etc) and resizing
> > the columns.
> 
> I'm not sure about resizing the columns. I'd like to design that to have a
> more "menu" feel, not a "table" feel.

That's... confusing, but I leave this to UX experts. Please ask for shorlander to play with the netmonitor (beyond screenshots).



> > - timings look incomplete with that background. I'm not sure what to suggest
> > in terms of UI, but that was my first impression. Can you add page timings
> > as well? Like onload, content loaded, etc.
> > 
> 
> Obviously. We need timing delimitation marks (see
> REQUESTS_WATERFALL_BACKGROUND_PATTERN) but I need to make sure I'm not
> slowing things down. Onload and DOMContentReady event lines certainly need
> to be shown as well, but it'll happen in a followup.

Sounds good.


> > - I'd like some consistency between the various sidebars we have. In bug
> > 808370 I'm adding a sidebar to the web console that reuses the sidebar
> > implementation from the Inspector. Here you reuse the debugger's sidebar
> > implementation. Do we have a bug about consistency across tools?
> > 
> 
> How am I reusing debugger's sidebar implementation? It's a tabbox with
> tabpanels, just like your work in 808370. If anything, It's completely
> different from the debugger :) Maybe you mean the pane collapse/expand
> mechanism? That should work with anything.

Ah, my bad. Yes, the sidebar toggle. That's something we should make more common cross different tools, and the debugger still needs to use the sidebar thing.


> > @@ +131,5 @@
> > > +      this.tabClient = aTabClient;
> > > +
> > > +      aClient.attachConsole(aTabGrip.consoleActor, LISTENERS, (aResponse, aWebConsoleClient) => {
> > > +        if (!aWebConsoleClient) {
> > > +          Cu.reportError("Couldn't attach to console: " + aResponse.error);
> > 
> > Ditto and other similar occurrences.
> > 
> 
> It's mirroring the debugger implementation, mostly just mechanical copy
> pasting. There's not specific reason I can think of for these errors to
> happen, but there's no harm in logging, no?

Yes and no. One of the things I learned early at Mozilla, from various reviews, is that it's not ideal to try to check for things that could go wrong, unless you have known behavior. This is as needless as |if (!Cu) { ... }|. You can never catch all the things that can go wrong, and trying to do that, you may end up hiding broken behavior. (not saying this check goes as far as that, obviously it doesn't)


> > How did you find using the network event actors? The API and the protocol
> > design.
> >
> 
> It was quite delicious :) Knowing when to ask for request updates was a bit
> tricky to understand, but obvious after that.

Great!

> > ::: browser/devtools/netmonitor/netmonitor-view.js
> > @@ +340,5 @@
> > > +          // The information in the packet is empty, it can be safely ignored.
> > > +          continue;
> > > +        }
> > > +
> > > +        switch (key) {
> > 
> > This looks quite hacky and possibly easy to break whenever packets change,
> > since you iterate over the properties available in each networkEventUpdate
> > packet. That's not ideal. Why not use packet.updateType as the basis for
> > this switch block? I'd like to see this fixed.
> > 
> 
> That's not what happens.
> 
> These properties are part of the object literal passed when calling
> updateRequest from the controller. See _onNetworkEventUpdate and all its
> friends.

Ah, I see now - you pass a new object with the data you want for updateRequest(). This confused me as the objects seemed similar to the packets.


> > ::: browser/devtools/shared/widgets/VariablesView.jsm
> > @@ +1865,5 @@
> > >     *             - { value: { type: "object", class: "Object" } }
> > >     *             - { get: { type: "object", class: "Function" },
> > >     *                 set: { type: "undefined" } }
> > > +   * @param boolean aRelaxed
> > > +   *        True if name duplicates should be allowed.
> > 
> > How does this work? this._store is a map and you add properties by name, so
> > you end up overriding existing stored properties. Am I missing something?
> > 
> 
> This works poorly, but we're stretching the "variables view" metaphor a bit
> with the network monitor, so I'm not worried about it.

Sounds like there's a potential for bugs...

Followup?
Assignee

Comment 18

6 years ago
(In reply to Mihai Sucan [:msucan] from comment #17)
> > 
> > How am I reusing debugger's sidebar implementation? It's a tabbox with
> > tabpanels, just like your work in 808370. If anything, It's completely
> > different from the debugger :) Maybe you mean the pane collapse/expand
> > mechanism? That should work with anything.
> 
> Ah, my bad. Yes, the sidebar toggle. That's something we should make more
> common cross different tools, and the debugger still needs to use the
> sidebar thing.
> 

That pretty much just means changing the xul (from <vbox> to <tabpanel>) and that's it. The expand/collapse mechanism is shared.
Assignee

Comment 19

6 years ago
Testing is fun!
Assignee

Comment 20

6 years ago
Posted patch v1.3 (obsolete) — Splinter Review
Some tests added. A couple more are needed.
Assignee

Comment 21

6 years ago
Posted patch v1.4 (obsolete) — Splinter Review
Addressed all comments and added more tests. I'm going one more time through all this before asking for review.
Attachment #732848 - Attachment is obsolete: true
Assignee

Comment 22

6 years ago
Posted patch v2Splinter Review
This should be it. Some stuff has also changed in netmonitor-view.js (like NV_editor and friends) and basically everything was Promisified. Test rely on EventEmitter for receiving network event notifications because it's more convenient.
Attachment #731122 - Attachment is obsolete: true
Attachment #733306 - Attachment is obsolete: true
Attachment #733540 - Flags: review?(dcamp)

Updated

6 years ago
Attachment #733540 - Flags: review?(dcamp) → review+
Assignee

Updated

6 years ago
Priority: -- → P1
Assignee

Comment 23

6 years ago
Posted patch v2.1Splinter Review
Fought off some WinXP oranges.
Attachment #733856 - Flags: review+
Assignee

Comment 25

6 years ago
https://hg.mozilla.org/mozilla-central/rev/3fda72d2c6c3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Assignee

Comment 26

6 years ago
Followup bugs have a [netmonitor] in their title.
Depends on: 859091
I took a quick look at the strings and I think it's better to ask before opening a new unnecessary bug.

# LOCALIZATION NOTE (networkMenu.size): This is the label displayed
# in the network menu specifying the size of a request (in kb).
networkMenu.size=%Skb
networkMenu.total=→ %Sms

1) Are you really displaying kilobits?

let size = (aValue / 1024).toFixed(CONTENT_SIZE_DECIMALS);

This sounds like kilobytes, so KB (it should be kB, but en-US uses KB, see bug 814936 for some details)

2) AFAIK you need a space between numbers and units of measure
Assignee

Comment 28

6 years ago
(In reply to Francesco Lodolo [:flod] from comment #27)
> 
> This sounds like kilobytes, so KB (it should be kB, but en-US uses KB, see
> bug 814936 for some details)
> 

Yes, you're right. Filed bug 859221.

> 2) AFAIK you need a space between numbers and units of measure

Will change.
Assignee

Comment 29

6 years ago
Need some documentation :)
Keywords: dev-doc-needed
Depends on: 859332

Comment 30

6 years ago
Netmon should not use the profiler toolbarbutton icon:
  icon: "chrome://browser/skin/devtools/tool-profiler.png",
Assignee

Comment 31

6 years ago
(In reply to Alfred Kayser from comment #30)
> Netmon should not use the profiler toolbarbutton icon:
>   icon: "chrome://browser/skin/devtools/tool-profiler.png",

Bug 859222.

Updated

6 years ago
Depends on: 859222
Please do not add new -moz prefixed gradients anymore.
Depends on: 867552
Assignee

Comment 33

6 years ago
(In reply to Masatoshi Kimura [:emk] from comment #32)
> Please do not add new -moz prefixed gradients anymore.

Already removed.

Comment 34

6 years ago
I'm sorry to rate all your good work, but I have a couple of suggestions to improve the usability of this wonderful tool:

- request URL should be selectable (for easy copying)
- a raw view of headers would be also very nice, and useful (also for easy copying of all the header data in one piece)

Should I file separate bugs for these features?
(In reply to Greg Karz from comment #34)
> I'm sorry to rate all your good work, but I have a couple of suggestions to
> improve the usability of this wonderful tool:
> 
> - request URL should be selectable (for easy copying)
> - a raw view of headers would be also very nice, and useful (also for easy
> copying of all the header data in one piece)
> 
> Should I file separate bugs for these features?

Yes please. We always try to track new features/requests/issue reports separately.
Assignee

Comment 36

6 years ago
(In reply to Greg Karz from comment #34)
> 

Thanks for the interest Greg!

> - request URL should be selectable (for easy copying)

I wholeheartedly agree. However, there is bug 861686 and bug 859052 already filed. You should CC yourself on those if you want to follow the development.

> - a raw view of headers would be also very nice, and useful (also for easy
> copying of all the header data in one piece)
> 

Yup, bug 859133 already filed :)

Comment 37

6 years ago
(In reply to Victor Porof [:vp] from comment #36)
> (In reply to Greg Karz from comment #34)
> > 
> 
> Thanks for the interest Greg!
> 
> > - request URL should be selectable (for easy copying)
> 
> I wholeheartedly agree. However, there is bug 861686 and bug 859052 already
> filed. You should CC yourself on those if you want to follow the development.
> 
> > - a raw view of headers would be also very nice, and useful (also for easy
> > copying of all the header data in one piece)
> > 
> 
> Yup, bug 859133 already filed :)

Ohh.. Thank you! Sorry that I didn't search for them before my comment, but shouldn't they be blocking this bug? Or somehow linked together for easy tracking of improvements on this feature?
Assignee

Comment 38

6 years ago
(In reply to Greg Karz from comment #37)
> 
> Ohh.. Thank you! Sorry that I didn't search for them before my comment, but
> shouldn't they be blocking this bug? Or somehow linked together for easy
> tracking of improvements on this feature?

There are currently 59 followup bugs for the network monitor (that number used to be larger), so adding all of them here as blocking would be a bit of a pain. Just doing a search on bugzilla for all the new/assigned/reopened bugs in the "Developer Tools: Netmonitor" component should be enough.

Comment 39

6 years ago
(In reply to Victor Porof [:vp] from comment #38)
> (In reply to Greg Karz from comment #37)
> > 
> > Ohh.. Thank you! Sorry that I didn't search for them before my comment, but
> > shouldn't they be blocking this bug? Or somehow linked together for easy
> > tracking of improvements on this feature?
> 
> There are currently 59 followup bugs for the network monitor (that number
> used to be larger), so adding all of them here as blocking would be a bit of
> a pain. Just doing a search on bugzilla for all the new/assigned/reopened
> bugs in the "Developer Tools: Netmonitor" component should be enough.

Ohh.. okay, I'll do that next time, thank you!
Assignee

Comment 40

6 years ago
Thank you lsblakk! \o/
Depends on: 923938

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.