Soledad Penades [:sole] [:spenades] Reporter
	Description • 8 years ago

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

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 1 • 8 years ago

Yes, I like the idea!

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

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 3 • 8 years ago

See bug 1323454 for HTTP Status code

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	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

	Eduardo Bouças 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!

	Jan Honza Odvarko [:Honza] (always need-info? me)
	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

	Eduardo Bouças 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!

	Jan Honza Odvarko [:Honza] (always need-info? me)
	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

	Jan Honza Odvarko [:Honza] (always need-info? me)
	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?

	Eduardo Bouças 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.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	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

	Eduardo Bouças 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!

	Jan Honza Odvarko [:Honza] (always need-info? me)
	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

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 14 • 7 years ago

@eduardo: where you able to push to try? Is there anything I can help with?

Honza

	Eduardo Bouças 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.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	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

	Eduardo Bouças 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.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	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

	Eduardo Bouças 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

	Jan Honza Odvarko [:Honza] (always need-info? me)
	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

	Eduardo Bouças 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?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 22 • 7 years ago

Done

Honza

	Pulsebot
	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

	Ryan VanderMeulen [:RyanVM]
	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

	Jan Honza Odvarko [:Honza] (always need-info? me)
	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

	Carsten Book [:Tomcat]
	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

	Eduardo Bouças 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!

	Pulsebot
	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

	gasolin@mozilla.com
	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.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	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

	Carsten Book [:Tomcat]
	Comment 31 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/031087a1f3c6

	Mohammad Maruf Rahman
	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]

	Md. Majedul islam
	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]

	Sebastian Zartner [:sebo]
	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

	Dustin Driver
	Comment 35 • 7 years ago

Doc updates here: https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Headers