1499693 - Wrong add-on icon size is chosen in about:addons

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Oriol Brufau [:Oriol]]

The add-on icons in the new about:addons have changed from a 32x32 size into 24x24:
https://searchfox.org/mozilla-central/diff/92458357b34576960fc7e239c39da878a8a3e3eb/toolkit/themes/shared/extensions/extensions.inc.css#330-331

However, if an add-on has multiple icons, the 32x32 is still preferred:

var iconURL = AddonManager.getPreferredIconURL(aAddon, 32, window);
https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/toolkit/mozapps/extensions/content/extensions.xml#904

This kind of problem was already fixed in the past (bug 1474132) but since the about:addons design constantly changes I think a test is needed.

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: Bug 1499693 - Choose correct add-on icon size in about:addons. r=mstriemer

Comment 2

[Oriol Brufau [:Oriol]]

Icons in details are also affected

https://searchfox.org/mozilla-central/diff/92458357b34576960fc7e239c39da878a8a3e3eb/toolkit/themes/shared/extensions/extensions.inc.css#615-616
https://searchfox.org/mozilla-central/rev/9cb3e241502a2d47e2d5057ca771324a446b6695/toolkit/mozapps/extensions/content/extensions.js#2635

Comment 3

[Raul Gurzau (:RaulG)]

Landed here: https://hg.mozilla.org/integration/autoland/rev/e51fa157869fe16e97e27557ee716c1701561c29
Removing checkin-needed tag.

Comment 4

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset e51fa157869f (Bug 1499693) for browser_details.js failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e51fa157869fe16e97e27557ee716c1701561c29

Backout link: https://hg.mozilla.org/integration/autoland/rev/3f081283e13929bfea685b80c81f661904a73e6e

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=208467056&repo=autoland&lineNumber=19911

13:50:27     INFO - Running test 12
13:50:27     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_details.js | Should not get category when manager window is not loaded - 
13:50:27     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_details.js | Should not open category when manager window is not loaded - 
13:50:27     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_details.js | Should not check visible state when manager window is not loaded - 
13:50:27     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_details.js | Category should be visible if attempting to open it - 
13:50:27     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_details.js | Name should be correct - 
13:50:27     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_details.js | Element should not be null, when checking visibility - 
13:50:27     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_details.js | Version should not be hidden - 
13:50:27     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_details.js | Version should be correct - 
13:50:27     INFO - Buffered messages finished
13:50:27     INFO - TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_details.js | Icon should be correct - Got chrome://foo/skin/icon.png, expected chrome://foo/skin/icon264.png
13:50:27     INFO - Stack trace:
13:50:27     INFO - chrome://mochikit/content/browser-test.js:test_is:1295
13:50:27     INFO - chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_details.js:null:528
13:50:27     INFO - promise callback*chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/head.js:log_callback:178
13:50:27     INFO - chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/head.js:wait_for_view_load:352
13:50:27     INFO - chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_details.js:open_details:31
13:50:27     INFO - chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_details.js:null:508
13:50:27     INFO - chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/head.js:log_exceptions:170
13:50:27     INFO - chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/head.js:run_next_test/<:211
13:50:27     INFO - chrome://mochikit/content/browser-test.js:run:1324
13:50:27     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_details.js | Element should not be null, when checking visibility -

Comment 5

[Oriol Brufau [:Oriol]]

OK, that test needs to be updated. But I don't see what's the point of the icon64URL property of addons if 64x64 icons are no longer used anywhere. Should I rename it to icon32URL, just remove it, or keep it even if useless?

Comment 6

[Andrew Swan [:aswan]]

Without looking more closely, I wouldn't be opposed to getting rid of iconURL and icon64URL altogether and requiring callers to just use getPreferredIconURL() directly (maybe renaming it to something less clunky at the same time)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c212ed523a0
Choose correct add-on icon size in about:addons. r=mstriemer,aswan

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/0c212ed523a0

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 10

[CosminB]

Attachment: Bug1499693.png

I was able to reproduce this issue on Firefox 63.0(20181018182531) under Win 7 64-bit and iMac Sierra 10.12.6

This issue is verified as fixed on Firefox 65.0a1(20181031223503) under Win 7 64-bit, iMac Sierra 10.12.6 and Ubuntu 14.04 32-bit.

Please see the attached screenshot.

Comment 11

[Oriol Brufau [:Oriol]]

Firefox 63 was fixed in bug 1474132, so I would say it's unaffected by this one.

Comment 12

[Oriol Brufau [:Oriol]]

Comment on attachment 9018816 [details]
Bug 1499693 - Choose correct add-on icon size in about:addons. r=mstriemer

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1490366

User impact if declined: If an extension has multiple icons for different sizes, the wrong one may be chosen in about:addons, possibly causing blurriness.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The code just updates AddonManager.getPreferredIconURL calls to use the new correct size, and adds a test.

String changes made/needed:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9018816 [details]
Bug 1499693 - Choose correct add-on icon size in about:addons. r=mstriemer

[Triage Comment]
Fixes blurry icons in the addon manager if an extension includes multiple icon sizes. Approved for 64.0b6.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8a28169fa91d

Comment 15

[CosminB]

This issue is verified as fixed on Firefox 64.0b6(20181101155334) under Win 7 64-bit, iMac Sierra 10.12.6 and Ubuntu 14.04 32-bit.