1870644 - Provide a single function for obtaining icon URLs from search engines

Status: RESOLVED FIXED

(Firefox :: Search, task, P2)

Description

[Mark Banner (:standard8)]

Currently we have several different properties/methods for accessing the icons on a search engine:

• SearchEngine.iconURI - A getter which returns an nsIURI of the "preferred" icon.
• SearchEngine.getIconURLBySize(width, height) - Returns the URL as a string. The width and height must be an exact match to an icon defined by the search engine.
• SearchEngine.getIcons() - Returns an array of objects containing the icon width, height and url (nsIURI) for the icons. Only used in tests.

I want to simplify this down so that we only have a single function. Ideally the single function should be able to take a preferred width and return the icon closest to that width (e.g. bug 1655070).

However, the work to get us to the ideal state is more than I want to take on at the moment (because we also need to improve this function for search-config-v2), so I'm splitting this up. In this bug, I'm concentrating on implementing a single function, and updating all consumers to use that. We might need to iterate more on it for future patches, but this gets us to a more consistent state.

Hence, getIcons is being removed, as that isn't needed, and can be tested in other ways. iconURI and getIconURLBySize are being merged into one function, getIconURL, which will return a string - most of the consumers deal with strings, so I think that's the better option.

Comment 1

[Mark Banner (:standard8)]

Try push: https://treeherder.mozilla.org/jobs?repo=try&revision=a6caad367e4c6258250c27d23fc4a4e08b5f6bcc

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1870644 - Merge test_multipleIcons.js into test_opensearch_icon.js. r?#search-reviewers!

test_multipleIcons.js is basically testing OpenSearch icons, so we should make that clear.

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1870644 - Remove nsISearchEngine.getIcons as it is only used by tests and not necessary. r?#search-reviewers!

The tests are able to use getting the icon by size for their own checks.

Depends on D196731

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1870644 - Pass a preferred width rather than width and height when getting search engine icon urls. r?#search-reviewers!

This also renames the function, in preparation for iconURI being merged into it.

Depends on D196732

Comment 5

[Mark Banner (:standard8)]

Attachment: Bug 1870644 - Remove nsISearchEngine.iconURI and replace by the new getIconURL function. r?#search-reviewers!

Depends on D196733

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60d1748d9829
Merge test_multipleIcons.js into test_opensearch_icon.js. r=search-reviewers,mcheang
https://hg.mozilla.org/integration/autoland/rev/9d2863f2a18b
Remove nsISearchEngine.getIcons as it is only used by tests and not necessary. r=search-reviewers,mcheang
https://hg.mozilla.org/integration/autoland/rev/f687e03e530f
Pass a preferred width rather than width and height when getting search engine icon urls. r=search-reviewers,mcheang
https://hg.mozilla.org/integration/autoland/rev/8dd343dfcd66
Remove nsISearchEngine.iconURI and replace by the new getIconURL function. r=search-reviewers,extension-reviewers,settings-reviewers,mconley,mcheang,robwu

Comment 7

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/60d1748d9829
https://hg.mozilla.org/mozilla-central/rev/9d2863f2a18b
https://hg.mozilla.org/mozilla-central/rev/f687e03e530f
https://hg.mozilla.org/mozilla-central/rev/8dd343dfcd66

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4374c89e3727
adjust Thunderbird for "Remove nsISearchEngine.iconURI and replace by the new getIconURL function." rs=bustage-fix

Comment 9

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Was https://hg.mozilla.org/mozilla-central/rev/8dd343dfcd66d9d424d3b859742c762d8762644d#l5.14 and the one below supposed to be getIconURL()?

Comment 10

[Mark Banner (:standard8)]

(In reply to Magnus Melin [:mkmelin] from comment #9)

Was https://hg.mozilla.org/mozilla-central/rev/8dd343dfcd66d9d424d3b859742c762d8762644d#l5.14 and the one below supposed to be getIconURL()?

No, the preferences page has a confusing system of cloning parts of the engine objects and using that as a "store" for the display. The store is what is containing iconURL - as demonstrated in the second change on that file.