Tap timeline column to open the timing sidebar panel

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Netmonitor
P3
normal
VERIFIED FIXED
3 months ago
16 days ago

People

(Reporter: gasolin, Assigned: gabrielle.singhcadieux, Mentored)

Tracking

({dev-doc-needed, good-first-bug})

unspecified
Firefox 55
dev-doc-needed, good-first-bug
Points:
---

Firefox Tracking Flags

(firefox55 verified)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

3 months ago
When user tap the `js` icon in the `cause` column, he/she can see the `stack-trace `sidebar panel opened by default.

We can add the similar thing for timing column. 


in request-list-waterfall.js, hook onclick event in div with .requests-list-timings class.
Then define `onWaterfallClick` in request-list-content.js
(Assignee)

Comment 1

3 months ago
I would be interested in working on this! 
Would you be able to give me a little more information? I'm not sure how to replicate this event (i.e. I'm not sure what the stack-trace sidebar looks like because I'm not sure when `js` appears under `cause`).
Flags: needinfo?(gasolin)
(Reporter)

Comment 2

3 months ago
Hi gabrielle,

Thanks for interesting to contribute!
You can refer the Bug 1359682 which the task is very similar. If you are familiar with React, you can also join to help us decide if we should use onClick or onMouseDown event.

For the example case, the js icon(badge) is only shown when the request is a js file. You can spot that in most of web site.
Assignee: nobody → gabrielle.singhcadieux
Flags: needinfo?(gasolin)
(Reporter)

Comment 3

3 months ago
And welcome to go to slack #netmonitor channel to discuss anything related https://firefox-team.slack.com/messages/devtools/
(Assignee)

Comment 4

