594329 - Software Installation dialog reports wrong number of items to be installed

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Utku Altunkaya]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5) Gecko/20100101 Firefox/4.0b5
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5) Gecko/20100101 Firefox/4.0b5

When trying to install several extensions by drag-dropping XPI files on Firefox window, the Software Installation dialog wrongly reports "You have asked to install the following (some fractional number) items".

Reproducible: Always

Steps to Reproduce:
1. Select several XPI files from local disk and drag-drop them on Firefox window.
2. The Software Installation dialog pops up.
Actual Results:  
Although I drag-drop, say, 6 extensions, the dialog reports "1.5 items". On another occasion with some 10+ extension I've also seen "1.25 items".

Expected Results:  
The dialog should report the correct number of items.

Comment 1

[Utku Altunkaya]

Attachment: Screenshot

Comment 2

[Bobby Johnson [:rjohnson19]]

Confirmed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100907 Firefox/4.0b6pre

You need at least 5 XPIs to trigger the count of items that will be installed.

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/xpinstall/content/xpinstallConfirm.js#87 looks wrong but it hasn't changed in awhile so maybe I am missing something?

Comment 3

[Dave Townsend [:mossop]]

Ah yeah that is bogus, just need to get rid of the division there I think, and test it.

Comment 4

[Danny Moules]

Attachment: Patch proposal

My first Bugzilla patch/diff. Just implemented Mossop's suggestion, manually tested that it works as intended.

Comment 5

[Dave Townsend [:mossop]]

How do you feel about updating one of the automated tests to verify this fix? browser_signed_multiple.js would be a good one, as would browser_dragdrop.js

Comment 6

[Danny Moules]

I'll certainly see what I can do.

Comment 7

[Danny Moules]

Attachment: Patch With Test

Functional, but kinda ruins the semantics of the previous test (not so much signed anymore).

Comment 8

[Dave Townsend [:mossop]]

(In reply to comment #7)
> Created attachment 474390 [details] [diff] [review]
> Patch With Test
> 
> Functional, but kinda ruins the semantics of the previous test (not so much
> signed anymore).

I think that's fine, just one thing though, you need to actually test that the string in the dialog says the right thing in the test, as that is what you are fixing here.

Comment 9

[Danny Moules]

Oops. Stupid omission.

Just having a look, will I need to check the localised result against a localised test? If I use English might it break in some test environments?

Comment 10

[Dave Townsend [:mossop]]

Just test against the English string, I don't believe that we run the automated tests on localized builds.

Comment 11

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

I am fairly certain that we have had bugs filed by devs running tests against localized builds when the test relies on it being en-US. Would getting the string from the bundle in the test instead be a valid enough test?

Comment 12

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

For example, the test could hard code the number expected when getting the string while the client code calculates it. I would think that would be adequate without breaking localized build tests.

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #12)
> For example, the test could hard code the number expected when getting the
> string while the client code calculates it. I would think that would be
> adequate without breaking localized build tests.

I guess we could do that, though I can point to a lot of tests that just use the English string right now

Comment 14

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

It is entirely possible that people gave up filing bugs or making tests just work no matter the locale. Wouldn't hurt to do it this way though and it would IMO be the correct way.

Comment 15

[Danny Moules]

Attachment: Proposed Patch + Test

Hopefully this should do the job (supports localised strings)

Comment 16

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

Comment on attachment 474419 [details] [diff] [review]
Proposed Patch + Test

>+  var expectedIntroString = bundle.GetStringFromName("itemWarnIntroMultiple");
>+  expectedIntroString = expectedIntroString.replace("%S", "5"); //You have asked to install the following 5 items:
You can use formatStringFromName instead as follows:
var expectedIntroString = bundle.formatStringFromName("itemWarnIntroMultiple", "5", 1);

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 474419 [details] [diff] [review]
Proposed Patch + Test

Make the changes that Rob suggests and I think we'll be good here.

Comment 18

[Danny Moules]

Attachment: Proposed Patch + Test

Implemented proper String handling - note: second argument should be an array.

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 474559 [details] [diff] [review]
Proposed Patch + Test

Awesome, thanks for the patch

Comment 20

[Dave Townsend [:mossop]]

Thanks for the patch, landed: http://hg.mozilla.org/mozilla-central/rev/33405969cb62

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100917 Firefox/4.0b7pre