541943 - LightweightTheme doesn't support theme name/description in UTF-8

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[LI Leding]

Attachment: Patch to modules/lightweightthememanager.jsm

We beijing office have a modified Personas server.
Here we provide persona in Chinese name/description.
Unfortunately, the Firefox 3.6 doesn't support LWTheme in UTF-8 encoding.
It uses _prefs.set/getCharPref to store/load LWTheme list in json format but these cannot be applied to UTF-8 strings.

So, when user choose a LWTheme with Chinese name/description, it will definitely a unreadable chars in addon manager.
Sometimes it erases all LWTheme data.

We made a patch to handle this circumstance.
Just replace _pref.set/getCharPref by Application.prefs.set/getValue.
And it is good.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

No need to use FUEL here, just use getComplexValue(name, Ci.nsISupportsString)/setComplexValue(name, Ci.nsISupportsString, value) directly.

Comment 2

[Lingfeng.Guan]

Attachment: patch version 2 for resource://gre/modules/LightweightThemeManager.jsm

Comment 3

[Lingfeng.Guan]

Got you:). I just switched to getComplexValue/setComplexValue
Btw: Do you think it is possible to land in 1.9.2.1?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

We'll need to take into account the compatibility issues. There are possible values for the pref that, when set with getCharPref(), will make the return value of getComplexValue invalid JSON, I think (truncating the result). Does the code handle that correctly?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 423347 [details] [diff] [review]
patch version 2 for resource://gre/modules/LightweightThemeManager.jsm

(and you need to request review before requesting approval)

Comment 6

[Lingfeng.Guan]

Oops. You are right, if the user had that pref set using setCharPref already, I don't know what will happen, if we get it out using getComplexValue, I guess it could be a mess...

Comment 7

[Lingfeng.Guan]

But how can I tell the different between a nsISupportsString pref a normal string pref?
I looked at http://mxr.mozilla.org/mozilla1.9.2/source/toolkit/components/exthelper/extApplication.js#231 
and it seems that prefs in FUEL just use getComplexValue to handle all string prefs

Comment 8

[Lingfeng.Guan]

Gavin: About the question you asked, I think the answer is a yes, this is the code used to get the usedThemes, I think it has handled the situation when parse failed

get usedThemes () {
    try {
      return JSON.parse(_prefs.getComplexValue("usedThemes", Ci.nsISupportsString).data);
    } catch (e) {
      return [];
    }
  },

Comment 9

[:Gavin Sharp [email: [e-mail]]]

OK. You should ask dao@mozilla or :mossop for review.

Comment 10

[Jia Mi [:mijia]]

I think setComplexValue part should be:

var str = Components.classes["@mozilla.org/supports-string;1"]
           .createInstance(Ci.nsISupportsString);
str.data = JSON.stringify(aList);
_prefs.setComplexValue("usedThemes", Ci.nsISupportsString, str);

Otherwise, it will throw exception of bad conversion of arg.

Comment 11

[Lingfeng.Guan]

Oops, you are right! I'll update the patch

Comment 12

[Lingfeng.Guan]

Attachment: patch version 3 for resource://gre/modules/LightweightThemeManager.jsm

Comment 13

[Lingfeng.Guan]

Attachment: 423489: patch version 3 for resource://gre/modules/LightweightThemeManager.jsm

got the line number wrong, sorry for the disturbance

Comment 14

[Lingfeng.Guan]

What should I do now? If I was assigned with this bug, it's my first time:)

Comment 15

[Markus Stange [:mstange]]

Just wait for Mossop to review it. Thanks for the patch!

Comment 16

[Dave Townsend [:mossop]]

https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch might be a useful read to explain the process.

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 423490 [details] [diff] [review]
423489: patch version 3 for resource://gre/modules/LightweightThemeManager.jsm

Please use Cc and Ci instead of Components.classes and Components.interfaces and align the .createInstance part the same as is done throughout the rest of this file.

We need an automated test to verify the behaviour here.

Please in the future provided patches against the original source file rather than the compiled version, there is no guarantee they will be the same as they are in this case, and use 8 lines of context and function headers as explained here: https://developer.mozilla.org/en/Creating_a_patch

Comment 18

[Lingfeng.Guan]

Yes, sir

Comment 19

[Lingfeng.Guan]

Attachment: patch version 4, created using hg diff

