Add cookies and set cookies columns

VERIFIED FIXED in Firefox 55

Status

VERIFIED FIXED
2 years ago
5 months ago

People

(Reporter: ntim, Assigned: vkatsikaros)

Tracking

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

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

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)

Updated

2 years ago
Assignee: nobody → vkatsikaros
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Attachment #8861302 - Flags: review?(ntim.bugs) → review?(rchien)

Comment 2

2 years ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
mozreview-review-reply
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.
(Assignee)

Comment 5

2 years ago
mozreview-review
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?
(Reporter)

Comment 6

2 years ago
mozreview-review
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)
(Assignee)

Comment 7

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
mozreview-review-reply
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.
(Reporter)

Comment 12

2 years ago
mozreview-review
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
(Reporter)

Comment 13

2 years ago
mozreview-review
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 15

2 years ago
mozreview-review
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 17

2 years ago
mozreview-review
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.
(Assignee)

Comment 18

2 years ago
(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 hidden (mozreview-request)
(Reporter)

Comment 21

2 years ago
mozreview-review
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 hidden (mozreview-request)

Comment 23

2 years ago
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

Comment 26

2 years ago
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
(Assignee)

Comment 27

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

Thanks Tim, it looks like it
Flags: needinfo?(vkatsikaros)

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3651cb71e99f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
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)

Comment 31

2 years ago
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

Comment 33

2 years ago
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
status-firefox55: fixed → verified

Updated

5 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.