1398524 - Devtools/Netmonitor - sort request/response cookies in Cookies tab

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[Franck (Wip)]

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

Comment 1

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

I like that idea, thanks for the report!

Honza

Comment 2

[sajattack]

Is this the file we'd be looking at modifying? http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/cookies-panel.js

Comment 3

[sajattack]

Or this? http://searchfox.org/mozilla-central/source/browser/components/preferences/cookies.js

Comment 4

[Abhinav Koppula]

Attachment: Bug 1398524 - Sort Request-Cookies,Response-Cookies in 'Cookies' tab and Query-String,Form-Params,JSON in Params tab.

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

Comment 5

[Abhinav Koppula]

Hi Honza,
I have taken a stab at this issue and have submitted a mozreview-request.
Please let me know if I have missed anything.

Comment 6

[Abhinav Koppula]

Comment on attachment 8909045 [details]
Bug 1398524 - Sort Request-Cookies,Response-Cookies in 'Cookies' tab and Query-String,Form-Params,JSON in Params tab.

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

Comment 7

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

Comment on attachment 8909045 [details]
Bug 1398524 - Sort Request-Cookies,Response-Cookies in 'Cookies' tab and Query-String,Form-Params,JSON in Params tab.

https://reviewboard.mozilla.org/r/180640/#review185934

Thanks for the patch!

It would be great if Params and Headers are also sorted.
File a follow up?

R+ assuming try is green

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=09950e79edba495499cbe482a990fc236d8d3fc2

Honza

::: devtools/client/netmonitor/src/components/cookies-panel.js:77
(Diff revision 2)
> +/**
> + * Sorts object by keys in alphabetical order
> + * If object has nested children, it sorts the child-elements also by keys
> + * @param {object} which should be sorted by keys in alphabetical order
> + */
> +function sortObjectKeys(object) {

This method might be useful for sorting other net side panels (params and headers). In such case we should put it into utils/sort-utils.js (the files doesn't exist yet).

::: devtools/client/netmonitor/src/components/cookies-panel.js:83
(Diff revision 2)
> +  return Object.keys(object).sort(function (left, right) {
> +    return left.toLowerCase().localeCompare(right.toLowerCase());
> +  }).reduce((acc, key) => {
> +    if (typeof object[key] === "object") {
> +      acc[key] = sortObjectKeys(object[key]);
> +    } else {

Just curious, why it does the recursion here?

Comment 8

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

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=32dd89b6ae7f

@Abhinav: please see my question in comment #7

Honza

Comment 9

[Abhinav Koppula]

Attachment: CookieNestedHeaders.png

Hi Honza,
1. I am seeing that both bug 1398522 and this issue are linked as far as refactoring is concerned. Can we first land bug 1398522 and then as part of this bug, I will refactor the sort method to a utils file? Also, I was thinking of sorting the Params tab as part of this bug only. Your thoughts on the above things? 

2. I was using recursion after I saw that we sometimes render nested objects in Cookies Panel as the attached screenshot shows for youtube.com. So I felt that all keys(child as well as parent) should be sorted.

Comment 10

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

(In reply to Abhinav Koppula from comment #9)
> Created attachment 8910392 [details]
> CookieNestedHeaders.png
> 
> Hi Honza,
> 1. I am seeing that both bug 1398522 and this issue are linked as far as
> refactoring is concerned. Can we first land bug 1398522
Yes

> and then as part of
> this bug, I will refactor the sort method to a utils file?
Yes

> Also, I was
> thinking of sorting the Params tab as part of this bug only. Your thoughts
> on the above things? 
Sounds good to me.

> 2. I was using recursion after I saw that we sometimes render nested objects
> in Cookies Panel as the attached screenshot shows for youtube.com. So I felt
> that all keys(child as well as parent) should be sorted.
I see

Thanks!
Honza

Comment 11

[Abhinav Koppula]

Comment on attachment 8909045 [details]
Bug 1398524 - Sort Request-Cookies,Response-Cookies in 'Cookies' tab and Query-String,Form-Params,JSON in Params tab.

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

Comment 12

[Abhinav Koppula]

Comment on attachment 8909045 [details]
Bug 1398524 - Sort Request-Cookies,Response-Cookies in 'Cookies' tab and Query-String,Form-Params,JSON in Params tab.

https://reviewboard.mozilla.org/r/180640/#review185934

Hi Honza,
Thanks for the review.
I have moved the sorting to a utils file and have sorted Headers/Cookies/Params panels using the utils.

Comment 13

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

Thanks!

Here is a try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=30fb3c9ce23c

We can land if it's green

Honza

Comment 14

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

I am seeing some failures
devtools/client/netmonitor/test/browser_net_post-data-01.js | The first query param name was incorrect.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30fb3c9ce23c&selectedJob=133090872

Honza

Comment 15

[Abhinav Koppula]

Comment on attachment 8909045 [details]
Bug 1398524 - Sort Request-Cookies,Response-Cookies in 'Cookies' tab and Query-String,Form-Params,JSON in Params tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180640/diff/3-4/

Comment 16

[Abhinav Koppula]

Hi Honza,
I have updated the review-request and have fixed the following tests - browser_net_post-data-01.js, browser_net_post-data-02.js and browser_net_post-data-03.js which were failing due to my changes.
browser_net_prefs-reload.js seems to fail with and without my changes on my local.
Can you push it to TRY once?

Comment 17

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

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d4ae8e89734c73b6b6de628449456aa0b1b0b4c
Honza

Comment 18

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

Comment on attachment 8909045 [details]
Bug 1398524 - Sort Request-Cookies,Response-Cookies in 'Cookies' tab and Query-String,Form-Params,JSON in Params tab.

https://reviewboard.mozilla.org/r/180640/#review190896

Try looks good, let's try to land.

Great job Abhinav!

Honza

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c6aaee62972
Sort Request-Cookies,Response-Cookies in 'Cookies' tab and Query-String,Form-Params,JSON in Params tab. r=Honza

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/7c6aaee62972