Software Installation dialog reports wrong number of items to be installed

VERIFIED FIXED in mozilla2.0b7

Status

()

--
trivial
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: utkualtunkaya, Assigned: opensource)

Tracking

Trunk
mozilla2.0b7
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [AOMTestday][in-litmus-bug-week][good first bug])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
Created attachment 473006 [details]
Screenshot
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
(Assignee)

Comment 4

8 years ago
Created attachment 473563 [details] [diff] [review]
Patch proposal

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
(Assignee)

Comment 6

8 years ago
I'll certainly see what I can do.
(Assignee)

Comment 7

8 years ago
Created attachment 474390 [details] [diff] [review]
Patch With Test

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.
(Assignee)

Comment 9

8 years ago
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.
(Assignee)

Comment 15

8 years ago
Created attachment 474419 [details] [diff] [review]
Proposed Patch + Test

Hopefully this should do the job (supports localised strings)
Attachment #474390 - Attachment is obsolete: true
Attachment #474419 - Flags: review?(dtownsend)
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-
(Assignee)

Comment 18

8 years ago
Created attachment 474559 [details] [diff] [review]
Proposed Patch + Test

Implemented proper String handling - note: second argument should be an array.
Attachment #474419 - Attachment is obsolete: true
Attachment #474559 - Flags: review?(dtownsend)
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
Last Resolved: 8 years ago
Flags: in-testsuite+
Flags: in-litmus-
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.