Sorry for the delay, I've finally built a firefox for the first time:) And here is the patch created using hg. I've read the part about test case, and I'm not sure how to write one yet, and I think that might need sometime, so I attached the patch in advance

Comment 20

[Dave Townsend [:mossop]]

This file holds the main tests for the lightweight theme manager, you could either add to it or write a new file to do the specific test for this bug: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/unit/test_LightweightThemeManager.js

Comment 21

[Lingfeng.Guan]

Attachment: patch version 5, test case added

Note that the new http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/unit/test_LightweightThemeManager.js file includes Chinese characters, in order for it to work, should set the encoding of the file to utf-8 without BOM

Comment 22

[Lingfeng.Guan]

Attachment: patch version 6, test case added, change tab indent to double space

misdeleted a enter, and replaced a tab indent with spaces

Comment 23

[Dave Townsend [:mossop]]

Comment on attachment 425619 [details] [diff] [review]
patch version 6, test case added, change tab indent to double space

This patch has been bitrotted on trunk so you'll need to update it accordingly, also the following minor issues need correcting then it should be good to land:

>diff --git a/toolkit/mozapps/extensions/src/LightweightThemeManager.jsm b/toolkit/mozapps/extensions

> var LightweightThemeManager = {
>   get usedThemes () {
>     try {
>-      return JSON.parse(_prefs.getCharPref("usedThemes"));
>+      return JSON.parse(_prefs.getComplexValue("usedThemes", Ci.nsISupportsString).data);

Lines should be no longer than 80 characters.

>diff --git a/toolkit/mozapps/extensions/test/unit/test_LightweightThemeManager.js b/toolkit/mozapps/extensions/test/unit/test_LightweightThemeManager.js

>+  //use chinese id to test utf-8, for bug #541943

The ID is not the thing to be testing here. The bug is specifically about supporting international characters in the name and description so please check those fields specifically.

You should also test that parseTheme handles international characters using the roundtrip functions in the test file.

Comment 24

[Lingfeng.Guan]

Yes, sir!
But I didn't understand the first line. Is it I cloned a wrong branch, or I just need to pull?

Comment 25

[Dave Townsend [:mossop]]

(In reply to comment #24)
> Yes, sir!
> But I didn't understand the first line. Is it I cloned a wrong branch, or I
> just need to pull?

Changes have been made to the file since you wrote your patch, the file also moved to a different place so your patch no longer applies cleanly.

Comment 26

[Lingfeng.Guan]

got it. it appears that I cloned mozilla-1.9.2 branch, I'll use mozilla-central

Comment 27

[Lingfeng.Guan]

Attachment: Patch version 7, fix bugs in tests

All things mentioned is solved:)

Comment 28

[Dave Townsend [:mossop]]

Comment on attachment 426718 [details] [diff] [review]
Patch version 7, fix bugs in tests

Still a couple of nits here that need to be cleaned up before we can land this:

>diff --git a/toolkit/mozapps/extensions/test/unit/test_LightweightThemeManager.js b/toolkit/mozapps/extensions/test/unit/test_LightweightThemeManager.js

>+  //test whether a JSON set with setCharPref can be retrieved with usedThemes
>+  ltm.currentTheme = dummy("x0");
>+  ltm.currentTheme = dummy("x1");
>+  var _prefs =Cc["@mozilla.org/preferences-service;1"]
>+                .getService(Ci.nsIPrefService).getBranch("lightweightThemes.");
>+  _prefs.setCharPref("usedThemes", JSON.stringify(ltm.usedThemes));

prefs is already declared later in the file, just move your tests below that and reuse it.

>+//check whether parseTheme handles international characters right

Indent this comment like the rest of the code.

Comment 29

[Lingfeng.Guan]

Attachment: Patch version 8, fix the problems mentioned.

Comment 30

[Lingfeng.Guan]

Attachment: Patch version 8, fix the problems mentioned.

Comment 31

[Dave Townsend [:mossop]]

Comment on attachment 428021 [details] [diff] [review]
Patch version 8, fix the problems mentioned.

Looks good, are you able to land this?

Comment 32

[Lingfeng.Guan]

I'm not sure what do I need to do. But I have the passion:)

Comment 33

[Dave Townsend [:mossop]]

(In reply to comment #32)
> I'm not sure what do I need to do. But I have the passion:)

Do you have commit privileges for mozilla-central?

Comment 34

[Lingfeng.Guan]

I don't think I have:(

Comment 35

[Dave Townsend [:mossop]]

I'll land it if no-one else gets to it by tomorrow.

Comment 36

[Lingfeng.Guan]

Thank you very much:)

Comment 37

[Dave Townsend [:mossop]]

landed: http://hg.mozilla.org/mozilla-central/rev/9849994d25f7

Comment 38

[Dave Townsend [:mossop]]

Comment on attachment 428021 [details] [diff] [review]
Patch version 8, fix the problems mentioned.

This is necessary on the branch for supporting international personas.

Comment 39

[LegNeato]

Comment on attachment 428021 [details] [diff] [review]
Patch version 8, fix the problems mentioned.

a=LegNeato for 1.9.2.5. Please ONLY land this on mozilla-1.9.2 default, as we
are still working on 1.9.2.4 on the relbranch

Comment 40

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bf47aa81a47b

Comment 41

[Tony Chung [:tchung]]

hello, do you have a test persona with UTF-8 title that can be downloaded and tested?  if so please provide it here so it can be verified.  Thanks.

Comment 42

[Jia Mi [:mijia]]

Hi Tony,

Like this one? But it is not in getpersonas.com or amo, but in China fork.
http://personas.g-fox.cn/persona/200000083

Comment 43

[Tony Chung [:tchung]]

hi Jia,

i installed that persona and the title spells out "zhongguofeng*qinghua" in the persona title.   shouldnt i see the double byte characters instead? 

This is against fx3.6.7 on windows

Comment 44

[Jia Mi [:mijia]]

O, right.
Since there was this bug, we have substituted all the UTF-8 name to Chinese pinyin name. Should we create a test case or just upload a persona to AMO with UTF-8 name and description?

Comment 45

[Tony Chung [:tchung]]

i just need a testcase attached here, but if you're planning to upload to AMO anyway, then feel free to do so and point me to it when you're done.   Thanks.

Comment 46

[Jia Mi [:mijia]]

Please try this one, 
http://personas.g-fox.cn/persona/200000083

I only changed the title and description of this persona in our server for the test. You can check the JSON:
http://personas.g-fox.cn/static/8/3/200000083/index_1.json

Comment 47

[Tony Chung [:tchung]]

Attachment: UTF Title screenshot

Hi Jia,  something with that example still doesnt look right.  see attachment.

Comment 48

[Lingfeng.Guan]

Thx, Tony

    It is exactly the same situation with the versions before 3.6.7, I'll look into it

Comment 49

[Lingfeng.Guan]

Jia, could you help me test whether the encoding of the page http://personas.g-fox.cn/static/8/3/200000083/index_1.json being ANSI has anything to do with the result?

Comment 51

[Dave Townsend [:mossop]]

(In reply to comment #45)
> i just need a testcase attached here, but if you're planning to upload to AMO
> anyway, then feel free to do so and point me to it when you're done.   Thanks.

Tony, it looks like the following personas are a valid testcase for this (see bug 553340):

http://www.getpersonas.com/en-US/persona/84398
http://www.getpersonas.com/en-US/persona/84389
https://www.getpersonas.com/en-US/persona/243732

Comment 52

[Lingfeng.Guan]

Hi, Tony

    Sorry that the test case we provided before summit seems not quite right. I and Jia looked into Firefox code today, and found that LightWeightTheme does not use index_1.json as personas did, instead it get the json data from an attribute of a node in the page. and the name attribute of that json is ___.__, because we changed all chinese charactors to _ to make it work for firefox 3.5, and in that test case, we forgot to change it back.
    Jia is working on make another valid test case right now, we'll provide it to you ASAP

Comment 53

[Jia Mi [:mijia]]

Hi Tony,

Try this again, I have changed the json data inside the image element.
http://personas.g-fox.cn/persona/200000083

Thank you very much.

Comment 54

[Tony Chung [:tchung]]

Thanks Jia, this testcase works. Verified fix on trunk and branch.  

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; en-US; rv:2.0b2pre) Gecko/20100715 Minefield/4.0b2pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.7) Gecko/20100701 Firefox/3.6.7

Comment 55

[Jia Mi [:mijia]]

Cool!

Comment 56

[Lingfeng.Guan]

Awesome! Thx, Tony and Mossop :)