1398522 - Devtools/Netmonitor - sort request/response headers in Headers tab

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[Franck (Wip)]

It could help the readability of Netmonitor's Headers tab content if request and response headers were sorted alphabetically (case insensitive).

Since order matters if a same header is set several times, it should be taken in account, at least after bug 1095273 is fixed. 

Developers caring about the real headers' order can still use "Raw headers".

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Yep, I like that idea.

Honza

Comment 2

[Abhinav Koppula]

Attachment: Bug 1398522 - Sort 'Request-Headers' and 'Response-Headers' in 'Headers' tab.

Review commit: https://reviewboard.mozilla.org/r/180678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180678/

Comment 3

[Abhinav Koppula]

Hi Honza,
I have taken a stab at this issue.
Please let me know if I have missed anything.

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8909078 [details]
Bug 1398522 - Sort 'Request-Headers' and 'Response-Headers' in 'Headers' tab.

https://reviewboard.mozilla.org/r/180678/#review185940

Thanks for working on this!

I am seeing test failure when running the test

Browser Chrome Test Summary
        Passed: 22
        Failed: 1
        Todo: 0
        Mode: e10s
*** End BrowserChrome Test Results ***
The following tests failed:
85 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_headers_sorted.js | Actual label value Request headers (471 B) not equal to expected label value Request headers (466 B) - Got Request headers (471 B), expected Request headers (466 B)
Stack trace:
    chrome://mochikit/content/browser-test.js:test_is:1011
    chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_headers_sorted.js:null:37
    chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_headers_sorted.js:null:36
    Tester_execTest@chrome://mochikit/content/browser-test.js:798:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:697:9
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
Buffered messages finished
SUITE-END | took 10s

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Just like I mentioned in bug 1398524, we should sort also
URL query arguments (in the Params side panel)
Needs a follow up

Honza

Comment 6

[Abhinav Koppula]

Hi Honza,
Interestingly on my local, all tests are passing.
TEST-INFO | checking window state
Browser Chrome Test Summary
    Passed: 23
    Failed: 0
    Todo: 0
    Mode: e10s
*** End BrowserChrome Test Results ***
Buffered messages finished
SUITE-END | took 9s

Can you please push to try once so that I can check if TRY shows the same results or not?
I don't have access to push to TRY.

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Sure, here is the try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=daf5775178467ae408b4ae96f85f7645086c0de7

If you are interested in getting access to the Try server read this instructions:
https://wiki.mozilla.org/Build:TryServer#Getting_access_to_the_Try_Server
You'll need Level 1 Commit Access and I'll vouch for you

Honza

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8909078 [details]
Bug 1398522 - Sort 'Request-Headers' and 'Response-Headers' in 'Headers' tab.

https://reviewboard.mozilla.org/r/180678/#review187054

OK, try is green, let's land this!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=daf5775178467ae408b4ae96f85f7645086c0de7

R+

Honza

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48e2fba9e04e
Sort 'Request-Headers' and 'Response-Headers' in 'Headers' tab. r=Honza

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Sorry, this had to be backed out for failing devtools' devtools/client/netmonitor/test/browser_net_headers_sorted.js:

https://hg.mozilla.org/integration/autoland/rev/7a40d9d95e3c59295b0d7adafbad27e777d1c20c

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=48e2fba9e04e3d209b92a02d1e7198ac879703e9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=132204964&repo=autoland

11:46:24     INFO -  Buffered messages finished
11:46:24    ERROR -  614 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_headers_sorted.js | Actual label value Request headers (463 B) not equal to expected label value Request headers (466 B) - Got Request headers (463 B), expected Request headers (466 B)
11:46:24     INFO -  Stack trace:
11:46:24     INFO -      chrome://mochikit/content/browser-test.js:test_is:1011
11:46:24     INFO -      chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_headers_sorted.js:null:37
11:46:24     INFO -      chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_headers_sorted.js:null:36
11:46:24     INFO -      Tester_execTest@chrome://mochikit/content/browser-test.js:798:9
11:46:24     INFO -      Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:697:9

Please fix the issue and submit an updated patch.

Comment 11

[Abhinav Koppula]

Comment on attachment 8909078 [details]
Bug 1398522 - Sort 'Request-Headers' and 'Response-Headers' in 'Headers' tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180678/diff/1-2/

Comment 12

[Abhinav Koppula]

Hi Sebastian, Honza,
Sorry for the inconvenience caused.
I have updated my mozreview-request and have changed the failing test.
Please review and let me know if it is fine.

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8909078 [details]
Bug 1398522 - Sort 'Request-Headers' and 'Response-Headers' in 'Headers' tab.

https://reviewboard.mozilla.org/r/180678/#review187408

The test passes for me on Win10 now, let's try to re-land.

Thanks for the patch update!
Honza

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10fe5727b04f
Sort 'Request-Headers' and 'Response-Headers' in 'Headers' tab. r=Honza

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/10fe5727b04f

Comment 16

[Franck (Wip)]

Tested on Nightly 58.0a1 20170922100051, works great.

It's that kind of little thing that sometime eases one's life.
Thanks guys :)