Closed Bug 111428 Opened 18 years ago Closed 14 years ago

If a folder name ends '\' or '/' in IE Favorites, the folder cannot be imported

Categories

(NSPR :: NSPR, defect)

x86
Windows 2000
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mentin, Assigned: masayuki)

References

()

Details

(4 keywords, Whiteboard: [tjp-dl])

Attachments

(2 files, 5 obsolete files)

Repro:
1. Open IE, open page http://www.computerra.ru, bookmark it, close IE
2. Open Mozilla, click Bookmarks\Imported IE Favorites

Result: bookmark is not there
Western-character-only bookmarks are OK

Expected: Bookmark displayed
*** Bug 112533 has been marked as a duplicate of this bug. ***
Severity: minor → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
mass reassign of pchen bookmark bugs to ben
Assignee: pchen → ben
Status: NEW → ASSIGNED
Target Milestone: --- → Future
Blocks: 120814
This appears to be fixed. See bug 122816 for a related problem.

Can we resolve this as worksforme?
Keywords: intl
Old Summary: IE bookmarks containing non-western characters are not exported

New Summary: IE favorites with non-western characters are not imported
Summary: IE bookmarks containing non-western characters are not exported → IE favorites with non-western characters are not imported
*** Bug 140362 has been marked as a duplicate of this bug. ***
i just want to add some comments from bug # 140362: this problem occurs when the
browser charset is different from system locale, e.g: japanese bookmarks would
be imported/displayed on the japanese system, as well as russian on the cyrillic
locale.
*** Bug 174734 has been marked as a duplicate of this bug. ***
Not fixed for me. I have an IE6 favourite with partly Hebrew characters in
Win2000 western setup. There are two separate issues here:

1) The bookmark doesn't appear at all under "imported IE favorites".

2) I exported my IE favourites to a file and reimported to Mozilla. The bookmark
now appears but with Hebrew characters replaced by ?'s. But the ?'s are in the
exported file, so this one is IE's problem.
Mass reassign of my non-Firefox bugs to ben_seamonkey@hotmail.com
Assignee: bugs → ben_seamonkey
Status: ASSIGNED → NEW
Can confirm this. The bookmark´s description and title is using cyrillic characters.
A bookmark with German umlauts works instead. (Happy me)


Another problem with importing/editing bookmarks in FF:
You can´t delete the personal tollbar folder. If you import bookmarks from
Mozilla   you will probably have two of those folders then. You are only able to
delete the imported folder.

Several times importing IE Favorites creates folder "imported IE favorites"
several times.
It would be great if this was fixed.. I had a large selection of bookmarks in
IE, and when I switched to Firefox lots of them weren't imported correctly..
and, its not just non-western characters, some characters such as quotation
marks and others similar to that are the same way.
L10N is the localized builds right? so this should definately blocking them i'm
going to request it to block that build.
Flags: blocking-aviary1.0-L10N?
Flags: blocking-aviary1.0mac?
Flags: blocking-aviary1.0?
I think it's very important that this bug is fixed, especially because a alot of
people who will be switching from IE will want to have all there bookmarks imported.
This is a Mozilla bug and not a Firefox bug.
Flags: blocking-aviary1.0mac?
Flags: blocking-aviary1.0mac-
Flags: blocking-aviary1.0?
Flags: blocking-aviary1.0-L10N?
Flags: blocking-aviary1.0-L10N-
Flags: blocking-aviary1.0-
Product: Browser → Seamonkey
Asa-

