Closed Bug 872078 Opened 8 years ago Closed 2 months ago

Display preview for fonts in Response tab

Categories

(DevTools :: Netmonitor, enhancement, P3)

enhancement

Tracking

(relnote-firefox ?, firefox89 wontfix, firefox90 fixed)

RESOLVED FIXED
90 Branch
Tracking Status
relnote-firefox --- ?
firefox89 --- wontfix
firefox90 --- fixed

People

(Reporter: vporof, Assigned: sebo)

References

(Blocks 3 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(3 files)

When inspecting the content of a font/* response, the source editor is basically populated with garbled text. We can do better than this.
Priority: -- → P3
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.
Flags: needinfo?(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.
Flags: needinfo?(paul)
See Also: → 886041
Severity: normal → enhancement
Product: Firefox → DevTools

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.

Assignee: nobody → sebastianzartner
Status: NEW → ASSIGNED

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

Depends on: 1703967
Pushed by sebastianzartner@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4cb63632679a
Allow multi-line font previews. r=jdescottes
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch
Pushed by sebastianzartner@gmail.com:
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

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backout by malexandru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/420c8b935a0c
Backed out 2 changesets for causing dt failures in browser_net_fonts.js

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.

Flags: needinfo?(sebastianzartner)

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

Flags: needinfo?(sebastianzartner) → needinfo?(hmanilla)

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.

Flags: needinfo?(hmanilla)

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

Flags: needinfo?(hmanilla)

(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.

Flags: needinfo?(hmanilla)

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

Pushed by sebastianzartner@gmail.com:
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

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

Flags: needinfo?(sebastianzartner)
Pushed by sebastianzartner@gmail.com:
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

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

Flags: needinfo?(sebastianzartner)
Flags: needinfo?(jdescottes)
Flags: needinfo?(hmanilla)
Flags: needinfo?(abutkovits)

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.

Flags: needinfo?(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.

Flags: needinfo?(sebastianzartner)
Flags: needinfo?(hmanilla)
Flags: needinfo?(abutkovits)

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

Flags: needinfo?(sebastianzartner)

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

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

Sebastian

Pushed by sebastianzartner@gmail.com:
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

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 -

Flags: needinfo?(sebastianzartner)

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

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

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 ;)

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

Sebastian

Flags: needinfo?(sebastianzartner) → needinfo?(jdescottes)
Flags: needinfo?(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?

Flags: needinfo?(sebastianzartner)

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

Attachment #9212350 - Attachment description: Bug 872078 - Added test case for font preview in Response view of Netmonitor. r=bomsy → Bug 872078 - Added test case for font preview in Response view of Netmonitor. r=bomsy,jdescottes
Attachment #9212350 - Attachment description: Bug 872078 - Added test case for font preview in Response view of Netmonitor. r=bomsy,jdescottes → Bug 872078 - Added test case for font preview in Response view of Netmonitor. r=bomsy
Attachment #9212350 - Attachment description: Bug 872078 - Added test case for font preview in Response view of Netmonitor. r=bomsy → Bug 872078 - Added test case for font preview in Response view of Netmonitor. r=bomsy,jdescottes
Pushed by sebastianzartner@gmail.com:
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
Flags: needinfo?(sebastianzartner)
Status: REOPENED → RESOLVED
Closed: 2 months ago2 months ago
Resolution: --- → FIXED

Yay, finally!

Sebastian

Flags: in-testsuite+
Target Milestone: 89 Branch → 90 Branch
Summary: Some special treatment should be given to fonts in the Response tab → Display preview for fonts in Response tab
Keywords: dev-doc-needed

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)

relnote-firefox: --- → ?
You need to log in before you can comment on or make changes to this bug.