1953387 - Duration for incomplete requests should not show "0 min"

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[Julian Descottes [:jdescottes]]

Spotted while looking at Bug 1920146.
The Duration column shows "0 min" for incomplete requests. This seems to mess up with the sorting of the column and it's also incorrect.

It should probably say "pending" or something similar.
Can be a good next bug.

Comment 1

[Joshua O'Brien]

I'd be happy to work on this

Comment 2

[Julian Descottes [:jdescottes]]

(In reply to Joshua O'Brien from comment #1)

I'd be happy to work on this

Thanks for your interest and sorry about the delay, I was off since last week.

So for this particular issue, the string is generated here https://searchfox.org/mozilla-central/rev/be3db2aef8d3916c891527794a2d5e2f2dc9fab1/devtools/client/netmonitor/src/utils/format-utils.js#79-89

function getFormattedTime(ms) {
  if (ms < MAX_MILLISECOND) {
    return L10N.getFormatStr("networkMenu.millisecond", ms | 0);
  }
  if (ms < MAX_SECOND) {
    const sec = ms / MAX_MILLISECOND;
    return L10N.getFormatStr("networkMenu.second", getTimeWithDecimals(sec));
  }
  const min = ms / MAX_SECOND;
  return L10N.getFormatStr("networkMenu.minute", getTimeWithDecimals(min));
}

But for pending requests, ms is set to undefined. So we end up in the last branch (for minutes), and compute the incorrect "0 min" string.
For reference, the localized strings used here are at https://searchfox.org/mozilla-central/rev/be3db2aef8d3916c891527794a2d5e2f2dc9fab1/devtools/client/locales/en-US/netmonitor.properties#354-356

# LOCALIZATION NOTE (networkMenu.minute): This is the label displayed
# in the network menu specifying timing interval divisions (in minutes).
networkMenu.minute=%S min

The idea would be to add another branch in this method to handle the case where ms is undefined and create a new localized string (for instance set to "pending").

I see you already fixed a few bugs, so I'll let you upload a patch once you have something we can look at, and the bug will be automatically assigned to you!

(feel free to ask questions if needed of course :) )

Comment 3

[joel.mozillaosi]

Attachment: Bug 1953387 - [devtools] Display empty string for undefined/NaN in netmonitor time columns. r?jdescottes

Comment 4

[joel.mozillaosi]

Hey Julian and Joshua, I'm also a new contributor and have a patch that addresses this issue as outlined in comment 2. I don't mean to step on your toes Joshua but noticed that there hadn't been any activity for over a week and was having a hard time sourcing similarly-accessible bugs.

Comment 5

[Joshua O'Brien]

(In reply to joel.mozillaosi from comment #4)

Hey Julian and Joshua, I'm also a new contributor and have a patch that addresses this issue as outlined in comment 2. I don't mean to step on your toes Joshua but noticed that there hadn't been any activity for over a week and was having a hard time sourcing similarly-accessible bugs.

No problem at all! I haven't had a chance to look at this yet, so all good :)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b61b6a86a3e
[devtools] Display empty string for undefined /NaN in netmonitor time columns. r=jdescottes,devtools-reviewers

Comment 7

[amarc]

Backed out for causing dt failures @ browser_net_block-csp.js

• Backout link
• Push with failures
• Failure Log

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff50bc28b0d7
[devtools] Display empty string for undefined/NaN in netmonitor time columns. r=jdescottes,devtools-reviewers

Comment 9

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/ff50bc28b0d7