See comment #7.  You duped a firefox bug to this bug yet in comment #14 you say
"This is a Mozilla bug and not a Firefox bug."  which is it?
AIUI, there's different code doing this in Seamonkey and Firefox, and this was
clearly a Seamonkey bug. I'll undupe the duplicate bug and bug 174734 can cover
this issue for Firefox.
Flags: blocking-aviary1.0mac-
Flags: blocking-aviary1.0-L10N-
Flags: blocking-aviary1.0-
Assignee: ben_seamonkey → p_ch
QA Contact: claudius → bookmarks
Component: Bookmarks → Internationalization
Product: Mozilla Application Suite → Core
Summary: IE favorites with non-western characters are not imported → If a folder ends '\' or '/' in IE Favorites, the folder cannot be imported
Target Milestone: Future → mozilla1.8.1
Assignee: p_ch → smontagu
QA Contact: bookmarks → amyy
Attached patch Patch rv1.0 (obsolete) — Splinter Review
This is a NSPR bug. It is not checking the DBCS lead byte.
Assignee: smontagu → masayuki
Status: NEW → ASSIGNED
Attachment #211114 - Flags: superreview?(darin)
Attachment #211114 - Flags: review?(jshin1987)
Summary: If a folder ends '\' or '/' in IE Favorites, the folder cannot be imported → If a folder name ends '\' or '/' in IE Favorites, the folder cannot be imported
As far as xpcom is concerned, this doesn't matter any more if bug 162361 is fixed. Nonetheless, this needs to be fixed in NSPR.
Component: Internationalization → NSPR
Product: Core → NSPR
Target Milestone: mozilla1.8.1 → ---
Version: Trunk → 3.0
Comment on attachment 211114 [details] [diff] [review]
Patch rv1.0

r=jshin
I'm not so fond of 'IsChar', but I don't have any better name.
Attachment #211114 - Flags: review?(wtchang)
Attachment #211114 - Flags: review?(jshin1987)
Attachment #211114 - Flags: review+
Comment on attachment 211114 [details] [diff] [review]
Patch rv1.0

Sorry. This patch has a problem. I get the feedback.
Attachment #211114 - Flags: superreview?(darin)
Attachment #211114 - Flags: review?(wtchang)
Attachment #211114 - Flags: review-
Attached patch Patch rv2.0 (obsolete) — Splinter Review
On previous patch, if the last character is '\' and its previous character's second byte is in leading byte range, it is not working fine. This patch may fix the problem.
Attachment #211114 - Attachment is obsolete: true
Attachment #211138 - Flags: review?(jshin1987)
Attachment #211138 - Flags: review?(wtchang)
Comment on attachment 211138 [details] [diff] [review]
Patch rv2.0

Thanks a lot for your patch.  Please attach a new
patch with the following changes.

1. "primpl.h" already includes <windows.h>, so we
don't need to include <windows.h> again.

2. In NSPR source code, the indentation offset is
4, not 2 (the indentation offset of Mozilla source
code).

3. Is it possible to use functions in <mbstring.h>
so we can avoid linking with user32.lib?  Should we
replace CharPrevA in this patch by _mbsdec?

We are using two <mbstring.h> functions (_mbsinc and
_mbspbrk) in ntio.c and w95io.c.  Should we replace
_mbsinc by CharNextA?  But I can't find a replacement
for _mbspbrk.

4. Modify IsLastChar to take another 'const char *'
argument, pass 'filename + len' or 'fn + len' (or equivalently, '&filename[len]' or '&fn[len]') as
that argument.  The function would be renamed
IsPrevChar.

A further optimization is to rename the function
IsPrevCharSlashBackslash and hardcode the comparisons
with '/' and '\\' in the function.
Attachment #211138 - Flags: review?(wtchang) → review+
Attached patch Patch rv3.0 (obsolete) — Splinter Review
Attachment #211138 - Attachment is obsolete: true
Attachment #211232 - Flags: review+
Attachment #211138 - Flags: review?(jshin1987)
Attachment #211232 - Flags: review?(jshin1987)
Comment on attachment 211232 [details] [diff] [review]
Patch rv3.0

Kimura-san:

I changed |CharPrevA| to |_mbsdec|, is it right?
Attachment #211232 - Flags: review?(VYV03354)
Comment on attachment 211232 [details] [diff] [review]
Patch rv3.0


