1789042 - [foxfooding] Firefox View doesn't always show favicons for Recently Closed Tabs list

Status: RESOLVED FIXED

(Firefox :: Firefox View, defect, P2)

Description

[Peter deHaan [:pdehaan]]

Attachment: zoom no zoom.jpeg

(Proxy filing for somebody else)

the website icon doesn't always appear in the recently closed list. i see globe icons for some zoom closed tabs but further down there is the zoom icon

Comment 1

[:Gijs (he/him)]

Marco, could you take a look at the implementation here (called from here) and let us know if there is something we could do differently to improve the situation? Would using page-icon protocol or something else make things better, and/or do you know why some tabs would have icons and others... not?

Comment 2

[Marco Bonardo [:mak]]

getImageURL was not really necessary, there is PlacesUtils.favicons.getFaviconLinkForIcon() doing the same thing. Using page-icon may have better results, because moz-anno:favicon:favicon:iconURL is requesting a very specific icon uri to be present in the favicons database... using page-icon the system will return what it thinks may be a good icon for that page, including potentially using the root icon if it was stored, or whatever it has at hand.
In short, page-icon will be more lenient than asking for a specific favicon url. That said, it's still querying an offline cache, that may or may not have the icon shown on the tab (in certain cases it's not a given).

I'm also not sure why this has an explicit fallback on chrome://global/skin/icons/defaultFavicon.svg when both the favicon protocols should already do that.
I think this code should check if image is a local resource (data, chrome, resource) if so just use it, otherwise fallback to page-icon.

Now, loooking with the inspector at the page, I think the favicon service here is not even involved, all the images I see are data: uri, and the missing ones point to the default icon, that means createFaviconElement() was invoked with an null image.
Indeed if I invoke SessionStore.getClosedTabData(window) in my console I see that zoom pages have an image: null property... why? I'm not sure but it has something to do with Session Restore.

To sum up, one should investigate why Session Restore is storing a null image for certain pages (zoom meetings for example), and then I'd change the code to check for local icon or fallback to page-icon.

Comment 3

[Ray Fambro]

Shifting back to a P2. Even though this is more of an aesthetic concern that doesn't affect functionality, I feel that this kind of detail gives a more polished perception for this feature and could increase trust.

Comment 4

[Sarah Clements [:sclements]]

getImageURL was not really necessary, there is PlacesUtils.favicons.getFaviconLinkForIcon() doing the same thing. Using page-icon may have better results, because moz-anno:favicon:favicon:iconURL is requesting a very specific icon uri to be present in the favicons database... using page-icon the system will return what it thinks may be a good icon for that page, including potentially using the root icon if it was stored, or whatever it has at hand.

Thanks for the comments on this Marco, but I think there's a few issues being confused/conflated here... first is whether Firefox View should be prepending page-icon instead of moz-anno:favicon to an image URL that starts with https. And the second is why the SessionStore returns image: null for some entries.

Regarding the first issue, when I asked around about this months ago in the Firefox Desktop Development matrix channel, I wasn't told to not use moz-anno (and there's no real documentation that I can see about this and the Favicon service - perhaps a comment should be added here to discourage people from using it if its outdated or will be deprecated?).

As far as I can see, moz-anno:favicon works, but I can change it if page-icon is better to use. But to clarify, the local resource is used which looks to be primarily data, per this helper. As to why the default favicon is provided by a fallback in that same helper is because not doing so results in no favicon whatsoever being shown when SessionStore returns image:null.

Regarding why SessionStore returns an image string for one zoom entry and not for others (per Peter's screenshot) is a mystery. I can file a bug for it, but curiously I haven't been able to replicate that scenario.

But, I don't understand how using PlacesUtils.favicons.getFaviconLinkForIcon() would help here. From what I can see here its only useful if you have an icon, not null. Perhaps what we can do when we have image: null is what UrlbarUtils.getIconForUrl(tab.url) in that block of code is doing, which is to grab the url and prepend page-icon: to it.

To sum up, one should investigate why Session Restore is storing a null image for certain pages (zoom meetings for example), and then I'd change the code to check for local icon or fallback to page-icon.

Marco, I can file a bug about the Session Store issue. But you mentioned Session Restore... is this different from Session Store? :)

Comment 5

[Sarah Clements [:sclements]]

Thinking about this a bit more ... I guess a follow up question for this is what is the expected behavior of SessionStore? I'd think that if its capable of supplying a url where it doesn't have a local resource (for example, for firefox accounts I see a url), it should be able to do that for all entries where it doesn't have a local resource. Failing that, it should return the default favicon.

I can do what Marco suggested and investigate whats happening with Session Store, but if it looks to be complicated I'd probably try to do the workaround I mentioned in my previous comment, since we want a patch uplifted to 107 beta.

Marco, you mentioned the favicon protocols and that you seemed surprised that the favicon service is not involved at all... Is it supposed to be used by Session Store?

Comment 6

[Sarah Clements [:sclements]]

Attachment: Screenshot 2022-10-20 at 12.47.12.png

I'm not able to reproduce this, so I'm curious if its still an issue.

Comment 7

[Marco Bonardo [:mak]]

