256409 - bookmarks file path with non-ASCII characters doesn't work

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Jungshik Shin]

'GetCharPref' or 'CopyCharPref' is used to retrieve the path of bookmarks file.
Both APIs can only deal with 'string' (of unspecified encoding. Well, actually,
it's UTF-8). We should use GetComplexValue, instead.

Comment 1

[Jungshik Shin]

Attachment: patch for both seamonkey and aviary

I'm not using nsILocalFile directly because the implementation of
GetComplexValue with nsILocalFile is broken. Or depending on the point of view,
one might say implementations of  SetPersistentProperties is broken in
nsLocalFileXXX.cpp

Comment 2

[Jungshik Shin]

Comment on attachment 156681 [details] [diff] [review]
patch for both seamonkey and aviary

asking for r/sr

Comment 3

[jag (Peter Annema)]

Comment on attachment 156681 [details] [diff] [review]
patch for both seamonkey and aviary

And we should use GetComplexValue + NS_NewLocalFile because
NS_NewNativeLocalFile expects the path in the OS's native character encoding?
That makes sense.

Shouldn't you change
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/boo
kmarksManager.js#147 to set it as a complex value then, or does that store it
as UTF-8, which will (or should?) automatically get converted to UTF-16 when we
GetComplexValue?

Re: nsILocalFile and prefs, please file a bug describing how it's broken.

Comment 4

[Jungshik Shin]

(In reply to comment #3)

> Shouldn't you change
>http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/bookmarksManager.js#147

> to set it as a complex value then, or does that store it

Thanks. You're absolutely right. I should have done that, too. I'll upload a new
patch. 

> Re: nsILocalFile and prefs, please file a bug describing how it's broken.

  I filed bug 256410 for that. 

Comment 5

[Jungshik Shin]

Attachment: patch v2 (include bookmarksManager.js)

took care of bookmarksManager.js (thanks, jag)

Comment 6

[Jungshik Shin]

Comment on attachment 156710 [details] [diff] [review]
patch v2 (include bookmarksManager.js)

asking for r/sr

Comment 7

[jag (Peter Annema)]

Comment on attachment 156710 [details] [diff] [review]
patch v2 (include bookmarksManager.js)

sr=jag

Comment 8

[Jungshik Shin]

fix checked into the trunk.
i'll ask for aviary-1.0 and 1.7 branch approval after testing with branch trees
which I don't have handy at the moment

Comment 9

[[e-mail]]

Comment on attachment 156710 [details] [diff] [review]
patch v2 (include bookmarksManager.js)

>+            nsXPIDLString bookmarksFile;
>+            prefVal->ToString(getter_Copies(bookmarksFile));
Nit: I think using an nsString would be more efficient here, as GetData would
only share the internal buffer rather than copying it.

Comment 10

[Jungshik Shin]

You meant it'd be more efficient this way ?

  nsString bookmarksFile;
  prefValue->GetData(bookmarksFile); 

http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsSupportsPrimitives.cpp#158

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #9)
> Nit: I think using an nsString would be more efficient here, as GetData would
> only share the internal buffer rather than copying it.

But use nsAutoString, which is more efficient if the function's impl does not do
string sharing

> You meant it'd be more efficient this way ?

yes.

Comment 12

[Jungshik Shin]

Attachment: patch v3 (with GetData)

Per Neil's and Christian's comment. I've already checked in the difference
between attachment 156710 [details] [diff] [review] and this patch. I'm uploading this to get a1.7 and
a-aviary1.0

Comment 13

[Jungshik Shin]

Comment on attachment 158632 [details] [diff] [review]
patch v3 (with GetData) 

carrying over r/sr and asking for a1.7 and a-aviary

This patch (sans GetData()) on the trunk was already verified by a Korean user. 
English-speaking users wouldn't be affected at all. Non-English speaking users
with their bookmarks file at a custom location (with non-ASCII characters)
would be happy to see this fixed.

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 158632 [details] [diff] [review]
patch v3 (with GetData) 

a=asa (on behalf of the aviary team and drivers) for checkin to the aviary
branch and the 1.7 branch.

Comment 15

[Asa Dotzler [:asa]]

This bug seems to have an aviary branch checkin associated with it. If this has
landed on the aviary branch (as much as it's going to for 1.0) can you please
add the "fixed-aviary1.0" keyword? Thanks.

Comment 16

[Jungshik Shin]

Adding keywords for branch landing