Closed
Bug 1320233
Opened 7 years ago
Closed 7 years ago
Make HTTP response headers link to documentation in MDN
Categories
(DevTools :: Netmonitor, defect, P3)
DevTools
Netmonitor
Tracking
(firefox54 fixed)
RESOLVED
FIXED
Firefox 54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: sole, Assigned: mail, Mentored)
References
Details
(Keywords: dev-doc-complete, good-first-bug)
Attachments
(2 files, 2 obsolete files)
356.83 KB,
image/png
|
Details | |
10.47 KB,
patch
|
Details | Diff | Splinter Review |
The MDN team recently finished documenting a set of common HTTP response headers. Jean-Yves proposed that we include this information in devtools to help developers access it faster. They have had great feedback and engagement after including links to common JS error codes in the console. An obvious first place to add this could be making the name of each header clickable when inspecting the response of an asset, in the Network panel. This should not require any additional pop up or UI element - the links should open in a new tab. (see screenshot) Maybe the console might want to include these links if header names are displayed there (I don't think so but I might have missed it). Note that not all headers are documented. Perhaps we can keep a list of the ones we're aware of, to avoid linking to things that do not exist yet, etc. CC:ing Honza as "Network panel supreme" and also Jean-Yves and Florian as "MDN supremes" -- let's do this! :D
Comment 1•7 years ago
|
||
Yes, I like the idea! @Helen, I'll put this on list of things we can discuss in Hawaii Honza
Priority: -- → P3
Comment 3•7 years ago
|
||
See bug 1323454 for HTTP Status code Honza
Comment 4•7 years ago
|
||
The UI implementation of the 'Headers' panel is based on React and so, some knowledge of the library is needed. Here are some comments that should help when implementing this feature. 1) The (React) component representing the Headers side panel is implemented here: https://github.com/mozilla/gecko-dev/blob/master/devtools/client/netmonitor/shared/components/headers-panel.js 3) The HeadersPanel is using PropertiesView to render individual headers and their values https://github.com/mozilla/gecko-dev/blob/master/devtools/client/netmonitor/shared/components/properties-view.js See also: HeadersPanel.render() method. 4) The HeadersPanel should be responsible for customizing the way how the header name + '[more link]' (pointing to related MDN page) is rendered. 5) The PropertiesView uses TreeView and already customizes the rendering using TreeView's 'renderValue' property. See 'renderValueWithRep' method. 6) I think the the PropertiesView should introduce a new `renderValue` property that can be set by HeadersPanel and provide custom implementation of header value rendering. This custom representation should be responsible for rendering the '[learn more]' link pointing to related page in MDN 7) Not every HTTP header is documented on MDN adn so, we should also have hard-coded list of headers (in headers-panel.js file) that are documented on MDN). A '[lear more]' link should appear only next to headers that are actually documented. An example of MDN pages for HTTP headers: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Type https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Date Here is a screenshot showing how the '[learn more]' link looks like in the Console panel: https://bug1323442.bmoattachments.org/attachment.cgi?id=8818543 The Headers panel should use the same design. Honza
Mentor: odvarko
Keywords: good-first-bug
Assignee | ||
Comment 5•7 years ago
|
||
I've implemented this, as shown in the screencast: http://cloud.dadi.technology/1xk9dm I tried to follow the designs provided as closely as possible, but I'm more than happy to make any adjustments you think is necessary. I'm happy to also look into bug 1323454, as we should be able to reuse most of the code from here. I'll work on some tests tomorrow and then I'll submit for your review. Thanks!
Comment 6•7 years ago
|
||
(In reply to Eduardo Bouças from comment #5) > I've implemented this, as shown in the screencast: > http://cloud.dadi.technology/1xk9dm Looks good! > I tried to follow the designs provided as closely as possible, but I'm more > than happy to make any adjustments you think is necessary. Great, please attach your patch and I'll review it. > I'm happy to also > look into bug 1323454, as we should be able to reuse most of the code from > here. Sounds good to me, the bug isn't assigned to anyone yet. > I'll work on some tests tomorrow and then I'll submit for your review. Thanks! Honza
Assignee | ||
Comment 7•7 years ago
|
||
Here's my first stab at this. I still need to do some more work on the mochitest, but I'm blocked by an error I'm still trying to fix: FATAL ERROR: Non-local network connections are disabled and a connection attempt to developer.mozilla.org (63.245.215.53) was made. This happens when triggering a click on the "Learn More" link and opening the MDN link. I tried looking around in documentation and other bugs, but couldn't find a solution yet. In the meantime, I'd love to hear your thoughts on the patch. Happy to tweak as much as necessary. Thanks!
Attachment #8831565 -
Flags: ui-review+
Attachment #8831565 -
Flags: review+
Comment 8•7 years ago
|
||
Comment on attachment 8831565 [details] [diff] [review] 1320233-v1.0.0.patch Excellent, thanks for the patch I'll look at it! Btw. when you asking for a review you need to use ? (question-mark) and fill out the 'Requestee' field. The reviewer is consequently using + or - depending on whether the patch needs more work or not. Honza
Attachment #8831565 -
Flags: ui-review+
Attachment #8831565 -
Flags: review?(odvarko)
Attachment #8831565 -
Flags: review+
Comment 9•7 years ago
|
||
Comment on attachment 8831565 [details] [diff] [review] 1320233-v1.0.0.patch Review of attachment 8831565 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch, it looks very good! Please change the commit message: Bug 1320233 - Add Learn More link to headers in Network panel -> Bug 1320233 - Add Learn More link to headers in Network panel; r=Honza Also, see my inline comments. Honza ::: devtools/client/locales/en-US/netmonitor.properties @@ +735,5 @@ > netmonitor.custom.headers=Request Headers: > > +# LOCALIZATION NOTE (netmonitor.custom.learnMore): This is the label displayed > +# next to a header list item, with a link to external documentation > +netmonitor.custom.learnMore=Learn More Please change the string key to: netmonitor.headers.learnMore (the string is used in Headers side panel not in the custom request form) ::: devtools/client/netmonitor/shared/components/headers-panel.js @@ +240,5 @@ > + let headerDocURL = HeaderDocs.getURL(member.name); > + > + return ( > + div({ className: "treeValueCellDivider" }, > + span({ className: "objectBox objectBox-string" }, `"${value}"`), Can we use Rep() component to render the string? Something like as follows: return ( div({ className: "treeValueCellDivider" }, Rep(Object.assign(props, { // FIXME: A workaround for the issue in StringRep // Force StringRep to crop the text everytime member: Object.assign({}, member, { open: false }), mode: MODE.TINY, cropLimit: 60, })), headerDocURL ? a({ className: "learn-more-link", title: headerDocURL, onClick: (e) => onLearnMoreClick(e, headerDocURL), }, `[${L10N.getStr("netmonitor.custom.learnMore")}]`) : null ) ); You also need to require Rep module: const { REPS, MODE } = require("devtools/client/shared/components/reps/load-reps"); const Rep = createFactory(REPS.Rep); ::: devtools/client/netmonitor/test/browser_net_header-docs.js @@ +43,5 @@ > + /* > + * Tests that a "Learn More" button is only shown iff > + * header is documented in MDN. > + */ > + function testShowLearnMore(data) { nit: Remove whitespaces at the end of the line ::: devtools/server/actors/headerdocs.js @@ +1,1 @@ > +/* this source code form is subject to the terms of the mozilla public Please move (and rename) this file: devtools/client/netmonitor/shared/components/headers-mdn.js (HeadersMDN) @@ +10,5 @@ > +"use strict"; > + > +const baseURL = "https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/"; > +const params = > + "?utm_source=mozilla&utm_medium=firefox-network-panel&utm_campaign=default"; What is the utm_medium arg for? Could it be "devtools-netmonitor"? Is there any docs for the arg?
Attachment #8831565 -
Flags: review?(odvarko) → review-
Assignee | ||
Comment 10•7 years ago
|
||
> What is the utm_medium arg for?
> Could it be "devtools-netmonitor"?
> Is there any docs for the arg?
`utm_medium` is typically used for tracking purposes. I followed the convention used in devtools/server/actors/errordocs.js. I've now renamed the arg to "devtools-netmonitor" as requested.
I've attached a modified patch addressing your feedback. Sorry about some misnamed/misplaced things — did my best to follow existing conventions, but this was the first time I worked with this codebase.
Let me know if there's anything else you'd like me to change.
Attachment #8831565 -
Attachment is obsolete: true
Attachment #8831694 -
Flags: review?(odvarko)
Comment 11•7 years ago
|
||
Comment on attachment 8831694 [details] [diff] [review] 1320233-v1.1.0.patch Review of attachment 8831694 [details] [diff] [review]: ----------------------------------------------------------------- Great work here! R+ assuming Try is green. (let me know if I should push the patch to the Try server) Honza
Attachment #8831694 -
Flags: review?(odvarko) → review+
Assignee | ||
Comment 12•7 years ago
|
||
> Great work here! Thanks! > R+ assuming Try is green. > (let me know if I should push the patch to the Try server) I don't have access to the Try server. I just followed the documentation and created Bug 1335182. I believe I need someone to vouch — can you do that? Thanks!
Flags: needinfo?(odvarko)
Comment 13•7 years ago
|
||
(In reply to Eduardo Bouças from comment #12) > I don't have access to the Try server. I just followed the documentation and > created Bug 1335182. I believe I need someone to vouch — can you do that? Done Honza
Flags: needinfo?(odvarko)
Comment 14•7 years ago
|
||
@eduardo: where you able to push to try? Is there anything I can help with? Honza
Flags: needinfo?(mail)
Assignee | ||
Comment 15•7 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #14) > @eduardo: where you able to push to try? Is there anything I can help with? > > Honza Sorry for the delay — crazy day here. I was able to push to try, but my mochitest is failing with the error I pasted earlier. I found the cause, though, and I'm updating the test now. Should be finished in a few hours.
Flags: needinfo?(mail)
Comment 16•7 years ago
|
||
(In reply to Eduardo Bouças from comment #15) > Sorry for the delay — crazy day here. I was able to push to try, but my > mochitest is failing with the error I pasted earlier. I found the cause, > though, and I'm updating the test now. Should be finished in a few hours. I see, thanks! Honza
Assignee | ||
Comment 17•7 years ago
|
||
@Honza: I pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6951b1cb6d6a1f75136dc817048a11a67279d9b8&selectedJob=73762781 I'm still not fully happy with the test, but I'll ping you on IRC if that's okay.
Comment 18•7 years ago
|
||
@Eduardo: I am seeing some eslint failures: TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/netmonitor/shared/components/headers-mdn.js:14:1 | Line 14 exceeds the maximum line length of 90. (max-len) TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/netmonitor/shared/components/headers-mdn.js:118:2 | Missing semicolon. (semi) TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/netmonitor/shared/components/headers-panel.js:27:24 | 'span' is assigned a value but never used. (no-unused-vars) TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/netmonitor/shared/components/headers-panel.js:240:24 | Strings must use doublequote. (quotes) Honza
Assignee | ||
Comment 19•7 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #18) > @Eduardo: I am seeing some eslint failures: > Sorry, my bad. Sorted: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12faf22e99691677a95211955ab6a58c9ccebdf9&selectedJob=73969227
Comment 20•7 years ago
|
||
@Eduardo, looks good to me (the other test failures seem unrelated) You can add 'checkin-needed' into the 'Keywords' field (at the top of this page) to get the patch landed. Honza
Updated•7 years ago
|
Flags: needinfo?(mail)
Assignee | ||
Comment 21•7 years ago
|
||
I don't seem to have permissions to edit the "Keywords" field. Is it perhaps something that the reporter can do?
Flags: needinfo?(mail) → needinfo?(sole)
Comment 23•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b2b5d0dda746 Add Learn More link to headers in Network panel. r=Honza
Keywords: checkin-needed
Comment 24•7 years ago
|
||
Backed out for ESLint failures. https://treeherder.mozilla.org/logviewer.html#?job_id=74290108&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/1bacf545f4fd0b149e7903c03fe4776bda39748a
Assignee: nobody → mail
Comment 25•7 years ago
|
||
@Eduardo: Still some ESlint failures: 8461 TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/netmonitor/shared/components/headers-panel.js:27:24 | 'span' is assigned a value but never used. (no-unused-vars) 8462 TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/netmonitor/shared/components/headers-panel.js:238:24 | Strings must use doublequote. (quotes) Honza
Flags: needinfo?(mail)
Comment 26•7 years ago
|
||
also it seems a failure on own test like https://treeherder.mozilla.org/logviewer.html#?job_id=74295851&repo=mozilla-inbound
Assignee | ||
Comment 27•7 years ago
|
||
Sorry, guys. I forgot to attach the latest version of the patch to the ticket, so the patch you tried to push was outdated, hence the errors. I've attached the new patch (v1.2.0) now. On try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=247af98478ae0f65a87cf1ddf1e0669f51436507&selectedJob=74394503 Thanks!
Attachment #8831694 -
Attachment is obsolete: true
Flags: needinfo?(mail)
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 28•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/031087a1f3c6 Add Learn More link to headers in Network panel. r=Honza
Keywords: checkin-needed
Comment 29•7 years ago
|
||
Sorry for late discussion. According to comment 4, I have some concern with hard-coded list of headers 7) Not every HTTP header is documented on MDN adn so, we should also have hard-coded list of headers (in headers-panel.js file) that are documented on MDN). A '[lear more]' link should appear only next to headers that are actually documented. I think to add missing header document on MDN is way easier then hard-coded list of headers in headers panel. Add missing header document on MDN ends up make MDN a better reference resource. If we hard-code the list of headers, it leads to maintain overhead and we likely missed (or require extra effort to add) any new header updates from MDN.
Flags: needinfo?(odvarko)
Comment 30•7 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #29) > I think to add missing header document on MDN is way easier then hard-coded > list of headers in headers panel. Add missing header document on MDN ends up > make MDN a better reference resource. Yes, agree that improving MDN & docs is definitely what we want but, there can always be some custom headers that will never be documented. Also note that we want to avoid unnecessary HTTP requests to MDN (for docs that doesn't exist). > If we hard-code the list of headers, it leads to maintain overhead and we > likely missed (or require extra effort to add) any new header updates from > MDN. We need to be careful about this and cooperate with MDN team to update our list when needed (also note that list of JS Error is also hardcoded). Honza
Flags: needinfo?(odvarko)
Comment 31•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/031087a1f3c6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment 32•7 years ago
|
||
I have reproduced this bug with Nightly 53.0a1 (2016-11-24) (64-bit) on Windows 7, 64 Bit! This bug's fix is verified with latest Nightly! Build ID : 20170217030229 User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0 [bugday-20170222]
Comment 33•7 years ago
|
||
I have reproduced this bug with Nightly 53.0a1 (2016-11-24) (64-bit) on ubuntu 16.10,64 Bit! This bug's fix is verified with latest Nightly ! Build ID : 20170301110155 User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.053.0a1 (2016-11-24) (64-bit) [bugday-20170301]
Updated•7 years ago
|
Keywords: dev-doc-needed
Comment 34•7 years ago
|
||
Mentioned this feature at https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Headers and added a developer release note to https://developer.mozilla.org/en-US/Firefox/Releases/54#Developer_Tools. Sebastian
Keywords: dev-doc-needed → dev-doc-complete
Comment 35•7 years ago
|
||
Doc updates here: https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Headers
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•