Closed Bug 1320233 Opened 3 years ago Closed 3 years ago

Make HTTP response headers link to documentation in MDN

Categories

(DevTools :: Netmonitor, defect, P3)

defect

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)

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
Yes, I like the idea!

@Helen, I'll put this on list of things we can discuss in Hawaii

Honza
Priority: -- → P3
See bug 1323454 for HTTP Status code

Honza
Component: Developer Tools → Developer Tools: Netmonitor
Depends on: 1330858
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
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!
(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
Attached patch 1320233-v1.0.0.patch (obsolete) — Splinter Review
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 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 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-
Attached patch 1320233-v1.1.0.patch (obsolete) — Splinter Review
> 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 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+
> 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)
(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)
@eduardo: where you able to push to try? Is there anything I can help with?

Honza
Flags: needinfo?(mail)
(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)
(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
@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.
@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
(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
@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
Flags: needinfo?(mail)
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)
Done

Honza
Flags: needinfo?(sole)
Keywords: checkin-needed
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
@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)
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)
Keywords: checkin-needed
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
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)
(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)
https://hg.mozilla.org/mozilla-central/rev/031087a1f3c6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Depends on: 1337737
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]
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]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.