Closed Bug 1571110 Opened 1 year ago Closed 3 months ago

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

Categories

(WebExtensions :: General, enhancement, P3)

enhancement

Tracking

(firefox72 fixed)

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: robwu, Assigned: myeongjun.ko, Mentored, NeedInfo)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

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 )

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

Mentor: rob
Keywords: good-first-bug

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

Flags: needinfo?(rob)

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.

Flags: needinfo?(rob)
Priority: -- → P3

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

Flags: needinfo?(rob)

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

Assignee: nobody → myeongjun.ko
Flags: needinfo?(rob)
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3a763059a684
Change chrome_settings_overrides.search_provider.favicon_url from "url" to "relativeUrl" r=Standard8
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

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

Flags: needinfo?(myeongjun.ko)
You need to log in before you can comment on or make changes to this bug.