87535 - View -> Languages and Web Content -> Download More should not bring up restart mozilla dialouge

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Michael Hendy (Hendikins)]

Reproduction shouldn't be a problem. Since this option brings up a webpage
instead of changing settings, the dialouge shouldn't appear.

Comment 1

[David Einstein]

I suspect that this is intended, but it can be considered annoying. 

Comment 2

[Doron Rosenberg (IBM)]

moving

Comment 3

[nhottanscp]

Reassign to tao.

Comment 4

[Andreas Becker]

Switching QA to [e-mail]

Comment 5

[nhottanscp]

Is this a regression? Does NS6.1 PR1 have this problem?

Comment 6

[nhottanscp]

It does not happen with PR1.

Comment 7

[Jaime Rodriguez, Jr.]

Reassigning to bobj for now. cc'ing Roberets to watch this one, while I am on 
vacation.

Bob - Can you take a look at this?

Comment 8

[nhottanscp]

*** Bug 87857 has been marked as a duplicate of this bug. ***

Comment 9

[nhottanscp]

Reassign to nhotta.

Comment 10

[nhottanscp]

The following code checkes the two locale values (old and new) then alerts if
they are different.
http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/content/pref-appearance.xul#52

In case of "Download More", it could be treated as unmatched case.
Juraj, is that your code or added by ben?

Comment 11

[nhottanscp]

Juraj, I looked at bug 65251 and switchUILanguage is your code, so reassign to 
you. 
I don't see "general.useragent.locale" is modified by the UI language switch so 
the comparison always done against the original language not the current one. 
I think "general.useragent.locale" should not be modified by UI language switch. 
If the current UI language has to be remembered then probably need a separate 
pref.

Comment 12

[nhottanscp]

The code I mentioned was for pref dialog, so probably not causing the menu 
problem. I just talked to Juraj about that, he is going to look at this bug 
anyway, thanks.

Comment 13

[[e-mail] (away - not reading bugmail)]

Naoki, 

so I had a look and it seems that Ben somehow omitted 
"chromeRegistry.selectLocale(langcode, true);" from the original patch when 
changing from alert() to promptService. This is significant in two ways: 

1) without this statement the menu items cannot be used to set UI or content 
locale. 

2)When the language code is empty, as when the "Download More" item was 
clicked, chromeRegistry.selectLocale fails and we don't display the alert 
message. So without it we are getting the bogus alert.

Please see the attached patch, which also addresses the somewhat lame concept 
for bypassing the alert, when language code == null. Since this affects the 
branch, should we add nsBranch and try to get approval to check it in?

Comment 14

[[e-mail] (away - not reading bugmail)]

Attachment: proposed patch v1

Comment 15

[nhottanscp]

r=nhotta

Comment 16

[[e-mail] (away - not reading bugmail)]

cc'ing alecf for potential sr

Comment 17

[Alec Flett]

sr=alecf

for future reference, this code is kinda useless:


 var chromeRegistry =
Components.classes["@mozilla.org/chrome/chrome-registry;1"].getService();
     if ( chromeRegistry ) {
       chromeRegistry = chromeRegistry.QueryInterface(
Components.interfaces.nsIChromeRegistry );
     }

namely because getService() will throw an exception if the service is not found
- basically chromeRegistry will always have a value... if you are not sure if
the service will be there (i.e. that getService() will fail) then you should
wrap this with try/catch

but that code isn't actually part of THIS patch so it doesn't change the review
status

Comment 18

[[e-mail] (away - not reading bugmail)]

Attachment: simplified chrome registry service acquisition per alecf's suggestion

Comment 19

[[e-mail] (away - not reading bugmail)]

alecf, 

thanks for pointing this out - I'd suggest addressing it now. What's strictly 
required for this bug amounts to a 1-line change, and the larger part of the 
patch is already devoted to clean-up... 

Comment 20

[Alec Flett]

sr=alecf on that too

Comment 21

[Frank Tang]

please check into trunk and keep this bug open for moz0.9.2 landing. 

Comment 22

[Asa Dotzler [:asa]]

pushing out. 0.9.2 is done. (querying for this string will get you the list of
the 0.9.2 bugs I moved to 0.9.3)

Comment 23

[[e-mail] (away - not reading bugmail)]

Asa, 

I apologize for marking this 0.9.2. We are merely interested in obtaining 
branch checkin approval, as this addresses key i18n/l10n functionality and 
fixes a menu item regression.

Comment 24

[Asa Dotzler [:asa]]

jbatek.  Actually it should have been me apologizing for the trespass. The
Target Milestone traditionally belongs to the bug's Assignee (in this case, you)
and I generally don't mess with it except that it was targeted at a Milestone
that is now passed.  If you are still trying to get this landed on the branch I
believe that you need to email [e-mail].

Comment 25

[[e-mail] (away - not reading bugmail)]

Asa, 

thanks for clarifying this - I just emailed PDT...

Comment 26

[[e-mail] (away - not reading bugmail)]

waiting for the plan for lifting trunk fixes to the branch to materialize, will 
take additional steps after that...

Comment 27

[[e-mail] (away - not reading bugmail)]

adding vtrunk

Comment 28

[Frank Tang]

This was landed to trunk at Jun 29 15:00

Comment 29

[Jon Rubin]

Changing Platform and OS to All as I reproduced the original problem on WinMe-Ja
and MacOS9.1 (using 07-03-03-0.9.2).  About to verify fix on latest trunk build...

Comment 30

[Jon Rubin]

Verified on trunk on Win32, Linux, and Mac as follows:

07-03-11-trunk on WinMe-Ja
07-03-10-trunk on MacOS-9.1
07-03-18-trunk on Linux Redhat 7.1-Ja

Need to verify on branch when fix checked in there.

Comment 31

[Frank Tang]

mark it as nsbranch+

Comment 32

[Frank Tang]

pdt+ per pdt meting. Land it today

Comment 33

[Frank Tang]

land 92 branch. mark it fixed 

Comment 34

[Frank Tang]

fixed and check into m92

Comment 35

[Frank Tang]

fix in m92

Comment 36

[Jon Rubin]

Verified on branch on Win32, Linux, and Mac as follows:

07-10-05-branch on WinMe-Ja
07-13-03-branch on MacOS-9.1
07-13-04-branch on Linux Redhat 7.1-Ja