509194 - Remove extension manager dependency on classic.jar

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop] (he/him)]

The extension manager currently relies on the application including a classic.jar containing the icon and preview images for the default theme. There isn't really any need for this we should just put the images alongside the theme's install.rdf just like for other themes.

Comment 1

[Dave Townsend [:mossop] (he/him)]

Attachment: build config changes [checked in]

This patch stops packaging the images in classic.jar and installs them to <installdir>/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd} to match other themes. This is the only part that blocks bug 468011.

I think you're the right reviewer for this dao, but pass it on if not please.

Comment 2

[Dave Townsend [:mossop] (he/him)]

Attachment: extension manager changes

This stops us looking in classic.jar if the images aren't found in the filesystem.

Comment 3

[Dave Townsend [:mossop] (he/him)]

Attachment: thunderbird patch [checked in]

These are the changes for Thunderbird, as usual I can't guess who would be the right reviewer.

Comment 4

[Mark Banner (:standard8)]

(In reply to comment #3)
> These are the changes for Thunderbird, as usual I can't guess who would be the
> right reviewer.

For the record, the applicable review pages for comm-central can be found listed on the comm-central devmo page:

https://developer.mozilla.org/en/comm-central#comm-central_tree_rules

Having said that, philor is a good choice in this case.

Comment 5

[Phil Ringnalda (:philor)]

Comment on attachment 393364 [details] [diff] [review]
thunderbird patch [checked in]

Looks good to me, other than the two bugs I had to file after actually looking at the previews :)

I don't see quite why we need := for the assignment to FILES, but then I don't see quite why we use it in several other similar places in the tree, so I guess it's fine by me either way.

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 393355 [details] [diff] [review]
extension manager changes

Looks fine. Do you think it is worthwhile to add a test for this? Also, SeaMonkey and Sunbird will need similar changes as Thunderbird.

Comment 7

[Phil Ringnalda (:philor)]

They don't, actually - as Mossop explained to me when I was worried about needing 1.9.1 ifdefs, SeaMonkey already does it this way, while Sunbird does it in calendar/sunbird/app/profile/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/Makefile.in

Comment 8

[Robert Strong (they/them - no direct email)]

Cool that they are ahead of the curve on this one. :)

Comment 9

[Dave Townsend [:mossop] (he/him)]

Comment on attachment 393354 [details] [diff] [review]
build config changes [checked in]

Landed this part: http://hg.mozilla.org/mozilla-central/rev/f434af8fee98

Comment 10

[Dave Townsend [:mossop] (he/him)]

Comment on attachment 393355 [details] [diff] [review]
extension manager changes

I guess since this affects other applications it requires SR. Benjamin this just stops looking in classic.jar for the default theme's icon and preview, applications must include them alongside the install.rdf now, which has been an option all along anyway

Comment 11

[Dave Townsend [:mossop] (he/him)]

I'll do a post to dev.platform about this change before landing it.

Comment 12

[Dave Townsend [:mossop] (he/him)]

Comment on attachment 393364 [details] [diff] [review]
thunderbird patch [checked in]

Landed the Thunderbird part: http://hg.mozilla.org/comm-central/rev/1bb0e6d70434

Comment 13

[Dave Townsend [:mossop] (he/him)]

(In reply to comment #6)
> (From update of attachment 393355 [details] [diff] [review])
> Looks fine. Do you think it is worthwhile to add a test for this?

I'm not entirely sure what it would test. Maybe just try to get the iconURL for the default theme and check it maps to the filesystem?

Comment 14

[Robert Strong (they/them - no direct email)]

Testing the iconURL should suffice. When Fennec first started testing for app update they didn't have all of the pre-requisites at that time which caused a couple of bugs to be filed.

Comment 15

[Dave Townsend [:mossop] (he/him)]

Attachment: testcase [checked in]

Something like this?

Comment 16

[Dave Townsend [:mossop] (he/him)]

Comment on attachment 394004 [details] [diff] [review]
testcase [checked in]

Landed the test: http://hg.mozilla.org/mozilla-central/rev/376b78fc7223

Comment 17

[Dave Townsend [:mossop] (he/him)]

Posted to m.d.platform, but since we've branched already I've just landed this on trunk immediately. Will apply for approval shortly.

http://hg.mozilla.org/mozilla-central/rev/e5d56aa2b693

Comment 18

[Dave Townsend [:mossop] (he/him)]

Comment on attachment 393355 [details] [diff] [review]
extension manager changes

This is required to stop us looking for a classic.jar in the installation folder. Notice has been posted to the newsgroup. Pretty much no risk here

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 393355 [details] [diff] [review]
extension manager changes

a192=beltzner

Comment 20

[Dave Townsend [:mossop] (he/him)]

Pushed to branch: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2d06979c34bc