872078 - Display preview for fonts in Response tab

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[Victor Porof [:vporof][:vp]]

When inspecting the content of a font/* response, the source editor is basically populated with garbled text. We can do better than this.

Comment 1

[Victor Porof [:vporof][:vp]]

Paul, would it be possible to use the Font panel here? I think it'd be a great idea to make that a standalone view.

Comment 2

[Paul Rouget [:paul]]

Indeed. We need a font panel v2 that would take this use case into account, and also implement what's mentioned in bug 886041 comment 1.

Comment 3

[Sebastian Zartner [:sebo]]

Attachment: Bug 872078 - Allow multi-line font previews. r=jdescottes

This change prepares the font preview generation for the display within the Network Monitor's Response view.
In there, the preview is meant to contain all the letters of the Latin alphabet in upper and lower case plus the numbers from 0 to 9 and different characters in other alphabets.
As those are too long to be displayed in a single line, they will be split into several ones.

For this to work, the getFontPreviewData() method now splits the given preview text at new-line characters into several lines.
This also means that for the existing single-line preview texts within the Inspector there is no change.

Comment 4

[Sebastian Zartner [:sebo]]

Attachment: Bug 872078 - Added preview and info for fonts in Response view of Netmonitor. r=bomsy

A preview for fonts is added to the Response view of the Netmonitor. This view displays the name and MIME type of the font and generates a preview for it.

If no preview can be created, a hint is shown to the user. This is the case when the font isn't used within the page when generating the preview. For example, this is the case when loading the font via the Font Loading API.

Depends on D110167

Comment 5

[Sebastian Zartner [:sebo]]

Attachment: Bug 872078 - Added test case for font preview in Response view of Netmonitor. r=bomsy,jdescottes

Depends on D110169

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4cb63632679a
Allow multi-line font previews. r=jdescottes

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4cb63632679a

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f73193dcd637
Added preview and info for fonts in Response view of Netmonitor. r=bomsy
https://hg.mozilla.org/integration/autoland/rev/500850494fa9
Added test case for font preview in Response view of Netmonitor. r=bomsy

Comment 9

[Sebastian Zartner [:sebo]]

Looks like I've done a mistake when pushing this to lando. Only the first commit was landed, though the main ones not.
Therefore I've pushed them now and reopened this bug until those other two commits merged into mozilla-central.

Sebastian

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/420c8b935a0c
Backed out 2 changesets for causing dt failures in browser_net_fonts.js

Comment 11

[Alexandru Michis [:malexandru]]

Backed out 2 changesets (Bug 872078) for causing dt failures in browser_net_fonts.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/420c8b935a0c
Push with failures, failure log.

Comment 12

[Sebastian Zartner [:sebo]]

Bomsy, I need your help with this. In my previous tests this worked fine. On Treeherder there's a timeout.
So I fetched the changesets locally. Now I also get a timeout locally, though seemingly caused by another reason. The fonts are rejected by the sanitizer:

0:15.70 INFO Console message: [JavaScript Error: "downloadable font: rejected by sanitizer (font-family: "test-font" style:normal weight:400 stretch:100 src index:0) source: http://example.com/browser/devtools/client/netmonitor/test/test-font.woff2"]
0:15.71 INFO Console message: [JavaScript Error: "downloadable font: rejected by sanitizer (font-family: "test-font-bold" style:normal weight:700 stretch:100 src index:0) source: http://example.com/browser/devtools/client/netmonitor/test/test-font-bold.woff"]

And looking at the UI the previews are not generated. So I've added await pushPref("gfx.downloadable_fonts.sanitize", false); before anything else, though that didn't help getting the fonts loaded. I've also tried using the HTTPS url instead of the HTTP one, without success.

What do I need to change to get the fonts loaded? And it looks like in Treeherder they do get loaded, though the generation of the second preview fails.

Sebastian

Comment 13

[Hubert Boma Manilla (:bomsy)]

Hi Sebastian,

What do I need to change to get the fonts loaded? And it looks like in Treeherder they do get loaded, though the generation of the second preview fails.

It looks like the issue is with the font you are trying to load. The platform tries to sanitize it and fails, therefore it does not load the font.
Maybe you can change it to something more consistent, that would work both locally and on Treeherder. I tried these fonts https://searchfox.org/mozilla-central/search?q=ostrich-regular.ttf&path=&case=false&regexp=false and https://searchfox.org/mozilla-central/search?q=ostrich-black.ttf&path= and they seems to work. Here are some interesting details on sanitizing https://chromium.googlesource.com/external/github.com/khaledhosny/ots/+/refs/heads/master/docs/DesignDoc.md

Also the second issue is clickElement before the second might not be hitting the request element but instead a splitter

@@ -56,7 +56,7 @@ add_task(async function() {
   );
 
   // Check second font request
-  clickElement(requests[2], monitor);
+  clickElement(requests[2].querySelector(".requests-list-status"), monitor);
 
   await waitForDOM(document, "#response-panel .response-font[src^='data:']");
 
@@ -69,7 +69,7 @@ add_task(async function() {

I tried the above to be more specific and it works, You might do that or something better.

Comment 14

[Sebastian Zartner [:sebo]]

Thanks you very much for the detailed description! I could get the test to run again locally with your hints. So I pushed the changes to the test again to https://phabricator.services.mozilla.com/D110170. Would be fantastic if you had a quick look if everything's fine with the test now.

Sebastian

Comment 15

[Hubert Boma Manilla (:bomsy)]

(In reply to Sebastian Zartner [:sebo] from comment #14)

Thanks you very much for the detailed description! I could get the test to run again locally with your hints. So I pushed the changes to the test again to https://phabricator.services.mozilla.com/D110170. Would be fantastic if you had a quick look if everything's fine with the test now.

Sebastian

Sure thing! Thanks to Julian as well for helping figure out the click issue. I've had a look at the patch and it looks good. You might need a rebase to make sure not conflicts. Thanks for working on this.

Comment 16

[Sebastian Zartner [:sebo]]

Ok, so also a big thank you to Julian, then! :-)

I did the rebase and requested landing the two changesets now. Let's hope without any issues this time!

Sebastian

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3714c43d8bc9
Added preview and info for fonts in Response view of Netmonitor. r=bomsy
https://hg.mozilla.org/integration/autoland/rev/49e8b5ddbc77
Added test case for font preview in Response view of Netmonitor. r=bomsy

Comment 18

[Narcis Beleuzu [:NarcisB]]

Backed out for dt failures on browser_net_fonts.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/a4c48c7afe975d9fd83fa2bb43cede8bb4bd6926
Log link: https://treeherder.mozilla.org/logviewer?job_id=336531884&repo=autoland&lineNumber=5244

Comment 19

[Sebastian Zartner [:sebo]]

Again? :-( Looks like the MIME type of the fonts is interpreted differently on Treeherder than on my machine. I'll change it so it works there.

Thanks for having a look at it, Narcis!

Sebastian

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5728965f8788
Added preview and info for fonts in Response view of Netmonitor. r=bomsy
https://hg.mozilla.org/integration/autoland/rev/7f10ba2647c1
Added test case for font preview in Response view of Netmonitor. r=bomsy

Comment 21

[Atila Butkovits]

Backed out for failures at browser_net_fonts.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/6fa1583237a2721cb04a7ae90303b7628280ff59

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&revision=7f10ba2647c160428cdda561f86c905448cdd546&searchStr=linux%2C18.04%2Cx64%2Cdebug%2Cmochitests%2Ctest-linux1804-64%2Fdebug-mochitest-devtools-chrome-e10s%2Cdt9&selectedTaskRun=cYANVrobQriwAycKuivCiQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=336644123&repo=autoland&lineNumber=11619

Comment 22

[Sebastian Zartner [:sebo]]

Thank you for the note, Atila!

Unfortunately, I have no idea how to reproduce those errors to be able to fix them. On my Windows machine all tests pass.
Atila, Bomsy, Julian, can anyone of you provide a hint for what's going on here and how to get rid of the test failures? I'd really love to finish this up though I'm slowly becoming desperate regarding the test case.

Sebastian

Comment 23

[Julian Descottes [:jdescottes]]

To reproduce leak failures, you need to build with debug mode enabled.
Add ac_add_options --enable-debug in your mozconfig.
Then you should reproduce the failure by running the test.

I'll try to bisect to identify what is leaking.

If you are not doing it already, you can push to try before landing, especially when adding new tests.
You can use ./mach try --preset devtools, it will run all devtools related tests on all platforms, including debug builds.

Comment 24

[Julian Descottes [:jdescottes]]

Found it, there is a componentDidUnmount in the new FontPreview.js component. This is not a real lifecycle method, it should be componentWillUnmount. Fixing this will fix the leak :) (very common mistake, we've made it a lot of times before)

I'll file a bug to add componentDidUnmount to our linter.

Comment 25

[Sebastian Zartner [:sebo]]

Thank you for the very fast response and the very useful info, Julian! I'll fix it right away. I'm still not enough into React so that I didn't realize this mistake. (Frankly, I think I copied the function name from somewhere else in the code. :-) )

I'll also add the mozconfig option and test my changes with mach try from now on before pushing them.

Sebastian

Comment 26

[Sebastian Zartner [:sebo]]

I could reproduce the error now. So merci beaucoup again, Julian! :-)

I've fixed it now and just requested to land it again.

Sebastian

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15379790faad
Added preview and info for fonts in Response view of Netmonitor. r=bomsy
https://hg.mozilla.org/integration/autoland/rev/bec9c2c1a4cf
Added test case for font preview in Response view of Netmonitor. r=bomsy

Comment 28

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on browser_net_fonts.js

backout: https://hg.mozilla.org/integration/autoland/rev/d405cffb5f56140845fb9f69448f0f3f576a1d23

push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=bec9c2c1a4cf9fa027b86bc026f8ce02e82218ae&selectedTaskRun=LjOfDdtwRWK60GMFiK-vSQ.0

failure log: https://treeherder.mozilla.org/logviewer?job_id=336718549&repo=autoland&lineNumber=2750

[task 2021-04-15T23:01:50.927Z] 23:01:50 INFO - Console message: [JavaScript Error: "downloadable font: rejected by sanitizer (font-family: "test-font-bold" style:normal weight:700 stretch:100 src index:0) source: http://example.com/browser/devtools/client/netmonitor/test/ostrich-black.ttf"]
[task 2021-04-15T23:01:50.927Z] 23:01:50 INFO - > Network event progress: NetworkEvent: 3/3, PayloadReady: 1/3, EventTimings: 1/3, got NetMonitor:NetworkEventUpdated:EventTimings for server0.conn66.netEvent64
[task 2021-04-15T23:01:50.927Z] 23:01:50 INFO - > Network event progress: NetworkEvent: 3/3, PayloadReady: 2/3, EventTimings: 1/3, got NetMonitor:PayloadReady for server0.conn66.netEvent83
[task 2021-04-15T23:01:50.928Z] 23:01:50 INFO - > Network event progress: NetworkEvent: 3/3, PayloadReady: 3/3, EventTimings: 1/3, got NetMonitor:PayloadReady for server0.conn66.netEvent101
[task 2021-04-15T23:01:50.928Z] 23:01:50 INFO - > Network event progress: NetworkEvent: 3/3, PayloadReady: 3/3, EventTimings: 2/3, got NetMonitor:NetworkEventUpdated:EventTimings for server0.conn66.netEvent83
[task 2021-04-15T23:01:50.928Z] 23:01:50 INFO - > Network event progress: NetworkEvent: 3/3, PayloadReady: 3/3, EventTimings: 3/3, got NetMonitor:NetworkEventUpdated:EventTimings for server0.conn66.netEvent101
[task 2021-04-15T23:01:50.928Z] 23:01:50 INFO - Buffered messages finished
[task 2021-04-15T23:01:50.928Z] 23:01:50 INFO - TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_fonts.js | Test timed out -
[task 2021-04-15T23:01:50.970Z] 23:01:50 INFO - Removing tab.
[task 2021-04-15T23:01:50.970Z] 23:01:50 INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2021-04-15T23:01:50.986Z] 23:01:50 INFO - Got event: 'TabClose' on [object XULElement].
[task 2021-04-15T23:01:51.006Z] 23:01:51 INFO - Tab removed and finished closing
[task 2021-04-15T23:01:51.045Z] 23:01:51 INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_fonts.js | The main process DevToolsServer has no pending connection when the test ends -

Comment 29

[Julian Descottes [:jdescottes]]

As mentioned on elements, for some reason the two ttf files are empty in the last revision we pushed.
Since we are in code freeze, we should probably wait for next Tuesday to land this again. In the meantime let's update the diff on Phabricator, and check that try is green. Sorry the test has been such a pain to work with, it happens sometimes :(

Comment 30

[Sebastian Zartner [:sebo]]

This is definitely not my best work. 🤦‍♂️ Not sure how the fonts could get deleted by the update. I just wanted to get this landed before the code freeze and didn't realize that something went wrong when pushing the lastest changes. Sorry for the extra work you have to do because of that!

I'll update the diff over the weekend and push the changes to Try. Once you or Bomsy reviewed them, everything's green on Try, and the code freeze is over I'll try to land them again. I keep the needinfo until I have the changes done.

Sebastian

Comment 31

[Julian Descottes [:jdescottes]]

Thanks Sebo and thanks again for your contributions. Your plan sounds good, and don't worry about us we are just helping you push this, you're still doing all the actual work ;)

Comment 32

[Sebastian Zartner [:sebo]]

As written in the chat, I pushed a corrected changeset with restored font files. Could you please push them to Try, Julian?

Sebastian

Comment 33

[Julian Descottes [:jdescottes]]

Here's a try push with the "devtools" preset: https://treeherder.mozilla.org/jobs?repo=try&revision=6f4e03c134224a0f4f56777984b82fc4491e3f02

Comment 34

[Julian Descottes [:jdescottes]]

Looks like we still have the timeout issue related to the second clickElement call.
Can we add the suggestion from Bomsy in comment 13?

Comment 35

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #34)

Looks like we still have the timeout issue related to the second clickElement call.
Can we add the suggestion from Bomsy in comment 13?

As discussed on Elements, this is not the problem here. We have an issue with the second fonts file (ostrich-black.ttf), which is 15kB in the current patch, whereas the first version was 12kB (which also matches the file we currently have in the inspector).

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ceee4a44c668
Added preview and info for fonts in Response view of Netmonitor. r=bomsy
https://hg.mozilla.org/integration/autoland/rev/c37cd0b9dc82
Added test case for font preview in Response view of Netmonitor. r=bomsy,jdescottes

Comment 37

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/ceee4a44c668
https://hg.mozilla.org/mozilla-central/rev/c37cd0b9dc82

Comment 38

[Sebastian Zartner [:sebo]]

Yay, finally!

Sebastian

Comment 39

[Sebastian Zartner [:sebo]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Fonts were only displayed as binary data in the Netmonitor before. Now there's a nice preview.
[Affects Firefox for Android]: no
[Suggested wording]: There's now a preview shown for web fonts in the Network Monitor.
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor/request_details#response_tab (See https://github.com/mdn/content/pull/5516)