3 months ago
(In reply to Fred Lin [:gasolin] from comment #0)
> When user tap the `js` icon in the `cause` column, he/she can see the
> `stack-trace `sidebar panel opened by default.
> 
> We can add the similar thing for timing column. 
> 
> 
> in request-list-waterfall.js, hook onclick event in div with
> .requests-list-timings class.

I can't find request-list-waterfall.js - I'm wondering if this was meant to be request-list-item.js?
(Assignee)

Comment 5

3 months ago
(In reply to gabrielle.singhcadieux from comment #4)
> (In reply to Fred Lin [:gasolin] from comment #0)
> > When user tap the `js` icon in the `cause` column, he/she can see the
> > `stack-trace `sidebar panel opened by default.
> > 
> > We can add the similar thing for timing column. 
> > 
> > 
> > in request-list-waterfall.js, hook onclick event in div with
> > .requests-list-timings class.
> 
> I can't find request-list-waterfall.js - I'm wondering if this was meant to
> be request-list-item.js?

I just pulled the latest changes and now I see request-list-column-waterfall.js?
(Assignee)

Comment 6

3 months ago
Created attachment 8867229 [details] [diff] [review]
1359681.patch

Based on visual inspection, I believe that this patch produces the desired behaviour.
Attachment #8867229 - Flags: review?(gasolin)
(Reporter)

Comment 7

3 months ago
Comment on attachment 8867229 [details] [diff] [review]
1359681.patch

the patch looks good, thanks!


I will push the PR for auto test at next tuesday(cause I'm in travel now)

Though its optional, It will be nice if you can add a test case for this change.
(Assignee)

Comment 8

3 months ago
Created attachment 8867514 [details] [diff] [review]
1359681_v2.patch

I've now added a test for this based on devtools/client/netmonitor/test/browser_net_security-icon-click.js

However, the test always times out when I run it. I believe that this is based on
```
let wait = waitForDOM(document, "#response-panel");
...
yield wait;
```
I am not sure why - the test appears to be performing correctly based on visual inspection?

I'm also not sure whether I added the test to devtools/client/netmonitor/test/browser.ini in the correct place.
Attachment #8867229 - Attachment is obsolete: true
Attachment #8867229 - Flags: review?(gasolin)
Attachment #8867514 - Flags: review?(gasolin)
(Reporter)

Comment 9

2 months ago
Comment on attachment 8867514 [details] [diff] [review]
1359681_v2.patch

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

The main code works well! Just need to fix the test part. 
(It's fine to just remove the test in this case.)

::: devtools/client/netmonitor/test/browser_net_waterfall-click.js
@@ +12,5 @@
> +  let { document } = monitor.panelWin;
> +
> +  yield performRequestsAndWait();
> +
> +  let wait = waitForDOM(document, "#response-panel");

what we want to show is the "#timings-panel". You can use the browser toolbox to inspect the correct element
https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

@@ +13,5 @@
> +
> +  yield performRequestsAndWait();
> +
> +  let wait = waitForDOM(document, "#response-panel");
> +  let icon = document.querySelectorAll(".requests-list-timings")[0];

querySelectorAll(".requests-list-timings")[0] will select the column title but not the first row. You can use the browser toolbox to find the proper selector as well.

and `icon` is not a proper name for this selector, I suggest to use `timing`

@@ +20,5 @@
> +  EventUtils.synthesizeMouseAtCenter(icon, {}, monitor.panelWin);
> +
> +  yield wait;
> +
> +  let tabEl = document.querySelectorAll("#details-pane tab")[4];

You can check with "#timings-tab" directly

```
ok(document.querySelector("#timings-tab[aria-selected=true]"),
     "Timings tab is selected.");
```
Attachment #8867514 - Flags: review?(gasolin)
(Assignee)

Comment 10

2 months ago
(In reply to Fred Lin [:gasolin] from comment #9)
> Comment on attachment 8867514 [details] [diff] [review]
> 1359681_v2.patch
> 
> Review of attachment 8867514 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The main code works well! Just need to fix the test part. 
> (It's fine to just remove the test in this case.)
> 
> ::: devtools/client/netmonitor/test/browser_net_waterfall-click.js
> @@ +12,5 @@
> > +  let { document } = monitor.panelWin;
> > +
> > +  yield performRequestsAndWait();
> > +
> > +  let wait = waitForDOM(document, "#response-panel");
> 
> what we want to show is the "#timings-panel". You can use the browser
> toolbox to inspect the correct element
> https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox
> 
> @@ +13,5 @@
> > +
> > +  yield performRequestsAndWait();
> > +
> > +  let wait = waitForDOM(document, "#response-panel");
> > +  let icon = document.querySelectorAll(".requests-list-timings")[0];
> 
> querySelectorAll(".requests-list-timings")[0] will select the column title
> but not the first row. You can use the browser toolbox to find the proper
> selector as well.
> 
> and `icon` is not a proper name for this selector, I suggest to use `timing`
> 
> @@ +20,5 @@
> > +  EventUtils.synthesizeMouseAtCenter(icon, {}, monitor.panelWin);
> > +
> > +  yield wait;
> > +
> > +  let tabEl = document.querySelectorAll("#details-pane tab")[4];
> 
> You can check with "#timings-tab" directly
> 
> ```
> ok(document.querySelector("#timings-tab[aria-selected=true]"),
>      "Timings tab is selected.");
> ```

Thank you, I was really struggling with understanding the HTML structure of the developer tools! I did not know about the browser toolbox.
I will fix the test based on your comments.
(Assignee)

Comment 11

2 months ago
Created attachment 8868391 [details] [diff] [review]
1359681_v3.patch

The test now passes! 

However, 7 other netmonitor tests fail. Specifically
```
25743 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_filter-flags.js | There should be a specific amount of visible items in the requests menu. - Got 0, expected 1

25744 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_filter-flags.js | The item at index 1 has visibility=true - Got false, expected true

25745 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_filter-flags.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/netmonitor/test/head.js:399 - TypeError: target is undefined

25746 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_prefs-reload.js | Pref networkDetailsHeight should validate: 450 - Got (new Number(438)), expected (new Number(450))

25747 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_prefs-reload.js | The UI element affecting networkDetailsHeight should validate: 450 - Got (new Number(438)), expected (new Number(450))

25748 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_prefs-reload.js | Pref networkDetailsHeight should validate: 450 - Got (new Number(438)), expected (new Number(450))

25749 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_simple-request-details.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_simple-request-details.js:66 - TypeError: tabEl is undefined
```

Are these other bugs that need to be dealt with?
Attachment #8867514 - Attachment is obsolete: true
Attachment #8868391 - Flags: review?(gasolin)
(Reporter)

Comment 12

2 months ago
The simple rule of test:

* if those tests fail is caused by this patch, figure out why and fix them
* if those tests fail without any of your modification, it's ok (the patch cause the fail need to be backed out, or someone still need to fix them)
(Assignee)

Comment 13

2 months ago
(In reply to Fred Lin [:gasolin] from comment #12)
> The simple rule of test:
> 
> * if those tests fail is caused by this patch, figure out why and fix them
> * if those tests fail without any of your modification, it's ok (the patch
> cause the fail need to be backed out, or someone still need to fix them)

Those tests fail, regardless of this patch.
(Reporter)

Comment 14

2 months ago
Comment on attachment 8868391 [details] [diff] [review]
1359681_v3.patch

the patch can not be apply on today's repository now.
Please rebase the patch with new mozilla-central. It might solve test fail issues.
Attachment #8868391 - Flags: review?(gasolin)
(Reporter)

Updated

2 months ago
Flags: needinfo?(gabrielle.singhcadieux)
(Assignee)

Comment 15

2 months ago
Created attachment 8869273 [details] [diff] [review]
1359681_v3_rebased.patch

I've now rebased the patch. However, the following 4 tests still fail:

28710 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_prefs-reload.js | Pref networkDetailsHeight should validate: 450 - Got (new Number(438)), expected (new Number(450))

28711 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_prefs-reload.js | The UI element affecting networkDetailsHeight should validate: 450 - Got (new Number(438)), expected (new Number(450))

28712 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_prefs-reload.js | Pref networkDetailsHeight should validate: 450 - Got (new Number(438)), expected (new Number(450))

28713 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_simple-request-details.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_simple-request-details.js:66 - TypeError: tabEl is undefined
Attachment #8868391 - Attachment is obsolete: true
Flags: needinfo?(gabrielle.singhcadieux)
Attachment #8869273 - Flags: review?(gasolin)
(Reporter)

Comment 16

2 months ago
gabrielle, sorry for late response. I'll help check the test part within this week.
(Reporter)

Comment 17

2 months ago
browser_net_prefs-reload.js test looks fine in my desktop, and the issue around browser_net_simple-request-details.js looks like about the DOM load timing.


Since React does not use real mouse event, it has some delay between click & actual render the dom.
You can use `waitForDOM` for browser_net_simple-request-details as you have used in browser_net_waterfall_click, ex:

```
wait = waitForDOM(document, "#timings-panel");
EventUtils.sendMouseEvent({ type: "click" },
  document.querySelector(".network-details-panel-toggle"));
yield wait;
```

You may need to wrap each sendMouseEvent with waitForDOM to make test pass
Flags: needinfo?(gabrielle.singhcadieux)
(Reporter)

Updated

2 months ago
Attachment #8869273 - Flags: review?(gasolin)
(Assignee)

Comment 18

2 months ago
browser_net_simple-request-details fails at [1] because `tabEl` is not defined, meaning that the selector "#details-pane tab" is not matched. However, the only call to `sendMouseEvent` before this is at [2].
When I wrap the sendMouseEvent at [2] with `waitForDOM`, exactly as you suggested, the test times out.
I also tried wrapping the sendMouseEvent at [2] with `waitForDOM` which waits for "#details-pane", and again, the test times out.

Therefore, I believe that the issue is with the selector "#details-pane tab". I looked for the element with id "details-pane", and it appears to be a part of the performance tool rather than the netmonitor tool ([3] and [4]).
I verified this using the browser toolbox: when the developer toolbox is open, "#details-pane" is not found. It is also not found when the network tab is open. However, it is found when the performance tab is open.

What are your thoughts?

[1] https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/netmonitor/test/browser_net_simple-request-details.js#l66
[2] https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/netmonitor/test/browser_net_simple-request-details.js#l38
[3] https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/performance/performance-view.js#l93
[4] https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/performance/views/details.js#l50
Flags: needinfo?(gabrielle.singhcadieux) → needinfo?(gasolin)
(Reporter)

Comment 19

2 months ago
I think it still related to the timing when the panel DOM is actually ready. Instead of get general tabpanel element, you can rewrite the test and try to listen and check the actual target panel like #heads-panel, #timings-panel instead. 

You can refer browser_net_raw_headers to see how it check `headers-summary` after click the first row in requests-list
Flags: needinfo?(gasolin)
(Assignee)

Comment 20

2 months ago
I replaced [1] with
```
let tabEl = document.querySelector("#headers-tab");
let tabpanel = document.querySelector("#headers-panel");
```
which are both successfully matched.

However, the checks starting at [2] fail because the selectors aren't matched. I believe, based on searching using the browser toolbox, that these elements (eg. `"#headers-summary-url-value") don't exist.

However, I also can't find unique identifiers for those values. I believe that the following is the HTML for the header summary panel:
```
<div class="tabpanel-summary-container headers-summary">
  <div class="tabpanel-summary-label headers-summary-label">Request URL:</div>
  <input class="tabpanel-summary-value textbox-input devtools-monospace" readonly="" value="http://example.com/browser/devtools/client/netmonitor/test/sjs_simple-test-server.sjs">
</div>
<div class="tabpanel-summary-container headers-summary">
  <div class="tabpanel-summary-label headers-summary-label">Request method:</div>
  <input class="tabpanel-summary-value textbox-input devtools-monospace" readonly="" value="GET">
</div>
<div class="tabpanel-summary-container headers-summary">
  <div class="tabpanel-summary-label headers-summary-label">Remote address:</div>
  <input class="tabpanel-summary-value textbox-input devtools-monospace" readonly="" value="93.184.216.34:80">
</div>
<div class="tabpanel-summary-container headers-summary">
  <div class="tabpanel-summary-label headers-summary-label">Status code:</div>
  <div class="requests-list-status-icon" data-code="404"></div>
  <input class="tabpanel-summary-value textbox-input devtools-monospace status-text" readonly="" value="404 Not Found" size="13">
  <a class="learn-more-link" title="https://developer.mozilla.org/docs/Web/HTTP/Status/404?utm_source=mozilla&amp;utm_medium=devtools-netmonitor&amp;utm_campaign=default">[Learn More]</a>
  <button class="devtools-button">Edit and Resend</button>
  <button class="devtools-button">Raw headers</button>
</div>
<div class="tabpanel-summary-container headers-summary">
  <div class="tabpanel-summary-label headers-summary-label">Version:</div>
  <input class="tabpanel-summary-value textbox-input devtools-monospace" readonly="" value="HTTP/1.1">
</div>
```
If I understand [2] correctly, the test is checking that each of the summary values is correct. However, I don't see any identifier that distinguishes the summary values from one another.

[1] https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/netmonitor/test/browser_net_simple-request-details.js#l63
[2] https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/netmonitor/test/browser_net_simple-request-details.js#l69
Flags: needinfo?(gasolin)
(Reporter)

Comment 21

2 months ago
Ouch deeply sorry, its my bad to not find out at first time. 

The true reason is in browser.ini

```
 [browser_net_simple-request-details.js]
+[browser_net_waterfall-click.js]
 skip-if = true # Bug 1258809
```

The simple-request-details.js test was skipped with `skip-if` syntax, but the patch add a line between the script and the skip-if syntax...

Please add [browser_net_waterfall-click.js] after skip-if syntax, and it should be fine!


Please update the patch and we can try to land it :)
Flags: needinfo?(gasolin)
(Reporter)

Comment 22

2 months ago
By the way you are right in comment 20, the test target in simple-request-details are not exist anymore, and we skipped this test during the recent refactor.
(Assignee)

Comment 23

2 months ago
Created attachment 8874577 [details] [diff] [review]
1359681_v4.patch

Okay, I didn't realize that the if-condition in the ini file applied to the test file immediately above it!
Attachment #8869273 - Attachment is obsolete: true
Attachment #8874577 - Flags: review?(gasolin)
(Reporter)

Comment 24

2 months ago
Comment on attachment 8874577 [details] [diff] [review]
1359681_v4.patch

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

in browser.ini please put these tests name in alphabet order, then its ready to go

[browser_net_waterfall-click.js]
[browser_net_persistent_logs.js]
Attachment #8874577 - Flags: review?(gasolin) → review+
Comment hidden (mozreview-request)
(Reporter)

Comment 26

2 months ago
according to https://treeherder.mozilla.org/#/jobs?repo=try&revision=522acba5a377&selectedJob=104763568

2 lint error need to be fixed,

* components/request-list-content.js:309:2 | Expected indentation of 6 spaces but found 1 tab. 
* components/request-list-item.js:167:1 | Line 167 exceeds the maximum line length of 90

You can use following command to make sure all lint passed

`./mach eslint devtools/client/netmonitor/`
(Assignee)

Comment 27

2 months ago
(In reply to Fred Lin [:gasolin] from comment #26)
> according to
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=522acba5a377&selectedJob=104763568
> 
> 2 lint error need to be fixed,
> 
> * components/request-list-content.js:309:2 | Expected indentation of 6
> spaces but found 1 tab. 
> * components/request-list-item.js:167:1 | Line 167 exceeds the maximum line
> length of 90
> 
> You can use following command to make sure all lint passed
> 
> `./mach eslint devtools/client/netmonitor/`

Neither of those lines was modified by this patch - should I still change them and add that to this patch?
Flags: needinfo?(gasolin)
(Reporter)

Comment 28

2 months ago
Since we changed the request-list-content.js & request-list-item.js content, we do need make sure the lint for these files are valid. Or the sheriff will back out the patch
Flags: needinfo?(gasolin)
(Reporter)

Comment 29

2 months ago
If the error is caused by others, try to rebase the repo later and the lint issue might be gone
(Assignee)

Comment 30

2 months ago
Created attachment 8875327 [details] [diff] [review]
1359681_v5.patch

I'm sorry, I was misunderstanding the line numbers in the patch format - those lint errors were being produced by this code! They should be gone now.
Attachment #8874577 - Attachment is obsolete: true
Attachment #8875327 - Flags: review?(gasolin)
(Reporter)

Updated

2 months ago
Attachment #8874674 - Attachment is obsolete: true
(Reporter)

Updated

2 months ago
Attachment #8875327 - Flags: review?(gasolin) → review+
(Reporter)

Comment 31

2 months ago
thanks!
Keywords: checkin-needed, dev-doc-needed

Comment 32

2 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24ef2aa69cf3
Tap timeline column to open the timing sidebar panel. r=gasolin
Keywords: checkin-needed

Comment 33

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/24ef2aa69cf3
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 34

20 days ago
I can verify that in latest beta 55.0b7 (64-bit), clicking timeline column opens "Timing" tab in sidebar panel.

Build ID 	20170706155135
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
QA Whiteboard: [testday-20170707]
I also can confirm the bug is verified fixed on 55.0b7 build1 (20170706085221) using Windows 10 x64.  
Thank you, Sayed for verifying this issue.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.