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

VERIFIED FIXED in Firefox 64

Status

()

defect
P2
normal
VERIFIED FIXED
9 months ago
9 months ago

People

(Reporter: Oriol, Assigned: Oriol)

Tracking

(Blocks 1 bug, {regression})

64 Branch
mozilla65
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox63 unaffected, firefox64 verified, firefox65 verified)

Details

Attachments

(2 attachments)

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.
Priority: -- → P2
Keywords: checkin-needed
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 -
Flags: needinfo?(oriol-bugzilla)
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?
Flags: needinfo?(oriol-bugzilla)
Flags: needinfo?(mstriemer)
Flags: needinfo?(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)
Flags: needinfo?(aswan)
Pushed by mstriemer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c212ed523a0
Choose correct add-on icon size in about:addons. r=mstriemer,aswan
https://hg.mozilla.org/mozilla-central/rev/0c212ed523a0
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Please nominate this for Beta approval when you get a chance.
Blocks: 1474132
Flags: qe-verify+
Flags: needinfo?(oriol-bugzilla)
Flags: in-testsuite+
Keywords: regression
QA Contact: cbadescu
Posted image 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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Firefox 63 was fixed in bug 1474132, so I would say it's unaffected by this one.
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:
Flags: needinfo?(oriol-bugzilla)
Attachment #9018816 - Flags: approval-mozilla-beta?
Flags: needinfo?(mstriemer)
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.
Attachment #9018816 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.