there are three other places with a similar problem in w95io/ntio.c. I'll upload a patch that should be applied on top of this patch. Why don't you combine it with yours?
Attachment #211232 - Flags: review?(jshin1987)
Attached patch additional patch (obsolete) — Splinter Review
Masayuki, I just realized that you had metamorphosed this bug into something different from what's originally reported. What's originally reported was basically the same as bug 174734 (aside from firefox vs seamonkey difference) and has little to do with NSPR. I'm not saying we should not fix  the NSPR bug being fixed now, but you should have filed a new bug for that. 


Comment on attachment 211232 [details] [diff] [review]
Patch rv3.0

Looks good except that you should check whether aCurrent is greater or equal to aStr + 1 first.
Attachment #211232 - Flags: review?(VYV03354) → review+
Comment on attachment 211232 [details] [diff] [review]
Patch rv3.0

Sorry, please replace of the following phrase:
> aCurrent is greater or equal to aStr + 1
by:
whether aCurrent is greater than aStr
(In reply to comment #27)
> Masayuki, I just realized that you had metamorphosed this bug into something
> different from what's originally reported. What's originally reported was
> basically the same as bug 174734 (aside from firefox vs seamonkey difference)
> and has little to do with NSPR. 

If you are right, our report(bug 112533) should not be duped to this bug...
See comment 1.
(In reply to comment #30)
> (In reply to comment #27)
> > Masayuki, I just realized that you had metamorphosed this bug into something
> > different from what's originally reported. 
> If you are right, our report(bug 112533) should not be duped to this bug...
> See comment 1.

Your  fix doesn't fix what's reported orignally but it only fixes bug 112533. What more do you need to be convinced about what I wrote? 

The fact that bug 112533 was *wrongly* duped to this bug  doesn't make the orignal report the same as what you're fixing (which is bug 112533). bug 112533 should NOT have been duped to this bug. All other comments (4, 6, 8, 10 through 16, etc) are about not being able to import IE favorites whose names include characters outside the current default code page (i.e. Arabic in Japanese Windows, Hebrew in French Windows). 



(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #27)
> > > Masayuki, I just realized that you had metamorphosed this bug into something
> > > different from what's originally reported. 
> > If you are right, our report(bug 112533) should not be duped to this bug...
> > See comment 1.
> 
> Your  fix doesn't fix what's reported orignally but it only fixes bug 112533.
> What more do you need to be convinced about what I wrote? 
> 
> The fact that bug 112533 was *wrongly* duped to this bug  doesn't make the
> orignal report the same as what you're fixing (which is bug 112533). bug 112533
> should NOT have been duped to this bug. All other comments (4, 6, 8, 10 through
> 16, etc) are about not being able to import IE favorites whose names include
> characters outside the current default code page (i.e. Arabic in Japanese
> Windows, Hebrew in French Windows). 
> 

Yeah, but what should we do?

reopening bug 112533 -> fixed and this bug being dupped to bug 174734?
Or marking to fixed this bug is same as bug 112533?
Attached patch Patch for check-in (obsolete) — Splinter Review
Attachment #211232 - Attachment is obsolete: true
Attachment #211244 - Attachment is obsolete: true
O.K. I filed bug 326544 for original report of this bug.
This bug should be closed, because this has many non-related attachments.
Wan-Teh Chang:

Oh, I cannot check-in to NSPR. Could you check-in attachment 211259 [details] [diff] [review]? That cleared my build test.
Whiteboard: [needs check-in]
Comment on attachment 211259 [details] [diff] [review]
Patch for check-in

This patch looks good.  The reason I gave it review- is
the following change:

>-        if (!_PR_IS_SLASH(pathbuf[len - 1])) {
>+        if (IsPrevChar(pathbuf, pathbuf + len, '\\') ||
>+            IsPrevChar(pathbuf, pathbuf + len, '/')) {

I also found that we always make two IsPrevChar calls
together:

  IsPrevChar(..., '/') || IsPrevChar(..., '\\')

so we should combine the two calls into one function. I
will attach a new patch.
Attachment #211259 - Flags: review-
1. Fix a bug in jshin's "additional patch" (the ! in
front of _PR_IS_SLASH(pathbuf[len - 1]) was lost in
the new code).

2. Combine the paired IsPrevChar calls into the
IsPrevCharSlash function.

3. Other minor coding style changes.  One of them is
to change the _mbspbrk call back to the do-while loop.
This is a matter of personal preference.  The reason
I like the do-while loop better is that there is another
same do-while loop (with the comment "look for the final
slash") right below, so it's good to have some symmetry.
Attachment #211259 - Attachment is obsolete: true
Attachment #212170 - Flags: superreview?(jshin1987)
Attachment #212170 - Flags: review?(masayuki)
Comment on attachment 212170 [details] [diff] [review]
Patch for check-in, v2

Thanks. r=me.
Attachment #212170 - Flags: review?(masayuki) → review+
Comment on attachment 212170 [details] [diff] [review]
Patch for check-in, v2

Thanks for catching my mistake and fixing it. 
I'm not a superreviewer. Passing it to Darin.
Attachment #212170 - Flags: superreview?(jshin1987)
Attachment #212170 - Flags: superreview?(darin)
Attachment #212170 - Flags: review+
Comment on attachment 212170 [details] [diff] [review]
Patch for check-in, v2

sr=darin

be sure to land this on the NSPR trunk as well as on the NSPRPUB_PRE_4_2_CLIENT_BRANCH, which the mozilla trunk uses.
Attachment #212170 - Flags: superreview?(darin) → superreview+
I checked in the patch on the NSPR trunk (NSPR 4.7) and
the NSPRPUB_PRE_4_2_CLIENT_BRANCH (mozilla 1.9 alpha).

Checking in ntio.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/ntio.c,v  <--  ntio.c
new revision: 3.42; previous revision: 3.41
done
Checking in w95io.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w95io.c,v  <--  w95io.c
new revision: 3.28; previous revision: 3.27
done

Checking in ntio.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/ntio.c,v  <--  ntio.c
new revision: 3.36.2.6; previous revision: 3.36.2.5
done
Checking in w95io.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w95io.c,v  <--  w95io.c
new revision: 3.22.4.5; previous revision: 3.22.4.4
done
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.7
Comment on attachment 212170 [details] [diff] [review]
Patch for check-in, v2

Thank you.

Wan-Teh:

If we don't have any regression on Trunk, I hope that this will go to 1.8 branch and 1.8.0 branch too. Is it possible?
Attachment #212170 - Flags: approval-branch-1.8.1?(wtchang)
Attachment #212170 - Flags: approval-branch-1.8.1?(wtchang) → approval-branch-1.8.1+
I checked in this patch on the NSPR_4_6_BRANCH (NSPR 4.6.2)
and MOZILLA_1_8_BRANCH (Mozilla 1.8.1).

This bug may not be critical enough for the MOZILLA_1_8_0_BRANCH.
I don't know what kind of non-security fixes can go into that
branch.
Whiteboard: [needs check-in]
Target Milestone: 4.7 → 4.6.2
Comment on attachment 212170 [details] [diff] [review]
Patch for check-in, v2

> This bug may not be critical enough for the MOZILLA_1_8_0_BRANCH.
> I don't know what kind of non-security fixes can go into that
> branch.

This should go to 1.8.0.2, because this problem is critical for Japanese marketing.
By shift_jis encoding, '表' has 0x5c('\') on its second byte. This character is often used on tail of the text.
Attachment #212170 - Flags: approval1.8.0.2?
Comment on attachment 212529 [details] [diff] [review]
Patch for MOZILLA_1_8_BRANCH

approved for 180 branch, a=dveditz for drivers
Attachment #212529 - Flags: approval1.8.0.2+
Flags: blocking1.8.0.2+
Attachment #212170 - Flags: approval1.8.0.2?
Whiteboard: [needs check-in to 1.8.0 branch]
I checked in the "patch for MOZILLA_1_8_BRANCH" on the
MOZILLA_1_8_0_BRANCH for Mozilla 1.8.0.2.
Keywords: fixed1.8.0.2
Whiteboard: [needs check-in to 1.8.0 branch]
Thank you for your work!
Whiteboard: [tjp-dl]
You need to log in before you can comment on or make changes to this bug.