1571110 - Change chrome_settings_overrides.search_provider.favicon_url from "url" to "relativeUrl"

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P3)

Description

[Rob Wu [:robwu]]

chrome_settings_overrides.search_provider.favicon_url is currently of type "url", which means that it should be an absolute URL that can be loaded by extensions: https://searchfox.org/mozilla-central/rev/b3fd653bc6078b3be4a8d06db39eddc5714755da/browser/components/extensions/schemas/chrome_settings_overrides.json#42

In manifest.json, one cannot put moz-extension://, because the extension's UUID is unknown.

Currently the only way to do this is by setting an icon in the icons section of manifest.json.
If we change the type from url to relativeUrl, then extensions can use a favicon_url inside their extension, different from what's in the icons field.

(this bug follow from https://phabricator.services.mozilla.com/D40345#inline-245649 )

Comment 1

[Rob Wu [:robwu]]

This can easily be resolved by modifying chrome_settings_overrides.json (that I linked in the previous comment), and changing "url" to "relativeUrl".

Then we also need a unit test, to verify that the icon has the expected value. I suggest to add a new test to https://searchfox.org/mozilla-central/rev/b3fd653bc6078b3be4a8d06db39eddc5714755da/browser/components/extensions/test/browser/browser_ext_search.js , where a test extension has a favicon_url, so that the test can check the return value of browser.search.get, and verify that the result has the expected favIconUrl field.

I'll mentor this bug; anyone who is interested in contributing can get started by taking a look at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

Comment 2

[Megha [ :meghaaa ]]

I would like to work on it, can I take this up?

Comment 3

[Rob Wu [:robwu]]

Hi Megha, feel free to work on this bug. Comment 1 shows how this bug can be fixed, and also links to a page to help you with getting started.

Recently a new unit test was added to test favicon_url, at https://searchfox.org/mozilla-central/rev/9775cca0a10a9b5c5f4e15c8f7b3eff5bf91bbd0/browser/components/extensions/test/browser/browser_ext_search_favicon.js
You can achieve the desired test coverage by modifying one test extension, by removing icons: { 16: ... } and adding favicon_url: ... to its search_provider declaration.

Comment 4

[Myeongjun Go]

Attachment: Bug 1571110 - Change chrome_settings_overrides.search_provider.favicon_url from "url" to "relativeUrl" r=robwu

Comment 5

[Myeongjun Go]

I think that's enough time for the person up there.
So, I submitted patch.
Thank you.

Comment 6

[Mark Banner (:standard8)]

Myeongjun, thank you for the patch, I've just pushed this to our integration branch.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a763059a684
Change chrome_settings_overrides.search_provider.favicon_url from "url" to "relativeUrl" r=Standard8

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/3a763059a684

Comment 9

[Miruna Curtean]

Hello,
Does this issue require manual testing? In comment 3 it appears there was an unit test added but perhaps it is not enough.
Could you please set the qe+ verify flag and add provide a test extension example or steps to reproduce if manual testing is required?
Otherwise the "qe- verify" flag can be set.
Thank you