Closed Bug 594329 Opened 12 years ago Closed 12 years ago
Software Installation dialog reports wrong number of items to be installed
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.
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?
Ah yeah that is bogus, just need to get rid of the division there I think, and test it.
Status: UNCONFIRMED → NEW
blocking2.0: --- → final+
Ever confirmed: true
Whiteboard: [good first bug]
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
My first Bugzilla patch/diff. Just implemented Mossop's suggestion, manually tested that it works as intended.
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
I'll certainly see what I can do.
Functional, but kinda ruins the semantics of the previous test (not so much signed anymore).
Attachment #473563 - Attachment is obsolete: true
(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.
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?
Just test against the English string, I don't believe that we run the automated tests on localized builds.
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?
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.
(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
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.
Hopefully this should do the job (supports localised strings)
Attachment #474390 - Attachment is obsolete: true
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 on attachment 474419 [details] [diff] [review] Proposed Patch + Test Make the changes that Rob suggests and I think we'll be good here.
Attachment #474419 - Flags: review?(dtownsend) → review-
Implemented proper String handling - note: second argument should be an array.
Attachment #474419 - Attachment is obsolete: true
Comment on attachment 474559 [details] [diff] [review] Proposed Patch + Test Awesome, thanks for the patch
Attachment #474559 - Flags: review?(dtownsend) → review+
Thanks for the patch, landed: http://hg.mozilla.org/mozilla-central/rev/33405969cb62
Assignee: nobody → opensource
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100917 Firefox/4.0b7pre
Status: RESOLVED → VERIFIED
Whiteboard: [good first bug] → [AOMTestday][in-litmus-bug-week][good first bug]
You need to log in before you can comment on or make changes to this bug.