Add cookies and set cookies columns

VERIFIED FIXED in Firefox 55

Status

enhancement
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: ntim, Assigned: vkatsikaros)

Tracking

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

unspecified
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

Attachments

(1 attachment)

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

(Like on chrome)
Keywords: good-first-bug
Assignee: nobody → vkatsikaros
Status: NEW → ASSIGNED
Attachment #8861302 - Flags: review?(ntim.bugs) → review?(rchien)
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.
Attachment #8861302 - Flags: review?(rchien)
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 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 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.
Attachment #8861302 - Flags: review?(ntim.bugs) → review+
Attachment #8861302 - Flags: review?(rchien)
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 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.
Attachment #8861302 - Flags: review?(rchien) → review-
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?
(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 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 on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

https://reviewboard.mozilla.org/r/133264/#review136694
Attachment #8861302 - Flags: review?(rchien)
(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 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.
Attachment #8861302 - Flags: review?(rchien) → review+
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 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.
(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 : "";
```
Fair enough, feel free to drop the issues then.
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\"]");
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3e5ffcd2bba0
Add cookies and set cookies columns. r=ntim,rickychien
The other bug is probably guilty.
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/3651cb71e99f
Add cookies and set cookies columns. r=ntim,rickychien
Keywords: checkin-needed
(In reply to Tim Nguyen :ntim from comment #25)
> The other bug is probably guilty.

Thanks Tim, it looks like it
Flags: needinfo?(vkatsikaros)
https://hg.mozilla.org/mozilla-central/rev/3651cb71e99f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
"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.
Flags: needinfo?(vkatsikaros)
(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.
Flags: needinfo?(vkatsikaros)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dee7a59bc560
Update Set cookies header string comment. r=flod
Keywords: dev-doc-needed
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]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.