191053 - International domain names (IDN) are displayed as garbage in bookmark

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Teruko Kobayashi]

After I visited IDN and marked to bookmark, the International domain names are
displayed as garbage in bookmark.

Steps of reproduce
1. Put the following lines to pref.js
user_pref("network.IDN_prefix", "bq--");
user_pref("network.IDN_testbed", true);
user_pref("network.enableIDN", true);

2. Launch Netscape
3. go to the following IDN from
http://bugzilla.mozilla.org/attachment.cgi?id=112169&action=view

Active Chinese Domain Name
http://南极星.com/

Active Japanese Domain Name
http://www.トナー.com/

Active Korean Domain Name
http://부동산lg.com/

Active German Domain Name
http://internetdomänen.com/

Active Polish Domain Name
http://pasaż.com/

4. After visited each IDN, select menu Bookmarks | Bookmark This Page each time
5. Select menu Bookmarks | Manage Bookmarks..

Actual result
International Domain names are displayed as garbage.

Expected result
International Domain name is displayed in its language.

Tested 1-28 Win32 trunk build on Win98 J.

Comment 1

[Teruko Kobayashi]

Attachment: screen shot.

Only German name is displayed fine.

Comment 2

[Yuying Long]

Confirm this on Mac 10.2, change platforms to all.

The garabage display in location bar but not bookmark.

Comment 3

[Samir Gehani]

i18n triage team: nsbeta1+/adt3

Comment 4

[marina]

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

Comment 5

[Shinichi Hanaki]

Attachment: Proposed patch

Defined URI as PRUnichar* instead of char* to handle multibyte characters.
Please try this one.

Comment 6

[Jungshik Shin]

I'll try the patch. It seems like it should work. 

Comment 7

[Jungshik Shin]

A couple of more changes are necessary. I'm gonna upload a patch against the
trunk. BTW, if anyone want to post international domain names here (for that
matter, any non-ascii characters to any bug in bugzilla), please, make sure to
set 'character coding' in View menu to UTF-8. 

Comment 8

[Jungshik Shin]

Attachment: update

internet search also needs to be updated.

Comment 9

[Jungshik Shin]

Comment on attachment 141332 [details] [diff] [review]
update

asking for r/sr.

thanks for the patch, Hanaki-san.

Comment 10

[Jan Varga [:janv]]

Hmm, it could be AUTF8String, no?

Comment 11

[Jungshik Shin]

It saves us little. At some point, we have to convert UTF-8 to UTF-16. 

Besides, without nsAUTF8String (AUTF8String in IDL is translated to nsACString),
we'd better use PRUnichar*(wstring) or nsAString(AString) for Unicode.
Otherwise, some callers could make a mistake of passing a non-UTF-8 string.

Comment 12

[Jan Varga [:janv]]

I'm not an expert on this, I would ask darin
anyway, the patch looks good r=varga, but ask darin what he thinks about it

Comment 13

[Jungshik Shin]

Thanks. I'm adding darin to CC to ask for his opinion.

Comment 14

[jag (Peter Annema)]

Comment on attachment 141332 [details] [diff] [review]
update

On AUTF8String, someone could make the mistake of passing in non-UTF8 because
they only see the |const nsACString&| in the c++ header, but they should look
at the IDL interface or other documentation to determine what encoding is
expected, as should the reviewer(s).

In this case however, since we have to convert to UTF-16 anyway, I think this
patch is fine.

I would like darin's input on this though.

Comment 15

[Jungshik Shin]

ping darin :-) what do you think?

Comment 16

[jag (Peter Annema)]

Does this fix also fix bug 201050?

Comment 17

[Jungshik Shin]

Yes, it does. 

Comment 18

[Jungshik Shin]

setting the target milestone to 1.7beta

Comment 19

[Jungshik Shin]

Comment on attachment 141332 [details] [diff] [review]
update

varga already gave r, but wanted darin to take a look. To facillate darin's
review, I'm asking him for r.

Comment 20

[Darin Fisher]

Comment on attachment 141332 [details] [diff] [review]
update

r=darin

Comment 21

[Brendan Eich [:brendan]]

nsBookmarkService.cpp is forked under browser -- do we need the same fix there
for firefox?

/be

Comment 22

[Jungshik Shin]

Yes. I realized that 10 minutes ago and was about to fix it, but found that it'd
been already taken care of by ben because it resulted in a bustage in firefox
(nsInternetSearchService was not forked).
Marking as fixed. Thanks all.

Comment 23

