Closed Bug 1323454 Opened 7 years ago Closed 7 years ago

Network panel: integrate HTTP Status code with MDN

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox54 verified)

VERIFIED FIXED
Firefox 54
Iteration:
54.3 - Mar 6
Tracking Status
firefox54 --- verified

People

(Reporter: Honza, Assigned: kenweilee, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, good-first-bug, Whiteboard: [netmonitor-reserve])

Attachments

(2 files, 7 obsolete files)

Attached image status-code.png
See also bug 1323442

The "Headers" side panel available in the Network panel renders HTTP status code. We should display a link next to it pointing to related MDN page (if available)

Comments:

- The UI should be consistent with the Console panel and HTTP Headers (bug 1323442)
- We can render just simple "[learn more]" link (no quotes) next to the Status code
- List of MDN pages related to status code should be hardcoded.
- See attached mockup showing the link next to the status code field.

Honza
Whiteboard: [netmonitor][triage]
Priority: -- → P3
Whiteboard: [netmonitor][triage] → [netmonitor]
Hi Honza, should this bug be added to the MVP or Reserve backlog?
Flags: qe-verify?
Flags: needinfo?(odvarko)
Priority: P3 → --
Reserve backlog

Honza
Mentor: odvarko
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(odvarko)
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: [netmonitor] → [netmonitor-reserve]
I'm new to this stuff and trying to get my foot in the door. How would we go about implementing this?
No longer depends on: 1330858
(In reply to Parker from comment #4)
> I'm new to this stuff and trying to get my foot in the door. How would we go
> about implementing this?
Sounds great!

The UI implementation of the 'Headers' side 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

2) Rendering of the Status code is done in HeadersPanel.render() method
https://dxr.mozilla.org/mozilla-central/rev/6a23526fe5168087d7e4132c0705aefcaed5f571/devtools/client/netmonitor/shared/components/headers-panel.js#147

3) The '[more link]' should be displayed next to the status code and text (after the 'input' element, see the render method).

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.

4) Not all status codes are documented on MDN so, we should have hard-coded list of documented statuses in haders-panel.js file. The link should appear only for statuses that are actually documented.

MDN page for status codes:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status

MDN page for status 200 OK:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/200


Honza
Btw. there is similar bug for HTTP headers (bug 1320233)