(In reply to Sarah Clements [:sclements] from comment #4)

Regarding the first issue, when I asked around about this months ago in the Firefox Desktop Development matrix channel, I wasn't told to not use moz-anno

There's no reason to not use it, it's just that it may not be fit for the use case. For certain cases it's ok to use it, for other cases it's ok to use page-icon, for others it's something else.
Yes, of course it would be nice to have better documentation.

As far as I can see, moz-anno:favicon works, but I can change it if page-icon is better to use.

One returns a specific icon url, if it exists, the other one returns what it think is the best icon for a page url, if it exists. They are just different.

Regarding why SessionStore returns an image string for one zoom entry and not for others (per Peter's screenshot) is a mystery. I can file a bug for it, but curiously I haven't been able to replicate that scenario.

Yes, that's the main problem that I found here, if session store tells you the icon is null... not much you can do if you exactly want THAT favicon that was on the tab.

But, I don't understand how using PlacesUtils.favicons.getFaviconLinkForIcon() would help here.

It doesn't, I said that the getImageURL() helper that was added is not particularly useful because getFaviconLinkForIcon is pretty much doing the same. It would just be matter of null checking the image first.

To sum up, one should investigate why Session Restore is storing a null image for certain pages (zoom meetings for example), and then I'd change the code to check for local icon or fallback to page-icon.

Marco, I can file a bug about the Session Store issue. But you mentioned Session Restore... is this different from Session Store? :)

It's the same, Session Restore is how we used to call the feature for many years, but the component in the source code is called sessionstore.

Here you pretty much always get a data-uri icon or null, I can't see a case where you get https or other... I'd change the checks order so first of all check if you have a data uri, in that case just let it go through. For other cases you could fallback to page-icon:page_url.

If you want to use moz-anno:favicon as a fallback instead (because you want to exactly use the icon that was on the tab), I'd suggest to modify it so moz-anno:favicon:null returns the default icon, so it's simpler to just pass the icon uri to it and you'll always get an icon (the code is likely failing here https://searchfox.org/mozilla-central/rev/bfa4dfd40b8fe6f1a6504e86b5ad2c5fd402bfc0/toolkit/components/places/nsAnnoProtocolHandler.cpp#287-288 and not creating a channel).

Or if you want to handle each case, data uri first, then null check, if null use page-icon, otherwise use getFaviconLinkForIcon... it just seems a but overkill, but it would work.

Comment 8

[Sarah Clements [:sclements]]

(In reply to Marco Bonardo [:mak] from comment #7)

Here you pretty much always get a data-uri icon or null, I can't see a case where you get https or other... I'd change the checks order so first of all check if you have a data uri, in that case just let it go through. For other cases you could fallback to page-icon:page_url.

Yup, this is what I was suggesting in comment 4. This sounds the simplest but what happens if an icon can't be found with page-icon? Does it return a default favicon or null?

If you want to use moz-anno:favicon as a fallback instead (because you want to exactly use the icon that was on the tab), I'd suggest to modify it so moz-anno:favicon:null returns the default icon, so it's simpler to just pass the icon uri to it and you'll always get an icon (the code is likely failing here https://searchfox.org/mozilla-central/rev/bfa4dfd40b8fe6f1a6504e86b5ad2c5fd402bfc0/toolkit/components/places/nsAnnoProtocolHandler.cpp#287-288 and not creating a channel).

Or if you want to handle each case, data uri first, then null check, if null use page-icon, otherwise use getFaviconLinkForIcon... it just seems a but overkill, but it would work.

I'm not attached to any particular solution, I'm just trying to work out what is the best solution and whether this is even a common problem. From what I can see, the reason that SessionStore sometimes stores image: null is because of gBrowser.getIcon in SessionStore.maybeSaveClosedTab. That in turn checks for mIconURL on the browser (set in setIcon). There's no null check for setting that property, only for setting an image atrribute with the mIconURL so I think that's how we end up saving image: null.

I'd have to dig into it more and see if that's something that should be tinkered with in setIcon, but probably the more straightforward solution would be to change it in SessionStore.maybeSaveLastTab to aWindow.gBrowser.getIcon(aTab) ||page-icon:aTab.url. If there's some reason that shouldn't be done in SessionStore, I can just move it to Firefox View (and remove any use ofmoz-anno:favicon`).

Comment 9

[Ray Fambro]

(In reply to Sarah Clements [:sclements] from comment #6)

Created attachment 9299468 [details]
Screenshot 2022-10-20 at 12.47.12.png

I'm not able to reproduce this, so I'm curious if its still an issue.

Looking in the original screenshot that was filed...it seems like the favicons are missing from the Tab Pickup cards.

Comment 10

[Marco Bonardo [:mak]]

(In reply to Sarah Clements [:sclements] from comment #8)

Yup, this is what I was suggesting in comment 4. This sounds the simplest but what happens if an icon can't be found with page-icon? Does it return a default favicon or null?

default favicon

Comment 11

[Sarah Clements [:sclements]]

(In reply to Ray Fambro from comment #9)

(In reply to Sarah Clements [:sclements] from comment #6)

Created attachment 9299468 [details]
Screenshot 2022-10-20 at 12.47.12.png

I'm not able to reproduce this, so I'm curious if its still an issue.

Looking in the original screenshot that was filed...it seems like the favicons are missing from the Tab Pickup cards.

That is true, but Peter specifically mentioned the recently closed tabs list and how there was a favicon for one zoom entry but not others in that list. I think there was a specific iOS issue about the images that was dealt with already by that team (that would've addressed the missing favicons for the Tab Pickup section).

Comment 12

[Sarah Clements [:sclements]]

Attachment: Bug 1789042 - Prepend page-icon to url if image is null in Recently Closed Tabs list r=mak

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de765cbe7291
Prepend page-icon to url if image is null in Recently Closed Tabs list r=mak

Comment 14

[Sandor Molnar]

https://hg.mozilla.org/mozilla-central/rev/de765cbe7291

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit auto_nag documentation.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:sclements, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox107 to wontfix.

For more information, please visit auto_nag documentation.