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

VERIFIED FIXED in mozilla1.9.3a2

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: LI Leding, Assigned: Lingfeng.Guan)

Tracking

({verified1.9.2})

1.9.2 Branch
mozilla1.9.3a2
verified1.9.2
Points:
---

Firefox Tracking Flags

(status1.9.2 .5-fixed)

Details

Attachments

(2 attachments, 9 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 423330 [details] [diff] [review]
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.
Attachment #423330 - Attachment is patch: true
Attachment #423330 - Attachment mime type: application/octet-stream → text/plain
No need to use FUEL here, just use getComplexValue(name, Ci.nsISupportsString)/setComplexValue(name, Ci.nsISupportsString, value) directly.
(Assignee)

Comment 2

8 years ago
Created attachment 423347 [details] [diff] [review]
patch version 2 for resource://gre/modules/LightweightThemeManager.jsm
Attachment #423330 - Attachment is obsolete: true
Attachment #423347 - Flags: approval1.9.2.1?
(Assignee)

Comment 3

8 years ago
Got you:). I just switched to getComplexValue/setComplexValue
Btw: Do you think it is possible to land in 1.9.2.1?
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 on attachment 423347 [details] [diff] [review]
patch version 2 for resource://gre/modules/LightweightThemeManager.jsm

(and you need to request review before requesting approval)
Attachment #423347 - Flags: approval1.9.2.1?
(Assignee)

Comment 6

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

Updated

8 years ago
Component: Theme → Add-ons Manager
Product: Firefox → Toolkit
QA Contact: theme → add-ons.manager
Version: 3.6 Branch → 1.9.2 Branch
(Assignee)

Comment 7

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

Comment 8

8 years ago
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 [];
    }
  },
OK. You should ask dao@mozilla or :mossop for review.
(Assignee)

Updated

8 years ago
Attachment #423347 - Flags: review?(dtownsend)

Comment 10

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

Comment 11

8 years ago
Oops, you are right! I'll update the patch
(Assignee)

Comment 12

8 years ago
Created attachment 423489 [details] [diff] [review]
patch version 3 for resource://gre/modules/LightweightThemeManager.jsm
Attachment #423347 - Attachment is obsolete: true
Attachment #423489 - Flags: review?(dtownsend)
Attachment #423347 - Flags: review?(dtownsend)
(Assignee)

Comment 13

8 years ago
Created attachment 423490 [details] [diff] [review]
423489: patch version 3 for resource://gre/modules/LightweightThemeManager.jsm

got the line number wrong, sorry for the disturbance
Attachment #423490 - Flags: review?(dtownsend)
(Assignee)

Updated

8 years ago
Attachment #423489 - Attachment is obsolete: true
Attachment #423489 - Flags: review?(dtownsend)
Assignee: nobody → lguan
Status: NEW → ASSIGNED
(Assignee)

Comment 14

8 years ago
What should I do now? If I was assigned with this bug, it's my first time:)
Just wait for Mossop to review it. Thanks for the patch!
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
Attachment #423490 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 18

8 years ago
Yes, sir
(Assignee)

Comment 19

8 years ago
Created attachment 425206 [details] [diff] [review]
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
Attachment #423490 - Attachment is obsolete: true
Attachment #425206 - Flags: review?(dtownsend)
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
(Assignee)

Comment 21

8 years ago
Created attachment 425616 [details] [diff] [review]
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
Attachment #425206 - Attachment is obsolete: true
Attachment #425616 - Flags: review?(dtownsend)
Attachment #425206 - Flags: review?(dtownsend)
(Assignee)

Comment 22

8 years ago
Created attachment 425619 [details] [diff] [review]
patch version 6, test case added, change tab indent to double space

misdeleted a enter, and replaced a tab indent with spaces
Attachment #425616 - Attachment is obsolete: true
Attachment #425619 - Flags: review?(dtownsend)
Attachment #425616 - Flags: review?(dtownsend)
Attachment #425619 - Flags: review?(dtownsend) → review-
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.
(Assignee)

Comment 24

8 years ago
Yes, sir!
But I didn't understand the first line. Is it I cloned a wrong branch, or I just need to pull?
(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.
(Assignee)

Comment 26

8 years ago
got it. it appears that I cloned mozilla-1.9.2 branch, I'll use mozilla-central
(Assignee)

Comment 27

8 years ago
Created attachment 426718 [details] [diff] [review]
Patch version 7, fix bugs in tests

All things mentioned is solved:)
Attachment #425619 - Attachment is obsolete: true
Attachment #426718 - Flags: review?(dtownsend)
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.
Attachment #426718 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 29

8 years ago
Created attachment 428020 [details] [diff] [review]
Patch version 8, fix the problems mentioned.
Attachment #428020 - Flags: review?
(Assignee)

Comment 30

8 years ago
Created attachment 428021 [details] [diff] [review]
Patch version 8, fix the problems mentioned.
Attachment #426718 - Attachment is obsolete: true
Attachment #428020 - Attachment is obsolete: true
Attachment #428021 - Flags: review?(dtownsend)
Attachment #428020 - Flags: review?
Comment on attachment 428021 [details] [diff] [review]
Patch version 8, fix the problems mentioned.

Looks good, are you able to land this?
Attachment #428021 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 32

8 years ago
I'm not sure what do I need to do. But I have the passion:)
(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?
(Assignee)

Comment 34

8 years ago
I don't think I have:(
I'll land it if no-one else gets to it by tomorrow.
Keywords: checkin-needed
(Assignee)

Comment 36

8 years ago
Thank you very much:)
landed: http://hg.mozilla.org/mozilla-central/rev/9849994d25f7
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs baking]
Target Milestone: --- → mozilla1.9.3a2
Comment on attachment 428021 [details] [diff] [review]
Patch version 8, fix the problems mentioned.

This is necessary on the branch for supporting international personas.
Attachment #428021 - Flags: approval1.9.2.2?
Whiteboard: [needs baking]
Attachment #428021 - Flags: approval1.9.2.2? → approval1.9.2.3?

Comment 39

8 years ago
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
Attachment #428021 - Flags: approval1.9.2.4? → approval1.9.2.5+

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bf47aa81a47b
status1.9.2: --- → .5-fixed
Keywords: checkin-needed
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

8 years ago
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
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

8 years ago
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?
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

8 years ago
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
Created attachment 455718 [details]
UTF Title screenshot

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

Comment 48

8 years ago
Thx, Tony

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

Comment 49

8 years ago
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?
Duplicate of this bug: 553340
(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
(Assignee)

Comment 52

8 years ago
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

8 years ago
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.
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
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2

Comment 55

8 years ago
Cool!
(Assignee)

Comment 56

8 years ago
Awesome! Thx, Tony and Mossop :)
You need to log in before you can comment on or make changes to this bug.