Honza
Priority: P3 → P4
Hi guys,
I am learning to contribute to Mozilla. As [:Honza] pointed, I made change in headers-panel.js. 
The items are displayed using this way in render():
input({
            className: "tabpanel-summary-value textbox-input devtools-monospace",
            readOnly: true,
            value: `${status} ${statusText}`,
I tried adding a link beside it by:
input({
            className: "tabpanel-summary-value textbox-input devtools-monospace",
            readOnly: true,
            value: "[Learn More]".link("www.link.com"),}),
But, the value is displayed as HTML tags. I think because it is within input() and text-box input. How do I find other classes which can display HTML content within the dev-tools? Thanks in advance for any help.
Saghan, thanks for interesting in contribute!


You have a good start to trace the code and successfully find the `renderSummary` function

https://github.com/mozilla/gecko-dev/blob/master/devtools/client/netmonitor/shared/components/headers-panel.js#L81

You can import and add extra link element(`a`) after the input element to host the `[Learn More]` link.
You can refer `headerDocURL` in same file to find out how to construct the link element.

Then you could check the style and make sure UI shows correctly. (You might need apply flex flexible style to auto adjust the input field width)


You can reach us at IRC#devtools or just use bugzilla's needinfo if you need help
Flags: needinfo?(saghan99)
Depends on: 1339686
Once bug 1339686 is landed, you can use `MdnLink({ link: statusURL })` to render MDN status code link
Hi,

This is my first time trying my hand at this and I've stumbled into a problem at the last leg of this bug.

I have the `[Learn More]` link implemented and it's working as desired, the link only appearing if there's a valid MDN page.  The only issue I have now is that I can't quite seem to figure out how to style it.

I've looked at the `headerDocURL` section to try and use it but part of that code relies on the props parameter to renderValue() but render() has no such parameter.
Flags: needinfo?(gasolin)
Hi Ken, thanks for take it a look!

please put your work-in-progress patch onto bugzilla (via tap `Add an attachment` link on this page) and I can help check what should we do for the next step.

The basic idea is we have `Rep` and `MDNLink` element inside of `.treeValueCellDivider` class, and we should put `MDNLink` element always align to the right side of the space. So the result might be add a rule such as

```
.treeValueCellDivider > learn-more-link {
   (... align MDNLink to right)
}
```

You should also check headers panel's CSS and learn how it shows [Learn More] link besides the value.
Flags: needinfo?(gasolin) → needinfo?(kenweilee)
Attached patch wip.patch (obsolete) — Splinter Review
Hi Fred,

I have attached the WIP patch.  As you will see, I did use `treeValueCellDivider` so the text is aligned to the right.  However, I still need to make it blue and change it so that it only one line (it currently uses two lines).

I am not really sure how to use Rep, any help would be appreciated.
Flags: needinfo?(kenweilee) → needinfo?(gasolin)
const { getHeadersURL } = require("../../utils/mdn-utils");
+const { getHTTPStatusCodeURL } = require("../../utils/mdn-utils");

You can combine these 2 lines as 

```
const {
  getHeadersURL,
  getHTTPStatusCodeURL,
} = require("../../utils/mdn-utils");
```

+function getHTTPStatusCodeURL(statusCode) {
+  let idx = SUPPORTED_HTTP_CODES.findIndex(item => 

You can use simpler `SUPPORTED_HTTP_CODES.indexOf(statusCode)` instead of findIndex, since there's no capital issues in status code.

div
+            className: "treeValueCellDivider"},
+            statusCodeDocURL ? MDNLink({
+              url: statusCodeDocURL,
+            }) : null
+          ),


Sorry the comment 11 CSS style is based on header's condition.
For this case, instead of wrapping extra `div`, you can use MDNLink directly

```
statusCodeDocURL ? MDNLink({
  url: statusCodeDocURL,
}) : null
```

In this case, the MDNLink element should be near `devtools-button`. 

And you can use inspector to debug the devtools netmonitor as normal web page by following
https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox
Flags: needinfo?(gasolin)
Assignee: nobody → kenweilee
Status: NEW → ASSIGNED
Hi Fred,

Using the debugger, I have found out why the styling is not applied:

```
.treeTable .treeValueCell .learn-more-link {
  color: var(--theme-highlight-blue);
  cursor: pointer;
  margin 0 5px;
}

.treeTable .treeValueCell .learn-more-link:hover {
  text-decoration: underline;
}
```

I'm wondering if it'd be a good idea to remove `.treeTable .treeValueCell` and have the rule moved to netmonitor.css so that any `.learn-more-link` would be managed by the same style or if I should copy the style separately into netmonitor.css (where .tabpanel-summary-container and .headers-summary are found?)
Flags: needinfo?(gasolin)
Good catch! The styles you found in tree-view.css is the basic style, you don't need to touch it
http://searchfox.org/mozilla-central/source/devtools/client/shared/components/tree/tree-view.css#87

You could just add new rule in netmonitor.css to overwrite the styles for this case.
Flags: needinfo?(gasolin)
Attached patch roughfinal.patch (obsolete) — Splinter Review
Hi,

I've attached the rough draft of the finalized patch for review.
Attachment #8839303 - Attachment is obsolete: true
Attachment #8839954 - Flags: review?(odvarko)
I can't apply the patch on latest m-c. Can you please rebase?

Honza

patching file devtools/client/netmonitor/shared/components/headers-panel.js
Hunk #1 FAILED at 11
Hunk #2 FAILED at 188
2 out of 2 hunks FAILED -- saving rejects to file devtools/client/netmonitor/shared/components/headers-panel.js.rej
patching file devtools/client/netmonitor/utils/mdn-utils.js
Hunk #1 FAILED at 120
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/utils/mdn-utils.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
Hi Honza,

Sorry, but could you elaborate on that?  I'm very new to the process and Mercurial.

I read that as needing to get the latest mozilla-central and making a new patch file with my changes.

Is there an easier way to do that than manually copying what I added, updating to latest, and putting the code back in before making a new patch file?
Flags: needinfo?(odvarko)
Attached patch final.patch (obsolete) — Splinter Review
Hi Honza,

I fiddled around with Mercurial and tried to reset it to the latest before adding the fix again.

I hope this one works.
Attachment #8839954 - Attachment is obsolete: true
Attachment #8839954 - Flags: review?(odvarko)
Attachment #8840240 - Flags: review?(odvarko)
The code looks good, but the [learn more] link should be shown near the status code as the mockup provided by Honza
https://bugzilla.mozilla.org/attachment.cgi?id=8818568


And please 

* put CSS styles under /* Headers tabpanel */ section so we know this changes are related to Header's panel
* remove /* Learn more link */ since the class already explains all
Add `flex-grow: 1;` into `.tabpanel-summary-container .learn-more-link` rule and set input field width based on its value (304 Not modified -> width: 60px, 200 OK -> width: 40px) could do the trick. 

Though it will be great if this can be solved by pure CSS.
Comment on attachment 8840240 [details] [diff] [review]
final.patch

Review of attachment 8840240 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me too.

Please see my inline comments.

Also, please set correct commit message:

Bug 1323454 - Network panel: integrate HTTP Status code with MDN; r=Honza,gasolin

Thanks for working on this!
Honza

::: devtools/client/netmonitor/shared/components/headers-panel.js
@@ +12,4 @@
>  } = require("devtools/client/shared/vendor/react");
>  const { L10N } = require("../../utils/l10n");
>  const { writeHeaderText } = require("../../utils/request-utils");
> +const { 

nit: remove whitespace at the end of the line

::: devtools/client/netmonitor/utils/mdn-utils.js
@@ +95,5 @@
> +    "406",
> +    "410",
> +    "412",
> +    "451",
> +    "500",        

nit: remove whitespaces at the end of the line
Attachment #8840240 - Flags: review?(odvarko)
(In reply to Ken Lee from comment #19)
> Created attachment 8840240 [details] [diff] [review]
> final.patch
> 
> Hi Honza,
> 
> I fiddled around with Mercurial and tried to reset it to the latest before
> adding the fix again.
Yep, works fine now, thanks!

Honza
Flags: needinfo?(odvarko)
Attached patch final2.patch (obsolete) — Splinter Review
Hi,

Here's the patch with the necessary fixes as well as making it look like the mock-up. It builds on top of final.patch, I hope that's alright/the proper way to do this.
Attachment #8840677 - Flags: review?(odvarko)
Excellent, thanks!

I tested this and looks good to me.

Having two patches is ok.

But, the commit message is still not set (see my comment #22)

Commit message for the first patch might be somethings like as follows:
Bug 1323454 - Render learn more link for HTTP Status code; r=Honza,gasolin

...and the second 
Bug 1323454 - Fix learn more link layout; r=Honza,gasolin

Honza
Flags: needinfo?(kenweilee)
Hi Honza,

I used hg histedit to change the commit messages.

When I tried to push it said it'd create a new remote head and aborted the operation.  Should I go ahead and have it create a new remote head?  It also suggested I try merging before the push, but I didn't want to screw anything up by doing something I'm unsure of.

Also I'm not really sure if there's something I should be doing with the treeherder link.
Flags: needinfo?(kenweilee) → needinfo?(odvarko)
> Also I'm not really sure if there's something I should be doing with the treeherder link.

The Try push (comment #26) says that there is an Eslint error:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8830849df6d11f72e8294b445b8f09c35fa33790&selectedJob=79971429

So, this needs to be fixed:
R | /home/worker/checkouts/gecko/devtools/client/netmonitor/shared/components/headers-panel.js:191:1 | Line 191 exceeds the maximum line length of 90. (max-len)

I am using mercurial queues [1] and all you need to do to set the commit message is to push the patch into hg queue and use:

hg qref --message "<commit-message>"

(this will set commit message for the last applied patch in the queue)

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Mercurial/Queues

--

As soon as the eslint and commit message are fixed we'll mark this bug as 'checkin-needed' and somebody will land the patch(es) for us.


Honza
Flags: needinfo?(odvarko)
Attached patch final.patch (obsolete) — Splinter Review
Changed commit message of first final patch.
Attachment #8840240 - Attachment is obsolete: true
Attached patch final2.patch (obsolete) — Splinter Review
Fixed commit message of final2.patch
Attachment #8840677 - Attachment is obsolete: true
Attachment #8840677 - Flags: review?(odvarko)
Attached patch final3.patch (obsolete) — Splinter Review
Hi,

This third patch fixes the line length.

I have changed the commit messages of the previous 2 patches and re-uploaded them as well.
Attachment #8841602 - Flags: review?(odvarko)
I am still not seeing the commit message in your patches so, I merged all of them into one and set it. Please see the header at the top of the patch.

Especially the line:
Bug 1323454 - integrate HTTP Status code with MDN; r=Honza
(it's the commit message)

Here is new try push so, we know that there are no problems before it's landed.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=200e589f599136885e385c87d9ad405e6eb7ced5


See more info about Try server here:
https://wiki.mozilla.org/ReleaseEngineering/TryServer


Let's see how it goes! :-)

Honza
Attachment #8841616 - Flags: review?(odvarko)
Attachment #8841598 - Attachment is obsolete: true
Attachment #8841599 - Attachment is obsolete: true
Attachment #8841602 - Attachment is obsolete: true
Attachment #8841602 - Flags: review?(odvarko)
I was using hg commit to set the message, but I guess that doesn't work for patches.

For the future, I think I'll use the Mercurial Queue; thanks for showing it to me!
Attachment #8841616 - Flags: review?(odvarko) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52078590e97f
integrate HTTP Status code with MDN; r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/52078590e97f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Iteration: --- → 54.3 - Mar 6
Priority: P4 → P1
QA Contact: ciprian.georgiu
Depends on: 1343803
This issue is verified fixed on latest Aurora 54.0a2 (2017-03-06) using Windows 10 x64 and Mac OS X 10.11.6. 

I've verified this based on the info from comment 0, and I can confirm that the [Learn More] link from the Headers side panel is working as expected.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(saghan99)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.