Fix learn more links in the Network panel

RESOLVED FIXED in Firefox 65

Status

P2
normal
RESOLVED FIXED
a month ago
20 days ago

People

(Reporter: Honza, Assigned: lba_2, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 65
good-first-bug

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: good-first-bug)

Attachments

(1 attachment, 5 obsolete attachments)

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
(Assignee)

Comment 1

a month ago
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)
(Assignee)

Comment 3

a month ago
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
(Assignee)

Comment 5

a month ago
(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)
(Assignee)

Comment 7

a month ago
Created attachment 9017801 [details] [diff] [review]
bug1499042.patch

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)
(Assignee)

Comment 9

a month ago
Created attachment 9018064 [details] [diff] [review]
bug1499042_v4.patch

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
(Assignee)

Updated

a month ago
Attachment #9017801 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #9018064 - Attachment is obsolete: true
(Assignee)

Comment 12

a month ago
Created attachment 9018270 [details] [diff] [review]
bug1499042_v5.patch

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+
Keywords: checkin-needed

Comment 14

a month ago
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

Comment 16

a month ago
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
(Assignee)

Comment 17

28 days ago
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)
(Assignee)

Comment 19

27 days ago
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
(Assignee)

Comment 21

26 days ago
Created attachment 9019505 [details] [diff] [review]
bug1499042_v7.patch

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)
(Assignee)

Comment 23

25 days ago
@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)
(Assignee)

Comment 24

25 days ago
(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? :-)
Created attachment 9020029 [details] [diff] [review]
learn-more.patch

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)
(Assignee)

Comment 26

23 days ago
Created attachment 9020307 [details] [diff] [review]
learn-more.patch corrected ESlint failures

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)
Keywords: checkin-needed

Comment 30

20 days ago
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

Comment 31

20 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7e98f8ed12d4
Status: ASSIGNED → RESOLVED
Last Resolved: 20 days ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
You need to log in before you can comment on or make changes to this bug.