Closed
Bug 1431912
Opened 7 years ago
Closed 7 years ago
Response time and Latency are always shown to be 0min when waterfall is disabled
Categories
(DevTools :: Netmonitor, defect, P1)
DevTools
Netmonitor
Tracking
(firefox-esr52 unaffected, firefox57 unaffected, firefox58 unaffected, firefox59+ fixed, firefox60 fixed)
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox57 | --- | unaffected |
firefox58 | --- | unaffected |
firefox59 | + | fixed |
firefox60 | --- | fixed |
People
(Reporter: ntim, Assigned: Honza)
References
Details
(Keywords: regression)
Attachments
(2 files)
(deleted),
text/x-review-board-request
|
ochameau
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-review-board-request
|
ochameau
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
STR:
- Disable "Timeline" column from netmonitor context menu
- Enable "Response Time" and "Latency" columns
- Reload the page
Latency and Response Time are always 0 minutes. The actual values are only shown when clicking on the network request.
Probably a regression of lazy loading timings: bug 1425273.
Reporter | ||
Updated•7 years ago
|
status-firefox57:
--- → unaffected
status-firefox58:
--- → unaffected
status-firefox59:
--- → affected
tracking-firefox59:
--- → ?
Keywords: regression
We still can take a patch for this during beta 59. Jan, do you know who might work on it?
Flags: needinfo?(odvarko)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(odvarko)
Priority: -- → P2
Assignee | ||
Comment 2•7 years ago
|
||
Thanks for the report Tim!
> Latency and Response Time are always 0 minutes. The actual values are only
> shown when clicking on the network request.
On my machine:
The actual values are fetched only if the request is clicked
and the "Timings" panel selected.
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #1)
> We still can take a patch for this during beta 59. Jan, do you know who
> might work on it?
I'll try to analyze this ASAP.
Honza
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Priority: P2 → P1
Assignee | ||
Updated•7 years ago
|
Has STR: --- → yes
Reporter | ||
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8944663 [details]
Bug 1431912 - Properly populate timing columns;
https://reviewboard.mozilla.org/r/214816/#review220462
::: devtools/client/netmonitor/src/components/RequestListColumnResponseTime.js:17
(Diff revision 1)
> + * This component represents a column displaying selected
> + * timing value. There are following possible values this
> + * column can render:
> + * - Start Time
> + * - End Time
> + * - Response Time
> + * - Duration
> + * - Latency
> + */
This isn't true, Start Time, End Time, Response Time, Duration and Latency all have a different component.
(I'd totally be fine with unifying them in one component though)
status-firefox60:
--- → ?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #4)
> This isn't true, Start Time, End Time, Response Time, Duration and Latency
> all have a different component.
Ah, yes, good catch.
> (I'd totally be fine with unifying them in one component though)
Exactly, done.
Try push looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9baa8d3ed35c2ca11506f14e22b0942e52bb4e62
Honza
Reporter | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8944663 [details]
Bug 1431912 - Properly populate timing columns;
https://reviewboard.mozilla.org/r/214816/#review220790
Thanks for the fix, looks good.
::: devtools/client/netmonitor/src/components/RequestListColumnTime.js:14
(Diff revision 3)
> +const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
> +const { getFormattedTime } = require("../utils/format-utils");
> +const { fetchNetworkUpdatePacket } = require("../utils/request-utils");
> +const { getResponseTime } = require("../utils/request-utils");
> +const { getStartTime } = require("../utils/request-utils");
> +const { getEndTime } = require("../utils/request-utils");
You could merge all these lines into:
const { fetchNetworkUpdatePacket, getResponseTime, getStartTime, getEndTime } = require("../utils/request-utils");
::: devtools/client/netmonitor/src/components/RequestListItem.js:34
(Diff revision 3)
> - RequestListColumnStartTime,
> RequestListColumnStatus,
> RequestListColumnTransferredSize,
> RequestListColumnType,
> RequestListColumnWaterfall
> */
Shouldn't you mention "RequestListColumnTime" in this list?
Attachment #8944663 -
Flags: review?(poirot.alex) → review+
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8945019 [details]
Bug 1431912 - Test for timing columns;
https://reviewboard.mozilla.org/r/215214/#review220800
::: devtools/client/netmonitor/test/browser_net_columns_time.js:44
(Diff revision 1)
> +
> + let promises = [];
> + types.forEach(type => {
> + promises.push(waitUntil(() => {
> + let node = item.querySelector(".requests-list-" + type + "-time");
> + let value = parseInt(node.textContent, 10);
This is a bit surprising but it seems to work.
node.textContent will be the formated time here, so something like 100ms or 1.3s.
parseInt seems to ignore characters,
parseInt("100ms") returns 100...
::: devtools/client/netmonitor/test/browser_net_columns_time.js:49
(Diff revision 1)
> + let value = parseInt(node.textContent, 10);
> + return value > 0;
> + }));
> + });
> +
> + await Promise.all(promises);
I think you do not need a promise list here.
You could do:
for (let type of types) {
await waitUntil(() => {
...
});
}
Attachment #8945019 -
Flags: review?(poirot.alex) → review+
Reporter | ||
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8945019 [details]
Bug 1431912 - Test for timing columns;
https://reviewboard.mozilla.org/r/215214/#review220804
::: devtools/client/netmonitor/test/head.js:709
(Diff revision 1)
> "Headers for columns number " + i + " are aligned."
> );
> }
> }
>
> +function* hideColumn(monitor, column) {
It would be nice if you can change hideColumn and showColumn to async functions.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #9 and #10)
All fixed
(In reply to Tim Nguyen :ntim from comment #11)
> It would be nice if you can change hideColumn and showColumn to async
> functions.
I reported bug 1432865 for this.
Honza
Comment 17•7 years ago
|
||
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0b0e37f74f15
Properly populate timing columns; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/c1c7615bd593
Test for timing columns; r=ochameau
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0b0e37f74f15
https://hg.mozilla.org/mozilla-central/rev/c1c7615bd593
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Comment 19•7 years ago
|
||
Please request Beta approval on this when you get a chance.
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(odvarko)
Assignee | ||
Comment 20•7 years ago
|
||
Comment on attachment 8944663 [details]
Bug 1431912 - Properly populate timing columns;
Approval Request Comment
[Feature/Bug causing the regression]: bug 1425273
[User impact if declined]: Some columns in the Network panel (DevTools) are not populated with data.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: both patches in this bug
[Is the change risky?]: low risk, only developers
[Why is the change risky/not risky?]: only developers
[String changes made/needed]: n/a
Flags: needinfo?(odvarko)
Attachment #8944663 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 21•7 years ago
|
||
Comment on attachment 8945019 [details]
Bug 1431912 - Test for timing columns;
Approval Request Comment
[Feature/Bug causing the regression]: bug 1425273
[User impact if declined]: Some columns in the Network panel (DevTools) are not populated with data.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: both patches in this bug
[Is the change risky?]: low risk, only developers
[Why is the change risky/not risky?]: only developers
[String changes made/needed]: n/a
Attachment #8945019 -
Flags: approval-mozilla-beta?
Comment on attachment 8944663 [details]
Bug 1431912 - Properly populate timing columns;
Recent regression (in 59), has test coverage, let's uplift this for beta 6.
Attachment #8944663 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8945019 [details]
Bug 1431912 - Test for timing columns;
Thanks for the extra tests.
Attachment #8945019 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 24•7 years ago
|
||
bugherder uplift |
Updated•7 years ago
|
Flags: in-testsuite+
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•