Closed
Bug 397768
Opened 17 years ago
Closed 17 years ago
Opera bookmark import loses last bookmark
Categories
(Firefox :: Migration, defect)
Tracking
()
RESOLVED
FIXED
Firefox 3 beta1
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
Details
(Keywords: platform-parity)
Attachments
(2 files)
3.08 KB,
patch
|
mconnor
:
review+
Gavin
:
review+
mconnor
:
approval1.9+
|
Details | Diff | Splinter Review |
1.00 KB,
text/plain
|
Details |
Opera bookmark import loses last bookmark. STEPS TO REPRODUCE 1. start Opera 9.22 and create some bookmarks 2. exit Opera 3. start Firefox with a clean profile 4. File->Import, select "Opera", click Next, Next, Finish ACTUAL RESULTS All bookmarks are imported, except the last one. EXPECTED RESULTS All bookmarks are imported. PLATFORMS AND BUILDS TESTED Bug occurs in Firefox trunk 2007092604 on Linux. Bug occurs in Firefox 2.0.0.7 on Linux.
Assignee | ||
Comment 1•17 years ago
|
||
This fixes it for me on Linux. Not tested on other platforms.
Attachment #282554 -
Flags: review?(mconnor)
Updated•17 years ago
|
Assignee: mats.palmgren → nobody
Component: Bookmarks → Migration
QA Contact: bookmarks → migration
Updated•17 years ago
|
Attachment #282554 -
Flags: review+
Updated•17 years ago
|
Attachment #282554 -
Flags: review?(mconnor)
Attachment #282554 -
Flags: review+
Attachment #282554 -
Flags: approval1.9+
Comment 2•17 years ago
|
||
I can test this on Windows, and land it on the trunk. for "// XXX Todo: |description| is not saved.", I've logged bug #397771
Assignee: nobody → mats.palmgren
Target Milestone: --- → Firefox 3 M9
Comment 3•17 years ago
|
||
I have not yet able to reproduce the original problem on windows, but I can still test the patch. note, since we check if name or url are non-empty, this patch should not hurt windows (if we make another pass once moreData is false).
Status: NEW → ASSIGNED
Comment 4•17 years ago
|
||
mats, can you elaborate on what bookmarks you are creating in opera and where? I'm using Opera on 9.23. Can you attach your opera6.adr file to this bug, or email it to me?
Assignee | ||
Comment 5•17 years ago
|
||
(In reply to comment #4) > can you elaborate on what bookmarks you are creating in opera and where? Using an empty home directory: 1. start Opera 9.22 on Linux (click through licenses etc...) 2. visit http://www.dn.se/ 3. Bookmarks->Bookmark page... click OK in the dialog that opens 4. visit http://svt.se/ 5. Bookmarks->Bookmark page... click OK in the dialog that opens 6. quit The resulting opera6.adr is attached. I tried the same on Windows and compared the opera6.adr files and the only difference I can see is the different line endings, the file ends in \r\n\r\n on Windows vs. \n\n on Linux.
Assignee | ||
Comment 6•17 years ago
|
||
The reason for the different behaviour is deep down in NS_ReadLine. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/base/public/nsReadLine.h&rev=1.8&root=/cvsroot&mark=160,168,169-170,195-196#136 For the last line ending on Windows (\r\n) we will enter the block at line 169, Append an empty string, iterate again and return on line 168 - |*more| will be true in this case, causing an extra call by consumers that loop it's false. This is different from Linux where the last line ending (\n) also enter the block at line 169, Append an empty string, but then exit the loop since we ran out of buffer data, causing a new Read on line 191 (to check for \n\r), it's not, so we return on line 196 with |*more| == false. Thinking about this for a while I don't really see an error with the above, but I guess we could lie (remove line 195) to make it behave the same regardless of which line ending is in use. (alternatively, peek 1 char if we find \r\n at end-of-buffer) BTW, line 167 is pointless, AFAICT.
Keywords: pp
Comment 7•17 years ago
|
||
> BTW, line 167 is pointless, AFAICT.
I think it's an early carryover of an early patch iteration when eolStarted was a member...
As for the rest, we could change this to be consistent, sure. But the consumer here is obviously incorrect before this patch: nsILineInputStream clearly documents that a false return still means you have to process aLine if you want to read all the data.
Comment 8•17 years ago
|
||
On second thought, I do think we should fix the \r\n handling to not effectively pretend that there is another blank line. Mats, you want to do it?
Assignee | ||
Comment 9•17 years ago
|
||
Sure, filed bug 397850.
Assignee | ||
Comment 10•17 years ago
|
||
mozilla/browser/components/migration/src/nsOperaProfileMigrator.cpp 1.72 -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•