Closed Bug 386678 Opened 12 years ago Closed 12 years ago

append the default bookmarks when first run migration from IE, Opera, Safari, etc.

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3 alpha8

People

(Reporter: moco, Assigned: dietrich)

References

Details

(Keywords: regression)

Attachments

(2 files, 3 obsolete files)

append the default bookmarks when first run migration from IE, Opera, Safari, etc.

we should restore the fx 2 behavior of adding the "Get Bookmarks Add-ons" (and other bookmarks, like the "Mozilla Firefox" folder and its bookmarks) on startup-with-no-profiles migration from IE, Opera, Safari, etc.

spun off from bug #381298
Flags: blocking-firefox3?
Blocks: 387137
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 M8
taking, per discussion with dietrich
Assignee: nobody → sspitzer
dietrich had a good idea about this one, still discussing it:

first run migration happens here:

http://lxr.mozilla.org/seamonkey/source/toolkit/xre/nsAppRunner.cpp#2928

Before we call Migrate(), we init our bookmark services, importing the default bookmarks.html for the profile, and then our importer code would append the migrated bookmarks.

but, dietrich is investigating some other changes that might help us get this for free.  from irc:

<dietrich> 1. decouple db creation from bookmarks import
<dietrich> 2. fix the overwrite of default bookmarks by migration
<dietrich> 3. don't init places until later, for Ts

dietrich, any more thoughts on this?
(In reply to comment #2)
> dietrich had a good idea about this one, still discussing it:
> 
> first run migration happens here:
> 
> http://lxr.mozilla.org/seamonkey/source/toolkit/xre/nsAppRunner.cpp#2928
> 
> Before we call Migrate(), we init our bookmark services, importing the default
> bookmarks.html for the profile, and then our importer code would append the
> migrated bookmarks.

Well, we don't want to initialize the default bookmarks there as it's in toolkit, and we want all bookmarks.html-specific code to be in /browser.

I think we could add a method to nsBrowserProfilerMigratorUtils, which imports the default bookmarks.html file. And then have each migrator's CopyBookmarks method call it before migrating the bookmarks (if it's a migration, and not just an import).

Do you mind if i take this bug?

> 
> but, dietrich is investigating some other changes that might help us get this
> for free.  from irc:
> 
> <dietrich> 1. decouple db creation from bookmarks import
> <dietrich> 2. fix the overwrite of default bookmarks by migration
> <dietrich> 3. don't init places until later, for Ts
> 
> dietrich, any more thoughts on this?
> 

After looking into it, those other issues are orthogonal to this fix.
> Do you mind if i take this bug?

not at all, re-assigning to you.  thanks!

> After looking into it, those other issues are orthogonal to this fix.

cool.  if you think those are something we need to worry about, can you log spin off bugs?

your plan sounds good, import the default before calling CopyBookmarks() [or CopyFavorites(), for the win ie migrator]

just a heads up: watch out for the win ie migration code.  specifically, look out for this code which looks like it would remove the migrated items on the personal toolbar folder from the default bookmarks.html:

http://lxr.mozilla.org/seamonkey/source/browser/components/migration/src/nsIEProfileMigrator.cpp#1424
 
according to lxr, the opera and safari importers are the only other importers to use GetToolbarFolder(), but they appear to append bookmarks, unlike the win ie code.

please let me know if you'd like help porting or testing your fix for the ie / opera migrators (as I'm on windows xp and I have both browsers.)
Assignee: sspitzer → dietrich
Attached patch v1 (obsolete) — Splinter Review
tested w/ no profiles and migrating from Safari, and it worked.

still need to test:

