Closed Bug 1499042 Opened 6 years ago Closed 6 years ago

Fix learn more links in the Network panel

Categories

(DevTools :: Netmonitor, enhancement, P2)

enhancement

Tracking

(firefox65 fixed)

RESOLVED FIXED
Firefox 65
Tracking Status
firefox65 --- fixed

People

(Reporter: Honza, Assigned: lba_2, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: good-first-bug)

Attachments

(1 file, 5 obsolete files)

The Timings side panel contains a Learn more link currently linking to wrong MDN page. It should point to: https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor/request_details#Timings Here is the place where the link is calculated: https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/utils/mdn-utils.js#186 Honza
Mentor: odvarko
Keywords: good-first-bug
Priority: -- → P2
Whiteboard: good-first-bug
HI @Honza, since I am still waiting for some UI to work on my first bug, can I have a look at this one for outreachy? Thanks, Lenka
Flags: needinfo?(odvarko)
Sure, done! Honza
Assignee: nobody → lba_2
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
It seems that there are other similar problems with incorrect linking to documentation. -Same seems to be the case for Filter URLs -and same for when selecting Start Performance analysis on a request - > there is more info icon on Primed Cache and Empty cache that don't link to the correct MDN doc - possibly other places as well. I can have a look if assigned. Thnx. Lenka (lenpel)
(In reply to lba_2 from comment #3) > It seems that there are other similar problems with incorrect linking to > documentation. > -Same seems to be the case for Filter URLs > -and same for when selecting Start Performance analysis on a request - > > there is more info icon on Primed Cache and Empty cache that don't link to > the correct MDN doc > - possibly other places as well. > I can have a look if assigned. Thnx. Yes, please! I changed the bug title to cover all broken learn-more links in the Network panel. Assigned to you now. Thanks for helping with this! Honza
Summary: Fix learn more link in Timings panel → Fix learn more links in the Network panel
(In reply to Jan Honza Odvarko [:Honza] from comment #4) > (In reply to lba_2 from comment #3) > > It seems that there are other similar problems with incorrect linking to > > documentation. > > -Same seems to be the case for Filter URLs > > -and same for when selecting Start Performance analysis on a request - > > > there is more info icon on Primed Cache and Empty cache that don't link to > > the correct MDN doc > > - possibly other places as well. > > I can have a look if assigned. Thnx. > Yes, please! > > I changed the bug title to cover all broken learn-more links in the Network > panel. > > Assigned to you now. > > Thanks for helping with this! > Honza @Honza, how do I go about creating a patch for my first bug fix so I can have it reviewed, please? I am using Nightly and made corrections locally in 2 files in VS Code. *mdn-utils.js (to link to correct MDN documentation). *TimingsPanel.js (to show a tool tip "Learn more") thank you, Lenka
Flags: needinfo?(odvarko)
Sounds great! Generate a diff file using: hg diff > my-patch.patch ... and attach the file to this bug using `Attach File` link at the top. You should also set reviewer in the attachment details. Put question-mark into into the 'review' flag and use my email. Honza
Flags: needinfo?(odvarko)
Attached patch bug1499042.patch (obsolete) — Splinter Review
Patch - Fixes Learn more links in the Network panel and adds a Tooltip for Learn more in Timings
Attachment #9017801 - Flags: review?(odvarko)
Comment on attachment 9017801 [details] [diff] [review] bug1499042.patch Review of attachment 9017801 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch, looks great! But, please see my inline comments. Honza ::: browser/config/mozconfig @@ +8,5 @@ > > ac_add_options --enable-application=browser > + > +# Automatically download and use compiled C++ components: > +ac_add_options --enable-artifact-builds You shouldn't modify this file. Please remove this from the patch. The correct way is to crate a new `.mozconfig` file in you root source dir and put your custom build option in to it. ::: devtools/client/netmonitor/src/components/TimingsPanel.js @@ +95,5 @@ > div({ className: "panel-container" }, > timelines, > MDNLink({ > url: getNetMonitorTimingsURL(), > + title: L10N.getStr("netmonitor.headers.learnMore") Can you please introduce a new localized string: netmonitor.timings.learnMore=Learn more about timings ... and put it into netmonitor.properties next to the other "netmonitor.timings.*" strings.
Attachment #9017801 - Flags: review?(odvarko)
Attached patch bug1499042_v4.patch (obsolete) — Splinter Review
This patch corrects 'Learn more' links in Network panel for: Timings, Performance analysis and Filter box. In addition it also adds tool tip text to all Learn more links, specifically: in Timings, next to both charts in the performance analysis, in an empty Network panel for performance analysis and for Status Code in Headers.
Attachment #9018064 - Flags: review?(odvarko)
Comment on attachment 9018064 [details] [diff] [review] bug1499042_v4.patch Review of attachment 9018064 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the update! Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6856903af63f4860d4cc985820ff5a3a5fa2f874&selectedJob=206308425 I am seeing eslint failures, please fix I am also seeing this test to fail: devtools/client/responsive.html/test/browser/browser_toolbox_rule_view_reload.js ...but looks like unrelated intermittent. Honza
Attachment #9018064 - Flags: review?(odvarko)
Please, mark the old patches as obsolete (Details -> Edit Details -> Check Obsolete checkbox) Honza
Attachment #9017801 - Attachment is obsolete: true
Attachment #9018064 - Attachment is obsolete: true
Attached patch bug1499042_v5.patch (obsolete) — Splinter Review
Honza, I corrected the ESLint errors from the previous patch. Marked the old patches as obsolete. I am not quite sure what to do (if anything) with the failed test devtools/client/responsive.html/test/browser/browser_toolbox_rule_view_reload.js thnx, Lenka
Attachment #9018270 - Flags: review?(odvarko)
Comment on attachment 9018270 [details] [diff] [review] bug1499042_v5.patch (In reply to lba_2 from comment #12) > I am not quite sure what to do (if anything) with the failed test > devtools/client/responsive.html/test/browser/ > browser_toolbox_rule_view_reload.js This failure seems to be unrelated. I believe we are good to go here, thanks for the help! Honza
Attachment #9018270 - Flags: review?(odvarko) → review+
Pushed by ebalazs@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/646f2d021c11 "Fix learn more links in the Network panel". r=Honza
Keywords: checkin-needed
Backout by shindli@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/162dc0f0dbd5 Backed out changeset 646f2d021c11 for X failures in devtools/client/netmonitor/test/unit/test_mdn-utils.js
I looked into the failed test file : https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/unit/test_mdn-utils.js The test fails exactly on testing against the incorrect MDN Link which we are fixing in this bug. Same failure would happen for line 38 - getPerformanceAnalysisURL() where we are also fixing the incorrect MDN Link. It seems that the test file is also not up to date as it is NOT testing the url for getFilterBoxURL() which we are also correcting in this bug. Seems that the test_mdn-utils.js should be corrected, what do you think? Lenka
(In reply to lba_2 from comment #17) > Seems that the test_mdn-utils.js should be corrected, what do you think? Yes, please! Honza
Flags: needinfo?(odvarko)
I was wandering if (and what for) do we need the getGAParams() in the get URL functions - e.g. function getNetMonitorTimingsURL() { return `${MDN_URL}Tools/Network_Monitor/request_details${getGAParams()}#Timings`; } //returns https://developer.mozilla.org/docs/Tools/Network_Monitor/request_details?utm_source=mozilla&utm_medium=devtools-netmonitor&utm_campaign=default#Timings and then the link changes into: https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor/request_details In my original fix (the current patch) I had just this string: return `${MDN_URL}Tools/Network_Monitor/request_details#Timings`; (without getGAParams); and that works fine. When trying to figure out what the string "?utm_source=mozilla&utm_medium=devtools-netmonitor&utm_campaign=default" actually does, I tried fixing the incorrect links with getGAParams like in the top example. It works also, but the long link doesn't keep the "#Timings" at the end. I would like to better understand the use of this so when I change the test_mnd-utils.js file I do it correctly (with or without the utm_ params. thanks, Lenka p.s. Also, I was wandering if the test_mdn-utils.js should be in the same patch or separate.
Thanks for the update Lenka! (In reply to lba_2 from comment #19) > I was wandering if (and what for) do we need the getGAParams() in the get > URL functions - e.g. Yes, it's missing in the current patch (sorry for no noticing before) This is used for MDN stats > function getNetMonitorTimingsURL() { > return > `${MDN_URL}Tools/Network_Monitor/request_details${getGAParams()}#Timings`; > } > //returns > https://developer.mozilla.org/docs/Tools/Network_Monitor/ > request_details?utm_source=mozilla&utm_medium=devtools- > netmonitor&utm_campaign=default#Timings > > and then the link changes into: > > https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor/ > request_details That's correct (MDN redirect) > In my original fix (the current patch) I had just this string: > return `${MDN_URL}Tools/Network_Monitor/request_details#Timings`; (without > getGAParams); > and that works fine. Yes, but not stats info for MDN (the GA params needs to be there) > When trying to figure out what the string > "?utm_source=mozilla&utm_medium=devtools-netmonitor&utm_campaign=default" > actually does, I tried fixing the incorrect links with getGAParams like in > the top example. It works also, but the long link doesn't keep the > "#Timings" at the end. > > I would like to better understand the use of this so when I change the > test_mnd-utils.js file I do it correctly (with or without the utm_ params. > > thanks, > Lenka > > p.s. Also, I was wandering if the test_mdn-utils.js should be in the same > patch or separate. All can be in the same patch. Honza
Attached patch bug1499042_v7.patch (obsolete) — Splinter Review
in addition to fixing learn more links for network panel, it also fixes the test file used for checking these links.
Attachment #9018270 - Attachment is obsolete: true
Attachment #9019505 - Flags: review?(odvarko)
Comment on attachment 9019505 [details] [diff] [review] bug1499042_v7.patch Review of attachment 9019505 [details] [diff] [review]: ----------------------------------------------------------------- I am seeing a conflict when applying the patch on latest m-c HEAD applying learn-more.patch patching file devtools/client/netmonitor/test/unit/test_mdn-utils.js Hunk #1 FAILED at 11 1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/test/unit/test_mdn-utils.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh learn-more.patch Honza
Attachment #9019505 - Flags: review?(odvarko)
@Honza, thanks for the help updating m-c head and also applying patch (learnt something new again :-). When I did hg qpush I got the same error like you. Then I ran hg qpush again and got more HUNKs failed Then I ran it one more time and got: patch series fully applied. Without any action in between. So could it be the same when you try to apply the patch repeatedly? Please can you try? Thanks, Lenka
Flags: needinfo?(odvarko)
(In reply to lba_2 from comment #23) > @Honza, > thanks for the help updating m-c head and also applying patch (learnt > something new again :-). > > When I did hg qpush I got the same error like you. > Then I ran hg qpush again and got more HUNKs failed > Then I ran it one more time and got: patch series fully applied. Without any > action in between. > > So could it be the same when you try to apply the patch repeatedly? Please > can you try? > Thanks, > Lenka Because when I looked into .rej files I really didn't see any problem? Any clues for me? :-)
Attached patch learn-more.patch (obsolete) — Splinter Review
Here is what I am seeing when applying the latest version of the patch to latest m-c $ hg qpush applying learn-more.patch patching file devtools/client/netmonitor/test/unit/test_mdn-utils.js Hunk #1 FAILED at 11 1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/test/unit/test_mdn-utils.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh learn-more.patch Here is content of the test_mdn-utils.js.rej --- test_mdn-utils.js +++ test_mdn-utils.js @@ -12,29 +12,33 @@ function run_test() { "&utm_medium=devtools-netmonitor&utm_campaign=default"; const GTM_PARAMS_WC = "?utm_source=mozilla" + "&utm_medium=devtools-webconsole&utm_campaign=default"; const { getHeadersURL, getHTTPStatusCodeURL, getNetMonitorTimingsURL, - getPerformanceAnalysisURL + getPerformanceAnalysisURL, + getFilterBoxURL } = require("devtools/client/netmonitor/src/utils/mdn-utils"); info("Checking for supported headers"); equal(getHeadersURL("Accept"), `${MDN_URL}Web/HTTP/Headers/Accept${GTM_PARAMS_NM}`); info("Checking for unsupported headers"); equal(getHeadersURL("Width"), null); info("Checking for supported status code"); equal(getHTTPStatusCodeURL("200", "webconsole"), `${MDN_URL}Web/HTTP/Status/200${GTM_PARAMS_WC}`); info("Checking for unsupported status code"); equal(getHTTPStatusCodeURL("999", "webconsole"), `${MDN_URL}Web/HTTP/Status${GTM_PARAMS_WC}`); equal(getNetMonitorTimingsURL(), - `${MDN_URL}Tools/Network_Monitor${GTM_PARAMS_NM}#Timings`); + `${MDN_URL}Tools/Network_Monitor/request_details${GTM_PARAMS_NM}#Timings`); equal(getPerformanceAnalysisURL(), - `${MDN_URL}Tools/Network_Monitor${GTM_PARAMS_NM}#Performance_analysis`); + `${MDN_URL}Tools/Network_Monitor/Performance_analysis${GTM_PARAMS_NM}`); + + equal(getFilterBoxURL(), + `${MDN_URL}Tools/Network_Monitor/request_list${GTM_PARAMS_NM}#Filtering_by_properties`); } I am attaching modified version of the patch. Try push shows Eslint errors: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cb86c275c24c050e2fbf6f2a95833055db0d5d4&selectedJob=207780412 TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/netmonitor/src/components/TimingsPanel.js:99:61 | Missing trailing comma. (comma-dangle) TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/netmonitor/src/utils/mdn-utils.js:204:1 | Line 204 exceeds the maximum line length of 90. (max-len) Honza
Flags: needinfo?(odvarko)
I have corrected ES lint failures directly in the learn-more.patch file.
Attachment #9019505 - Attachment is obsolete: true
Attachment #9020307 - Flags: review?(odvarko)
Attachment #9020029 - Attachment is obsolete: true
:Honza, can you please r+ this revision so we can land it? Thank you,
Flags: needinfo?(odvarko)
Keywords: checkin-needed
Attachment #9020307 - Flags: review?(odvarko) → review+
Flags: needinfo?(odvarko)
Pushed by cbrindusan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7e98f8ed12d4 Fix learn more links in the Network panel. r=Honza
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: