1518031 - Add to addressbar in the Page Actions menu gives a blank icon when you pin "Add Search Engine"

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect, P2)

Description

[Asa Dotzler [:asa]]

Tested on latest x86 Nightly build on Windows 10

If you try to pin "Add Search Engine" to the addressbar from the Page Actions menu, you get a blank icon. It functions but it's blank. 

Steps to reproduce
1. Visit ecosia.com or any other page with a search engine you haven't installed (Bugzilla works too)
2. Click the Page Actions menu
3. Right click on "Add Search Engine" and select "Add to Addressbar"

Results: It gets added to the addressbar with no icon

Expected: We get some kind of icon in the addressbar.

Comment 1

[Mark Banner (:standard8)]

WFM on Mac (2019-01-06) - I get the favicon of the Ecosia page.

Comment 2

[Marco Bonardo [:mak]]

It works when I do it on the page having the search engine, if then I remove the added search engine and reload the page with the engine, I indeed get an empty space, as reported. Switching tab makes the icon appear. Something fishy.
Drew may know more about how this icon is set, maybe it's a timing problem, maybe the icon src is added when the button is not yet visible and we don't paint it.

Comment 3

[Drew Willcoxon :adw]

The icon is set here: https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/browser/base/content/browser-pageActions.js#1124

It looks like this.engines[0].icon is null when _updateTitleAndIcon is called via updateOpenSearchBadge, at least sometimes. The stack I'm seeing when it's null is:

_updateTitleAndIcon@chrome://browser/content/browser-pageActions.js:1139:11
updateEngines@chrome://browser/content/browser-pageActions.js:1128:7
updateOpenSearchBadge@chrome://browser/content/browser.js:3966:5
addEngine@chrome://browser/content/browser.js:3956:9
addSearch@chrome://browser/content/browser.js:3745:5
receiveMessage@chrome://browser/content/browser.js:3675:9
MessageListener.receiveMessageinit@chrome://browser/content/browser.js:3650:5
onLoad@chrome://browser/content/browser.js:1325:5
EventHandlerNonNull@chrome://browser/content/browser.xul:108:19

It's easy to reproduce: Visit a site that offers an engine, add the "Add Search Engine" action to the urlbar, and reload the page. That triggers the stack above.

It's possible this has always been a problem, but it might be a regression, I don't know.

It looks like the icon comes from browser.mIconURL: https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/browser/base/content/browser.js#3977 So we'd need to somehow wait until that becomes non-null and then set the action's icon.

This isn't a problem for the search bar because the icon is only shown once the user opens the search bar menu, and by that time the icon is likely non-null.

Moving this to Toolbars, where page action bugs live.

Comment 4

[Drew Willcoxon :adw]

Attachment: Bug 1518031 - Update "Add Search Engine" page action image when search engine icon becomes available.

We can just add `onLinkIconAvailable` to `TabsProgressListener` in browser.js and update the search badge.

Comment 5

[Dave Townsend [:mossop]]

This is likely another regression from bug 1453751. Previously the url for the favicon was sent to the browser very early in page load. Now we wait until we've actually loaded the icon's content which is probably after any search engines are detected.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8dbf2f4f7c53
Update "Add Search Engine" page action image when search engine icon becomes available. r=mossop

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8dbf2f4f7c53