- Camino
- Mac IE
- IE
- Seamonkey
- Opera
Attachment #278817 - Flags: review?(sspitzer)
(In reply to comment #4)
> > Do you mind if i take this bug?
> 
> not at all, re-assigning to you.  thanks!
> 
> > After looking into it, those other issues are orthogonal to this fix.
> 
> cool.  if you think those are something we need to worry about, can you log
> spin off bugs?

bug 394205 for decoupling history svc and bookmarks import

bug 364272 for the Ts issue

> 
> your plan sounds good, import the default before calling CopyBookmarks() [or
> CopyFavorites(), for the win ie migrator]
> 
> just a heads up: watch out for the win ie migration code.  specifically, look
> out for this code which looks like it would remove the migrated items on the
> personal toolbar folder from the default bookmarks.html:
> 
> http://lxr.mozilla.org/seamonkey/source/browser/components/migration/src/nsIEProfileMigrator.cpp#1424

thanks, i removed that, so we should import now.

> 
> according to lxr, the opera and safari importers are the only other importers
> to use GetToolbarFolder(), but they appear to append bookmarks, unlike the win
> ie code.
> 
> please let me know if you'd like help porting or testing your fix for the ie /
> opera migrators (as I'm on windows xp and I have both browsers.)
> 

yes please, thanks!
Status: NEW → ASSIGNED
(In reply to comment #5)
> 
> still need to test:
> 
> - Camino

not implemented:

http://mxr.mozilla.org/seamonkey/source/browser/components/migration/src/nsCaminoProfileMigrator.cpp#86

filed bug 394212

> - Mac IE

this failed when trying to get the profile dir in the history service:

    433 // nsNavHistory::InitDBFile
    434 nsresult
    435 nsNavHistory::InitDBFile(PRBool aForceInit)
    436 {
    437   // get profile dir, file
    438   nsCOMPtr<nsIFile> profDir;
    439   nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
    440                                        getter_AddRefs(profDir));
    441   NS_ENSURE_SUCCESS(rv, rv);
          ^^ failed

checking to see if this is related to this patch, or a separate bug.
Attached patch v2, but more is needed (obsolete) — Splinter Review
dietrich, see the change to opera and win ie migrators.  I believe that all callers to InitalizeBookmarks() should do this, so we should just remove the nsIFile* aTargetProfile param and do this work in InitalizeBookmarks().  Note, target is really the source profile (for example, the opera profile we are migrating from).

there will be a v3, at least for ie and opera, because we are ending up with two personal toolbars:  the migrated one from opera / IE is the "real" personal toolbar, and the one from bookmarks.html is also there, and they are not merged.

screen shot coming
Attachment #278817 - Attachment is obsolete: true
Attachment #278817 - Flags: review?(sspitzer)
yes, safari migration is also adding a new toolbar folder and setting it as such.
Attached patch v3 (obsolete) — Splinter Review
fixes the double toolbar problem.
Attachment #278855 - Attachment is obsolete: true
Attachment #278894 - Flags: review?(sspitzer)
Attachment #278894 - Attachment is obsolete: true
Attachment #278897 - Flags: review?(sspitzer)
Attachment #278894 - Flags: review?(sspitzer)
Comment on attachment 278897 [details] [diff] [review]
v3 (no win carriage returns)

r=sspitzer, tested this with win ie and opera first run and import, and everything looks good.  (note existing bug #394266 with opera import)
Attachment #278897 - Flags: review?(sspitzer) → review+
Checking in browser/components/migration/src/nsBrowserProfileMigratorUtils.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsBrowserProfileMigratorUtils.cpp,v  <--  nsBrowserProfileMigratorUtils.cpp
new revision: 1.26; previous revision: 1.25
done
Checking in browser/components/migration/src/nsBrowserProfileMigratorUtils.h;
/cvsroot/mozilla/browser/components/migration/src/nsBrowserProfileMigratorUtils.h,v  <--  nsBrowserProfileMigratorUtils.h
new revision: 1.18; previous revision: 1.17
done
Checking in browser/components/migration/src/nsDogbertProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsDogbertProfileMigrator.cpp,v  <--  nsDogbertProfileMigrator.cpp
new revision: 1.28; previous revision: 1.27
done
Checking in browser/components/migration/src/nsIEProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp,v  <--  nsIEProfileMigrator.cpp
new revision: 1.65; previous revision: 1.64
done
Checking in browser/components/migration/src/nsMacIEProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsMacIEProfileMigrator.cpp,v  <--  nsMacIEProfileMigrator.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in browser/components/migration/src/nsNetscapeProfileMigratorBase.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsNetscapeProfileMigratorBase.cpp,v  <--  nsNetscapeProfileMigratorBase.cpp
new revision: 1.25; previous revision: 1.24
done
Checking in browser/components/migration/src/nsOperaProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsOperaProfileMigrator.cpp,v  <--  nsOperaProfileMigrator.cpp
new revision: 1.70; previous revision: 1.69
done
Checking in browser/components/migration/src/nsPhoenixProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsPhoenixProfileMigrator.cpp,v  <--  nsPhoenixProfileMigrator.cpp
new revision: 1.22; previous revision: 1.21
done
Checking in browser/components/migration/src/nsSafariProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsSafariProfileMigrator.cpp,v  <--  nsSafariProfileMigrator.cpp
new revision: 1.45; previous revision: 1.44
done
Checking in browser/components/migration/src/nsSeamonkeyProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsSeamonkeyProfileMigrator.cpp,v  <--  nsSeamonkeyProfileMigrator.cpp
new revision: 1.37; previous revision: 1.36
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-litmus?
Duplicate of this bug: 387137
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007083003 Minefield/3.0a8pre and during tests with IE and Opera

also with Build 2007083005 on Mac OS X with Safari 2.0.4 (419.3)

During the Migration from this browsers to the Minefield trunk, the default Mozilla bookmarks are append to the migrated bookmarks of the browser.

- Changing to Verified
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus+
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.