1499042 - Fix learn more links in the Network panel

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P2)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 1

[Lenka Pelechova]

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

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Sure, done!
Honza

Comment 3

[Lenka Pelechova]

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)

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 5

[Lenka Pelechova]

(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

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 7

[Lenka Pelechova]

Attachment: bug1499042.patch

Patch - Fixes Learn more links in the Network panel and adds a Tooltip for Learn more in Timings

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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.

Comment 9

[Lenka Pelechova]

Attachment: 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.

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Please, mark the old patches as obsolete (Details  -> Edit Details -> Check Obsolete checkbox)

Honza

Comment 12

[Lenka Pelechova]

Attachment: 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

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/646f2d021c11
"Fix learn more links in the Network panel". r=Honza

Comment 15

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 646f2d021c11 (Bug 1499042) for X failures in devtools/client/netmonitor/test/unit/test_mdn-utils.js

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=6d6b5c10bba56bc286ae66cf9d72948a6a03d200&selectedJob=206605350

https://treeherder.mozilla.org/logviewer.html#?job_id=206605350&repo=mozilla-inbound&lineNumber=7614

Comment 16

[Pulsebot]

Backout by [e-mail]:
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

Comment 17

[Lenka Pelechova]

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

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 19

[Lenka Pelechova]

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.

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 21

[Lenka Pelechova]

Attachment: bug1499042_v7.patch

in addition to fixing learn more links for network panel, it also fixes the test file used for checking these links.

Comment 22

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 23

[Lenka Pelechova]

@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

Comment 24

[Lenka Pelechova]

(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? :-)

Comment 25

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: 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

Comment 26

[Lenka Pelechova]

Attachment: learn-more.patch corrected ESlint failures

I have corrected ES lint failures directly in the learn-more.patch file.

Comment 27

[Jan Honza Odvarko [:Honza] (always need-info? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c90d40922eb60362670a462f3eb7ad50d3bdba7

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try is green, this is ready to land!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf449df25c3eb3b17e0e9fcffe98eacd24bd099f

Honza

Comment 29

[Cristian Brindusan [:cbrindusan]]

:Honza, can you please r+ this revision so we can land it?
Thank you,

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e98f8ed12d4
Fix learn more links in the Network panel. r=Honza

Comment 31

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/7e98f8ed12d4