[Boris Zbarsky [:bzbarsky]]

This patch fails to build on windows.... (not all callers of
CreateBookmarkInContainer were updated).

Comment 24

[Boris Zbarsky [:bzbarsky]]

The tree has been red for two hours (granted, the first hour of that was from a
different patch), jshin is unreachable (past the "I'm working on this" comment
he made 20 minutes ago), and in my opinion any fixes to the bustage would be
somewhat nontrivial and at least require review.

Per agreement on #developers, patch backed out, along with the firefox bustage
fixes it required.

Comment 25

[Jungshik Shin]

Hey, I was watching the tree. The bustage fix is trivial. I'm not so happy with
your backing this out. 

Comment 26

[Boris Zbarsky [:bzbarsky]]

Jungshik, we tried to contact you, unsuccessfully, for about an hour.  Watching
the tree implies being contactible by others so they have some idea of what's
going on.  That means either being on irc or having a listed way to get at you
via aim or responding to questions on tinderbox, or _something_.

Once you have that fix, relanding is trivial -- just back out my backout and
then check in your fix.

And if fixing ResolveShortcut is in fact trivial (as it may be; my knowledge of
Win32 apis is not so hot) I apologize for not knowing how to do it; had I known
how, I would have done it.

Comment 27

[Jungshik Shin]

Boris, I apologzie to you for overreacting and causing the bustage in the first
place. It just happens that I've never been fond of any form of instant
messaging (including IRC), but I did add a comment to tinderbox that I'd fix it
soon. Then, I spotted another bustage in the migration code for Opera on
firefox.  Nonetheless, I could have been more responsive and in the future, I'll
make sure to be on #mozilla after landing a patch.
As for IUniformResourceLocator::GetURL(), MSDN doesn't say anything about the
encoding of URL, but I'm 99% sure that it's not a legacy multibyte encoding
because it's a part of a dll that comes with MS IE.  Only two remaining
possibilities are ASCII (escaped) or UTF-8. For both possibilities, we can treat
it as UTF-8(a little bit of perf. loss if it's always ASCII. When MSIE is
updated - when?? - to support IDN, it may become UTF-8, though.)
 
  

Comment 28

[Boris Zbarsky [:bzbarsky]]

Note that Ben had checked in the Opera migration bustage, as far as I can tell
(it was part of what I backed out).  Or was there other bustage too?

If you need sr for the updates to the patch, feel free to ask me (though it will
have to happen tomorrow morning).  I love reviewing code that depends on
undocumented apis.  ;)

Comment 29

[Jungshik Shin]

Attachment: what's backed out by bz(= 141332 + ben's firefox checkins) + Windows patch

What's new in this patch is two line-changes  (+ rather verbose comments) in
xpfe/components/bookmarks/src/nsBookmarksService.cpp to fix the Windows
bustage.
Everything else (my patch + ben's checkins except for my comment) was in the
cvs before. I'd just check this in if I can monitor the tree for the next two
hours (or I could build on Windows. Somehow, my Windows build is failing in
jpeg/ with an obscure error message from VC++ 6). I have to run now. bz, if
you're still around, feel free to check this in.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 143370 [details] [diff] [review]
what's backed out by bz(= 141332 + ben's firefox checkins) + Windows patch 

sr=bzbarsky for those two changes.

Jungshik, if you don't get to checking this in by this afternoon I'll do it for
you.

Comment 31

[Bernd]

backout the patch in bug 125762 see 
http://bugzilla.mozilla.org/show_bug.cgi?id=125762#c54 
and your build will run.
<rant>
Sheriff is #mozilla.

means  IMHO that if you turn the tree red you should explain to the sheriff
whats going on, regardless  how you like irc. 

from http://mozilla.org/hacking/bonsai.html

If you are on the hook, you are findable. You are either at your desk, or
pageable, checking e-mail constantly, or on IRC so that you can be found
immediately and can respond to any problems in your code.

When you did comment the tree was red already for a long enough time, given also
the fact that ben needed to fix two files on ff.
</rant>

Comment 32

[Jungshik Shin]

(In reply to comment #31)
> backout the patch in bug 125762 see 
> http://bugzilla.mozilla.org/show_bug.cgi?id=125762#c54 
> and your build will run.

Thanks, but I've already figured out how to fix it.

> <rant>

You're free to say whatever you want to say... It's unfortunate that you write
to me personally when I was monitoring my bugmail box. 

Comment 33

[Jungshik Shin]

Resolving as fixed for real. Thank you all.

Comment 34

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

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