1921778 - Consider saving the favicons of the top level PageInformation we capture for the profiler pages array

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P2)

Description

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

In the profiler frontend, we have a very naive way to find the favicons, which is just appending favicon.ico to a origin and then hope that it will be present in the server. But that's not always the case. Also whenever we are opening a profile we always ping the favicon urls to get the images, which is not ideal.

Inside Firefox, we already get the favicon, so we should be able to either query the engine to get this information, or add an profiler call while we are extracting this information to save this as well.

This will also help the profiler tab selector that I'm working on implementing, as we will always have proper favicons, and will not have missing ones when the icon is not in the usual /favicon.ico url.

Comment 1

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

(sent mistakenly)

Comment 2

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: wip-favicon.patch

Here's a WIP patch that I spent some time on yesterday. But it's terrible because:

1. It gets the favicon while we are loading the website.
2. It tries to get a favicon every time we find one, even if the profiler is not active.
3. It sends the favicon from parent to the child process via IPC so we can add it to the pages array.
4. It's still getting the URL instead of image data (but it's an easy fix).

And here's some findings I have and what path should we take:

Currently this patch is using a naive approach of calling a profiler function whenever we are adding the favicon to the places DB. Instead we should query the DB directly at the profile capture time. We have a nsIFaviconDataCallback and nsIFaviconService to get favicons easily, and here's an example code that uses them: 1, 2, 3. I tired implementing this approach but it failed because I was trying to get this info from the child process, but the DB is stored in the parent.

So instead of this approach, at the end of the profiling session, child processes can let the parent know about all the favicon it needs, and then the parent process can get these favicons using nsIFaviconService, and add it to the parent profile metadata.

A second approach would be to request the favicons from frontend with web channel during the first profile load. This is probably easier, but I'm not so sure if it's the best approach.

Comment 3

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1921778 - Format the typescript types files r?julienw!

Comment 4

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1921778 - Add a web channel request for returning the favicons to the profiler frontend r?julienw!

Comment 5

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1921778 - Add a test for favicon webchannel request r?julienw!

Comment 6

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1921778 - Remove the unneeded https_first_disabled=true from the profiler tests manifest r?julienw

Previously we needed these because we were using http for the webchannel tests.
After Bug 1917892 we started using https in all of the webchannel tests, so we
don't need them anymore.

---

wip

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7389ddd073e8
Format the typescript types files r=julienw,profiler-reviewers
https://hg.mozilla.org/integration/autoland/rev/8239a42b41fa
Add a web channel request for returning the favicons to the profiler frontend r=julienw,profiler-reviewers
https://hg.mozilla.org/integration/autoland/rev/ec57965323cf
Add a test for favicon webchannel request r=julienw,profiler-reviewers
https://hg.mozilla.org/integration/autoland/rev/24c6930a1b19
Remove the unneeded https_first_disabled=true from the profiler tests manifest r=julienw,profiler-reviewers

Comment 8

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/7389ddd073e8
https://hg.mozilla.org/mozilla-central/rev/8239a42b41fa
https://hg.mozilla.org/mozilla-central/rev/ec57965323cf
https://hg.mozilla.org/mozilla-central/rev/24c6930a1b19