1356869 - Add cookies and set cookies columns

Status: VERIFIED FIXED

(DevTools :: Netmonitor, enhancement)

Description

[Tim Nguyen :ntim]

Cookies: number of cookies
Set Cookies: number of set cookies

(Like on chrome)

Comment 1

[Vangelis Katsikaros]

Attachment: Bug 1356869 - Add cookies and set cookies columns.

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

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

https://reviewboard.mozilla.org/r/133264/#review136118

Hey vkats, 

could you rebase your patch and ask review again? There are many changes especially request list refactoring (changed to div table) but I still see your patch is based on previous version.

Comment 3

[Vangelis Katsikaros]

Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

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

Comment 4

[Vangelis Katsikaros]

Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

https://reviewboard.mozilla.org/r/133264/#review136118

Ricky, I rebased the branch.

Comment 5

[Vangelis Katsikaros]

Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

https://reviewboard.mozilla.org/r/133264/#review136668

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:441
(Diff revision 2)
>  }
>  
> +/* Cookies column */
> +
> +.requests-list-cookies {
> +  width: 6%;

I chose this value arbitrarily. Is there a process for picking a number for this?

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

https://reviewboard.mozilla.org/r/133264/#review136672

Looks fine to me, but I'd like to get an extra pair of eyes on this one.

::: devtools/client/netmonitor/src/components/request-list-column-cookies.js:23
(Diff revision 2)
> +    let { requestCookies: thisRequestCookies = { cookies: [] } } = this.props.item;
> +    let { requestCookies: nextRequestCookies = { cookies: [] } } = nextProps.item;
> +    thisRequestCookies = thisRequestCookies.cookies || thisRequestCookies;
> +    nextRequestCookies = nextRequestCookies.cookies || nextRequestCookies;
> +    return thisRequestCookies !== nextRequestCookies;
> +  },
> +
> +  render() {
> +    let { requestCookies = { cookies: [] } } = this.props.item;
> +    requestCookies = requestCookies.cookies || requestCookies;

Same comments about destructuring.

::: devtools/client/netmonitor/src/components/request-list-column-set-cookies.js:23
(Diff revision 2)
> +    let { responseCookies: thisResponseCookies = { cookies: [] } } = this.props.item;
> +    let { responseCookies: nextResponseCookies = { cookies: [] } } = nextProps.item;
> +    thisResponseCookies = thisResponseCookies.cookies || thisResponseCookies;
> +    nextResponseCookies = nextResponseCookies.cookies || nextResponseCookies;

This destructuring looks monstruous.

Can we do something like this ?

let { responseCookies } = this.props.item;
let nextResponseCookies = nextProps.item.responseCookies;

responseCookies = responseCookies.cookies || responseCookies || [];
nextResponseCookies = nextResponseCookies.cookies || nextResponseCookies || [];

::: devtools/client/netmonitor/src/components/request-list-column-set-cookies.js:31
(Diff revision 2)
> +    let { responseCookies = { cookies: [] } } = this.props.item;
> +    responseCookies = responseCookies.cookies || responseCookies;

Same here.

Comment 7

[Vangelis Katsikaros]

Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

https://reviewboard.mozilla.org/r/133264/#review136672

> This destructuring looks monstruous.
> 
> Can we do something like this ?
> 
> let { responseCookies } = this.props.item;
> let nextResponseCookies = nextProps.item.responseCookies;
> 
> responseCookies = responseCookies.cookies || responseCookies || [];
> nextResponseCookies = nextResponseCookies.cookies || nextResponseCookies || [];

Yep, it looks very ugly :( but it seems it is a necessary workaround https://bugzilla.mozilla.org/show_bug.cgi?id=1354507#c17

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

https://reviewboard.mozilla.org/r/133264/#review136684

Good job! It's very close. Please address opened issues and r? again.

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:441
(Diff revision 2)
>  }
>  
> +/* Cookies column */
> +
> +.requests-list-cookies {
> +  width: 6%;

I'd say this `width` value should be as long as user can see the entire header title.

For cookie, I think 6% is fine.

::: devtools/client/netmonitor/src/components/request-list-column-cookies.js:23
(Diff revision 2)
> +  propTypes: {
> +    item: PropTypes.object.isRequired,
> +  },
> +
> +  shouldComponentUpdate(nextProps) {
> +    let { requestCookies: thisRequestCookies = { cookies: [] } } = this.props.item;

nit: let's use currentRequestCookies or currRequestCookies instead of this.

::: devtools/client/netmonitor/src/components/request-list-column-set-cookies.js:23
(Diff revision 2)
> +  propTypes: {
> +    item: PropTypes.object.isRequired,
> +  },
> +
> +  shouldComponentUpdate(nextProps) {
> +    let { responseCookies: thisResponseCookies = { cookies: [] } } = this.props.item;

nit: let's use currentRequestCookies or currRequestCookies instead of this.

::: devtools/client/netmonitor/src/constants.js:142
(Diff revision 2)
>    {
> +    name: "cookies",
> +    canFilter: false,
> +  },
> +  {
> +    name: "setCookies",

I saw `SetCookie` header column doesn't apply `.requests-list-set-cookies` properly.

request-list-header will assume className: `requests-list-column requests-list-${boxName}`, for class name by default if there is no boxname property.

Please add `boxName: "set-cookies"` here to fix the issue.

Comment 9

[Vangelis Katsikaros]

Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

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

Comment 10

[Vangelis Katsikaros]

Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

https://reviewboard.mozilla.org/r/133264/#review136684

> I saw `SetCookie` header column doesn't apply `.requests-list-set-cookies` properly.
> 
> request-list-header will assume className: `requests-list-column requests-list-${boxName}`, for class name by default if there is no boxname property.
> 
> Please add `boxName: "set-cookies"` here to fix the issue.

`I saw SetCookie header column doesn't apply .requests-list-set-cookies properly.`

Ricky, just wondering how did you spot this one? Simply from reading the JS file, or did (or would) it manifest somehow?

Comment 11

[Tim Nguyen :ntim]

(In reply to Vangelis Katsikaros from comment #7)
> Yep, it looks very ugly :( but it seems it is a necessary workaround
> https://bugzilla.mozilla.org/show_bug.cgi?id=1354507#c17

Still, I'm thinking:

> > let { responseCookies } = this.props.item;
> > let nextResponseCookies = nextProps.item.responseCookies;
> > 
> > responseCookies = responseCookies.cookies || responseCookies || [];
> > nextResponseCookies = nextResponseCookies.cookies || nextResponseCookies || [];

should work fine too, but feel free to correct me if not.

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

https://reviewboard.mozilla.org/r/133264/#review136692

::: commit-message-73752:1
(Diff revision 3)
> +Bug 1356869 - Add cookies and set cookies columns. r?ntim

r=ntim, rickychien

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

https://reviewboard.mozilla.org/r/133264/#review136694

Comment 14

[Ricky Chien [:rickychien] (inactive)]

(In reply to Vangelis Katsikaros from comment #10)
> Ricky, just wondering how did you spot this one? Simply from reading the JS
> file, or did (or would) it manifest somehow?

Yep! After applying your patch, I noticed the column layout looks slightly weird. That's because you applied .requests-list-setCookies to header column.

Comment 15

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

https://reviewboard.mozilla.org/r/133264/#review136710

LGTM. I'm so happy to see these new columns have been added recently. Thanks for your work!

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:447
(Diff revision 3)
> +}
> +
> +/* Set Cookies column */
> +
> +.requests-list-set-cookies {
> +  width: 6%;

nit: 8%

Let's set 8% to see the entire "Set cookies" title. But feel free to tweak this width if you feel wider is better.

Comment 16

[Ricky Chien [:rickychien] (inactive)]

Don't forget to revise your commit message with appending r?ntim,rickychien. Mozreview support multiple reviewers, so every mozreview push will set r? to reviewers automatically.

Thanks!

Comment 17

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

https://reviewboard.mozilla.org/r/133264/#review136720

::: devtools/client/preferences/devtools.js:153
(Diff revision 3)
>  
>  // The default Network Monitor UI settings
>  pref("devtools.netmonitor.panes-network-details-width", 550);
>  pref("devtools.netmonitor.panes-network-details-height", 450);
>  pref("devtools.netmonitor.filters", "[\"all\"]");
> -pref("devtools.netmonitor.hiddenColumns", "[\"remoteip\",\"protocol\"]");
> +pref("devtools.netmonitor.hiddenColumns", "[\"remoteip\",\"protocol\",\"cookies\",\"setCookies\"]");

btw, please update this new pref in devtools/client/netmonitor/index.js as well.

Pref in index.js is using when running in launchpad. We should sync these setting and make sure developers can see the same result in different environments.

Comment 18

[Vangelis Katsikaros]

(In reply to Tim Nguyen :ntim from comment #11)
> (In reply to Vangelis Katsikaros from comment #7)
> > Yep, it looks very ugly :( but it seems it is a necessary workaround
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1354507#c17
> 
> Still, I'm thinking:
> 
> > > let { responseCookies } = this.props.item;
> > > let nextResponseCookies = nextProps.item.responseCookies;
> > > 
> > > responseCookies = responseCookies.cookies || responseCookies || [];
> > > nextResponseCookies = nextResponseCookies.cookies || nextResponseCookies || [];
> 
> should work fine too, but feel free to correct me if not.

I have the impression this doesn't work because responseCookies from `let { responseCookies } = this.props.item;` might be undefined. So `responseCookies = responseCookies.cookies || responseCookies || [];` will complain on the `responseCookies.cookies` part.

I tried it with this patch:
```
--- a/devtools/client/netmonitor/src/components/request-list-column-set-cookies.js
+++ b/devtools/client/netmonitor/src/components/request-list-column-set-cookies.js
@@ -20,16 +20,16 @@ const RequestListColumnSetCookies = crea
   },
 
   shouldComponentUpdate(nextProps) {
-    let { responseCookies: currResponseCookies = { cookies: [] } } = this.props.item;
-    let { responseCookies: nextResponseCookies = { cookies: [] } } = nextProps.item;
-    currResponseCookies = currResponseCookies.cookies || currResponseCookies;
-    nextResponseCookies = nextResponseCookies.cookies || nextResponseCookies;
-    return currResponseCookies !== nextResponseCookies;
+    let { responseCookies } = this.props.item;
+    let nextResponseCookies = nextProps.item.responseCookies;
+    responseCookies = responseCookies.cookies || responseCookies || [];
+    nextResponseCookies = nextResponseCookies.cookies || nextResponseCookies || [];
+    return responseCookies !== nextResponseCookies;
   },
 
   render() {
-    let { responseCookies = { cookies: [] } } = this.props.item;
-    responseCookies = responseCookies.cookies || responseCookies;
+    let { responseCookies } = this.props.item;
+    responseCookies = responseCookies.cookies || responseCookies || [];
     let responseCookiesLength = responseCookies.length > 0 ? responseCookies.length : "";
```

Comment 19

[Tim Nguyen :ntim]

Fair enough, feel free to drop the issues then.

Comment 20

[Vangelis Katsikaros]

Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

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

Comment 21

[Tim Nguyen :ntim]

Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

https://reviewboard.mozilla.org/r/133264/#review137192

::: devtools/client/netmonitor/index.js:26
(Diff revision 4)
>  
>  EventEmitter.decorate(window);
>  
>  pref("devtools.netmonitor.enabled", true);
>  pref("devtools.netmonitor.filters", "[\"all\"]");
> -pref("devtools.netmonitor.hiddenColumns", "[]");
> +pref("devtools.netmonitor.hiddenColumns", "[\"cookies\",\"protocol\",\"remoteip\",\"setCookies\"]");

This long line makes ESLint complain (see the try push).

This should be fine:

pref("devtools.netmonitor.hiddenColumns",
     "[\"cookies\",\"protocol\",\"remoteip\",\"setCookies\"]");

Comment 22

[Vangelis Katsikaros]

Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e5ffcd2bba0
Add cookies and set cookies columns. r=ntim,rickychien

Comment 24

[Carsten Book [:Tomcat]]

backed out in https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7706b9edd277518759814bdec10598db5abc09fa

seems one of this 2 bugs caused https://treeherder.mozilla.org/logviewer.html#?job_id=94838707&repo=autoland

Comment 25

[Tim Nguyen :ntim]

The other bug is probably guilty.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3651cb71e99f
Add cookies and set cookies columns. r=ntim,rickychien

Comment 27

[Vangelis Katsikaros]

(In reply to Tim Nguyen :ntim from comment #25)
> The other bug is probably guilty.

Thanks Tim, it looks like it

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3651cb71e99f

Comment 29

[Francesco Lodolo [:flod]]

"set cookies": is "set" a verb ("create cookies") or an adjective ("created cookies")? Without an explanation in the comment it's basically a wild guess.

Comment 30

[Tim Nguyen :ntim]

(In reply to Francesco Lodolo [:flod] from comment #29)
> "set cookies": is "set" a verb ("create cookies") or an adjective ("created
> cookies")? Without an explanation in the comment it's basically a wild guess.

It's an adjective in this case.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dee7a59bc560
Update Set cookies header string comment. r=flod

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 33

[Md. Majedul islam]

I have reproduced this bug with Nightly 55.0a1 (2017-04-16) on Ubuntu 16.04, 64 bit!

The fix is now verified on Latest Nightly.

Build ID 	20170524100215
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170524]