problems when manually importing bookmarks exported from places based bookmark builds (item_id problems and more)

RESOLVED FIXED in Firefox 3 alpha6

Status

()

RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: martijn.martijn, Assigned: moco)

Tracking

({testcase})

unspecified
Firefox 3 alpha6
testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
Created attachment 267796 [details]
bookmarks file to import

See the attached bookmarks file.
When importing that file I would expect a folder to be added with 3 bookmarks.

Actual results:
bm
- http: bookmark3 http://google.com/?1

Expected results:
bm
- data: bookmark1 http://google.com?1
- javascript: bookmark2 http://google.com?2
- http: bookmark3  http://google.com?2
I can't reproduce this on Win XP, latest trunk hourly.
See http://img264.imageshack.us/img264/6664/importpc1.png
(Reporter)

Comment 2

11 years ago
Hmm, yeah, with a new profile, it seems to work. However, it didn't import anything the first three tries. After that, it worked.
Not sure what's going on.
(Reporter)

Comment 3

11 years ago
Ok, it seems that I'm not able to import the bookmarks file at all with a new profile.
But when I've deleted the already existing bookmarks, then I'm able to import the bookmarks file.
I have no idea what's going on.
I see them also with a new profile but with my old set of bookmarks.
import bug + bookmarklets = blocking?
Flags: blocking-firefox3?
> bookmarklets

I misread the bug, these aren't javascript: bookmarks, but bookmarks with javascript: in the title.

but still, import bugs are worth nominating for blocker.
(Reporter)

Comment 7

11 years ago
Well, I only hope you can reproduce any of the problems I got.
I'm having problems, but different that what you are seeing.

Notice in your test case that you have:

            <DT><A HREF="http://google.com?2" ADD_DATE="1179742715" LAST_MODIFIED="1173033841" ITEM_ID="20">javascript: bookmark2</A>
            <DT><A HREF="http://google.com?3" ADD_DATE="1179742715" LAST_MODIFIED="1173033841" ITEM_ID="20">http: bookmark3</A>

Note, the ITEM_IDs are the same.

for me when I have a new profile, importing from your test case I don't get *any* of the bookmarks.

I exited, inspected the places.sqlite file, and they were in there, but I think your test case points out a problem with our import code.

I don't think we can use the imported item_id.  take the scenario that your test case exposes:  what if I export from one firefox 3 profile and re-import into another?

item_id collision can lead to a variety of problems, such as the one you ran into and the one I see with your test case.

I think the fix for this bug is to fix bug #381225 and to ignore ITEM_ID on import.  the side effect of that will be fx 2 parity, as well.

dietrich / mano / mconnor:  comments?
Assignee: nobody → sspitzer
Target Milestone: --- → Firefox 3 alpha6
> dietrich / mano / mconnor:  comments?

dietrich agreed with me over irc that we should stop importing / exporting item_id, and revert to the fx 2 behaviour of import always appending bookmarks.

that will fix this bug.
Status: NEW → ASSIGNED
morphing the summary.  on track to fixing this for A6.  I've got the easy patch to stop exporting item_ids, working on the rest of it.

note I also have to worry about not merging bookmarks into the personal toolbar folder.
Summary: Bookmarks import doesn't work well when title of bookmark contains javascript: → problems when manually importing bookmarks exported from places based bookmark builds (item_id problems and more)
OS: Windows XP → All
Hardware: PC → All
Created attachment 269550 [details] [diff] [review]
fix v1

don't merge bookmarks or folders, only append. in my tests, this makes import behave just like fx2. i think we need to do this for now, due to the data-loss issues of merging. we can tackle merging as a feature later if users ask for it.
Attachment #269550 - Flags: review?(sspitzer)
Created attachment 269678 [details] [diff] [review]
patch v1

oops, duplication of effort over the weekend.  I'll go review dietrich patch.
in fx 2, if you manually import, the manually imported personal toolbar folder gets appended as a "regular" folder.  with your patch, won't the items of the imported personal toolbar folder get merged into the existing personal toolbar folder?

I may try to convince dietrich to review my patch instead, as I think I've fixed that issue, and fixed the problem with our fake icon urls (see bug #380449) and removed the code that exports item_ids, (bug #381225, or that may just be a dup of this bug.)
Blocks: 381225
also note the BOOKMARKSS_MENU part of this fix.  It appears that when this file was moved from mozilla/toolkit/components/places/src/nsBookmarksHTML.cpp to mozilla/browser/components/places/src/nsPlacesImportExportService.cpp, that extra "s" slipped in. 

(compare 1.35 of nsBookmarksHTML.cpp to 1.1 of nsPlacesImportExportService.cpp and see the patch in bug #376008)

The place it really matters is here:

-static const char kBookmarksRootAttribute[] = " BOOKMARKSS_MENU=\"true\"";
+static const char kBookmarksRootAttribute[] = " BOOKMARKS_MENU=\"true\"";

So that should mean that when the trunk was exporting to bookmarks.html (for firefox 2), firefox 2 may have had problems, since there was no BOOKMARKS_MENU attribute.

Additionally, when the trunk was importing it's own bookmarks.html (on schema change), we may have had similar issues, as the import code in nsPlacesImportExportService.cpp was expecting one "s":

#define KEY_BOOKMARKSSMENU_LOWER "bookmarks_menu"

(The rest of the extra "s" changes are internal to the code)
Created attachment 269700 [details] [diff] [review]
updated patch, fix one more PRInt64 cast issue (date)
Attachment #269550 - Attachment is obsolete: true
Attachment #269678 - Attachment is obsolete: true
Attachment #269550 - Flags: review?(sspitzer)
Comment on attachment 269700 [details] [diff] [review]
updated patch, fix one more PRInt64 cast issue (date)

patch needs a little work / cleanup before I seek review.
Attachment #269700 - Attachment is obsolete: true
> firefox 2 may have had problems

I take that back, since fx 2 had no idea about the BOOKMARKS_MENU attribute

> Additionally, when the trunk was importing it's own bookmarks.html 
> (on schema change), we may have had similar issues

I take that back, too.  I'm not seeing us export BOOKMARKS_MENU attribute (or the PLACES_ROOT attribute).

I'll continue testing this patch, and then spin off bugs about the BOOKMARKS_MENU and PLACES_ROOT
Attachment #269711 - Flags: review?(dietrich)
> spin off bugs about the BOOKMARKS_MENU and PLACES_ROOT

see bug #385835

Comment on attachment 269711 [details] [diff] [review]
updated patch

thanks, looks good, r=me.
Attachment #269711 - Flags: review?(dietrich) → review+
fixed.

Checking in nsPlacesImportExportService.cpp;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v
  <--  nsPlacesImportExportService.cpp
new revision: 1.23; previous revision: 1.22
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
again, a test case could be written here to test these changes.

for nsIPlacesImportExportService, we could check that:

a) with "importHTMLFromFile(x, false);", importing appends
b) with "importHTMLFromFile(x, true);", importing replaces
c) exportHTMLToFile() doesn't write out HTML with ITEM_IDs.

In fact, there are a lot of import / export tests that we should be doing.  I'll log a spin of bug on that.
Flags: in-testsuite?
> I'll log a spin of bug on that.

See bug #385859
Flags: blocking-firefox3? → blocking-firefox3+

Comment 24

10 years ago
Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9) Gecko/2008051202 Firefox/3.0

works OK in RC1

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.