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

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[(not reading, please use [e-mail] instead)]

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

Comment 1

[(not reading, please use [e-mail] instead)]

taking, per discussion with dietrich

Comment 2

[(not reading, please use [e-mail] instead)]

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?

Comment 3

[Dietrich Ayala (:dietrich)]

(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.

Comment 4

[(not reading, please use [e-mail] instead)]

> 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.)

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: v1

tested w/ no profiles and migrating from Safari, and it worked.

still need to test:

- Camino
- Mac IE
- IE
- Seamonkey
- Opera

Comment 6

[Dietrich Ayala (:dietrich)]

(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!

Comment 7

[Dietrich Ayala (:dietrich)]

(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.

Comment 8

[(not reading, please use [e-mail] instead)]

Attachment: v2, but more is needed

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

Comment 9

[(not reading, please use [e-mail] instead)]

Attachment: personal toolbar issue after migration

Comment 10

[Dietrich Ayala (:dietrich)]

yes, safari migration is also adding a new toolbar folder and setting it as such.

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: v3

fixes the double toolbar problem.

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: v3 (no win carriage returns)

Comment 13

[(not reading, please use [e-mail] instead)]

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)

Comment 14

[Dietrich Ayala (:dietrich)]

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

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Aakash Desai [:aakashd]]

I was about to create these test cases on litmus, but found that they were already updated!

For 3.1 test run,
https://litmus.mozilla.org/show_test.cgi?id=6027
https://litmus.mozilla.org/show_test.cgi?id=6028
https://litmus.mozilla.org/show_test.cgi?id=6029
https://litmus.mozilla.org/show_test.cgi?id=6030

For 3.0 test run,
https://litmus.mozilla.org/show_test.cgi?id=4769
https://litmus.mozilla.org/show_test.cgi?id=4770
https://litmus.mozilla.org/show_test.cgi?id=4771
https://litmus.mozilla.org/show_test.cgi?id=4772

Thanks to whoever did that.

Comment 18

[Gervase Markham [:gerv]]

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