We should allow chrome URLs for search engines that are built in. This would allow us to better manage the search engine images and avoid the large data URLs we currently use.

I'm wondering if we should allow resource URLs in this case as well so that the images can go in resource://search-plugins/

Comment on attachment 8755989 [details] [diff] [review] Allow chrome icons by fallning through to data if we are a default engine Review of attachment 8755989 [details] [diff] [review]: ----------------------------------------------------------------- I'm surprised we can get this to work with such a simple patch, but I don't see anything else that would be needed :). Allowing resource too seems reasonable. I think this needs a test.

> I think this needs a test. I knew you'd say that :) Do you think in this case, just querying the iconURI is enough? To make sure it stayed the same as the passed in URL (versus being empty)

(In reply to Mike Kaply [:mkaply] from comment #3) > > I think this needs a test. > > I knew you'd say that :) > > Do you think in this case, just querying the iconURI is enough? To make sure > it stayed the same as the passed in URL (versus being empty) The test needs to at least check that: - the iconURI doesn't become empty. - the chrome iconURI isn't converted to a data URL. Additionally, it would be nice to test that things also work correctly for plugins with several icons (of different sizes). Are you planning to make this work for chrome URLs, or for relative URLs? Or both?

Comment on attachment 8755989 [details] [diff] [review] Allow chrome icons by fallning through to data if we are a default engine Review of attachment 8755989 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/search/nsSearchService.js @@ +1777,5 @@ > + this.name + "\"."); > // Only accept remote icons from http[s] or ftp > switch (uri.scheme) { > + case "chrome": > + // We only allow chrome icon URLS for builtin search engines nit: URLs

New patch with tests. I can't combine the two tests into one test because the names of the engines clash. This seemed like a better idea then having four engines with 1 test (although even when I tried that, it had trouble).

I clashed with your comments. > Additionally, it would be nice to test that things also work correctly for plugins with several icons (of different sizes). We already have a multi size test and this change doesn't really affect the core behavior. It just adds another URL case, so I think that's probably overkill. > Are you planning to make this work for chrome URLs, or for relative URLs? Or both? Only chrome and resource URls. > nit: URLs Will fix when i checkin.

Comment on attachment 8756427 [details] [diff] [review] Patch with testcases Review of attachment 8756427 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. (In reply to Mike Kaply [:mkaply] from comment #7) > > Additionally, it would be nice to test that things also work correctly for plugins with several icons (of different sizes). > > We already have a multi size test and this change doesn't really affect the > core behavior. It just adds another URL case, so I think that's probably > overkill. The reason why I suggested this additional test is to ensure we have the _addIconToMap/getIconURLBySize code paths covered for chrome URLs, so that no future code working only with data URLs is introduced there. I'm not requiring this change to r+, but it seems it would be easy to add a second <Image> line in engine-chromeicon.xml, and an engine2.getIconURLBySize check.

> I'm not requiring this change to r+, but it seems it would be easy to add a second <Image> line in engine-chromeicon.xml, and an engine2.getIconURLBySize check. Yep. After thinking on it, I agree. These URLs don't have to be existing images which is what I was thinking. I will probably also update the tests to have different URLs.

Patch with extra image checks that will be checked in.

(In reply to Mike Kaply [:mkaply] from comment #7) > > nit: URLs > > Will fix when i checkin. Looks like you forgot this when updating the patch.

https://hg.mozilla.org/integration/fx-team/rev/1e06203b983cbf41327bbf5706c68ef194bc62a3 Bug 1275366 - Allow chrome/resource icon URLs for built-in search engines. r=florian

I reproduced this issue using Nightly 49.0a1 (2016-05-24), Build ID: 20160524030227 on Linux x64. This Bug is now verified as fixed on Latest Firefox 53.0 (64-bit) Build ID: 20170418123315 User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0 OS: Linux 4.4.0-72-generic; Elementary OS 0.4