Add cookies and set cookies columns

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Netmonitor
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: ntim, Assigned: Vangelis Katsikaros (unavailable until May '18))

Tracking

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

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

Firefox Tracking Flags

(firefox55 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Cookies: number of cookies
Set Cookies: number of set cookies

(Like on chrome)
(Reporter)

Updated

a year ago
Keywords: good-first-bug
Assignee: nobody → vkatsikaros
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8861302 - Flags: review?(ntim.bugs) → review?(rchien)

Comment 2

a year 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

a year 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

a year 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

a year 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+
(Reporter)

Updated

a year ago
Attachment #8861302 - Flags: review?(rchien)
(Assignee)

Comment 7

a year 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

a year 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

a year 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?
(Reporter)

Comment 11

a year ago
(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

a year 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

a year ago
mozreview-review
Comment on attachment 8861302 [details]
Bug 1356869 - Add cookies and set cookies columns.

https://reviewboard.mozilla.org/r/133264/#review136694
(Reporter)

Updated

a year ago
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

a year 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

a year 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.
(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 : "";
```
(Reporter)

Comment 19

a year ago
Fair enough, feel free to drop the issues then.
Comment hidden (mozreview-request)
(Reporter)

Comment 21

a year 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

a year ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3e5ffcd2bba0
Add cookies and set cookies columns. r=ntim,rickychien
(Reporter)

Comment 25

a year ago
The other bug is probably guilty.
Keywords: checkin-needed

Comment 26

a year 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
(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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3651cb71e99f
Status: ASSIGNED → RESOLVED
Last Resolved: a year 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)
(Reporter)

Comment 30

a year ago
(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

a year ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dee7a59bc560
Update Set cookies header string comment. r=flod
(Reporter)

Updated

a year ago
Keywords: dev-doc-needed

Comment 33

a year 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]
(Reporter)

Updated

a year ago
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.