302230 - Cannot import bookmark of MacIE written in Japanese

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Hiro]

All Japanese is illegal in migration of MacIE. 

Mac OS X 10.3.9
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050726
Firefox/1.0+

Comment 1

[Hiro]

Attachment: Bookmark menu of MacIE to which Japanese is illegally displayed

Comment 2

[Hiro]

Attachment: Bookmark of MacIE where Japanese is included

Comment 3

[Hiro]

Perhaps, "x-mac-roman" is set to "charset" in Bookmark of MacIE as for any
language version. 

I think that there is a necessity for asking the user the language if it is 
impossible according to an automatic type of the character-code. 

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Taking.

If the META tag declared "x-mac-roman", the user has changed the OS locale to
using English locale or the user is using old version IE. We should ignore old
IE users. Because the user should use latest version browser(for security). And
newer IE works fine on OS's locale. So we should regard the META declaration.
But if the locale is Japan, IE using "x-sjis". But nsBookmarkService.cpp doesn't
allow to use alias. Therefore, we cannot import the bookmark if that is written
in Japanese.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv4.0

This patch fixes this bug.
Current code doesn't allow alias charset name in META tag.
We should allow it for MacIE's bookmark file.

And I'm changing following code:

-	 rv = ParseMetaTag(line, getter_AddRefs(mUnicodeDecoder));
+	 nsCOMPtr<nsIUnicodeDecoder> unicodeDecoder = nsnull;
+	 rv = ParseMetaTag(line, getter_AddRefs(unicodeDecoder));
+	 if (NS_SUCCEEDED(rv) && unicodeDecoder) {
+	     mUnicodeDecoder = unicodeDecoder;
+	 }

We should not set mUnicodeDecoder directly. Because in ParseMetaTag, it is
always initialized by nsnull or any Unicode decoder. This behavior is not good.
Because if it is not declaration of Content-Type, it always returns null. So,
the process is faild.

Comment 6

[Jungshik Shin]

Comment on attachment 191347 [details] [diff] [review]
Patch rv4.0

>
>+    nsCOMPtr<nsICharsetAlias> charsetAlias =
>+        do_GetService(NS_CHARSETALIAS_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCAutoString preferredCharset;
>+    rv = charsetAlias->GetPreferred(charset, preferredCharset);
>+    NS_ENSURE_SUCCESS(rv, rv);
>

You don't need the above.  You can just  use GetUnicodeDecoder in place of
GetUnicodeDecoderRaw below. 

>-        rv = charsetConv->GetUnicodeDecoderRaw(charset.get(), decoder);

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv4.1

Thank you, Jungshik.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 191358 [details] [diff] [review]
Patch rv4.1

I'll review this tomorrow.

Comment 9

[Asaf Romano (gone)]

Comment on attachment 191358 [details] [diff] [review]
Patch rv4.1

I get the same results w/ and w/o this patch, at least with the attached
'testcase' as my ie bookmarks file.

(In any case, you can poke my on moznet, nick: Mano).

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 191358 [details] [diff] [review]
Patch rv4.1

http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2868&action=view
This is default bookmark of MacIE-JA.
Please check it.

Comment 11

[Asa Dotzler [:asa]]

we'd consider a reviewed patch but we're not going to block on this.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: the bookmark of IE written in Japanese(x-sjis)

bugzilla.mozilla.gr.jp server is down now.
If you test the patch, please use this file instead of I pointed URL.

Comment 13

[Asaf Romano (gone)]

Comment on attachment 191358 [details] [diff] [review]
Patch rv4.1

>Index: browser/components/bookmarks/src/nsBookmarksService.cpp
>===================================================================

>     else if ((offset = line.Find(kOpenMeta, PR_TRUE)) >= 0)
>     {
>-        rv = ParseMetaTag(line, getter_AddRefs(mUnicodeDecoder));
>+        nsCOMPtr<nsIUnicodeDecoder> unicodeDecoder = nsnull;
>+        rv = ParseMetaTag(line, getter_AddRefs(unicodeDecoder));
>+        if (NS_SUCCEEDED(rv) && unicodeDecoder) {

Add the following comment here:
// We are not setting mUnicodeDecoder directly since ParseMetaTag set the
// returned decoded to null if this is not a Content-Type declaration.

>+            mUnicodeDecoder = unicodeDecoder;
>+        }
>     }

r=mano.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for check-in

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.