382459 - Only first tab of multitab homepage is migrated (xpfe -> suiterunner migration)

Status: RESOLVED FIXED

(SeaMonkey :: Startup & Profiles, defect)

Description

[Tony Mechelynck [:tonymec]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre) Gecko/20070530 SeaMonkey/2.0a1pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre) Gecko/2007052909 SeaMonkey/2.0a1pre 

When transitioning from xpfe to suiterunner, only the first tab of my multitab homepage was migrated.

Reproducible: Didn't try

Steps to Reproduce:
1. In SeaMonkey 1.5a, open 2 or more tabs
2. Edit => Preferences
3. Load on [ Navigator Startup |v], (*) Home Page
4. (Home Page), [ Use Current Group ]
5. [ OK ]
6. Close Sm 1.5a and install Sm 2.0a1.
7. Migrate "from Sm 1.1, Mozilla or Netscape 6" and go through the popups. Do not forget to migrate the home page.
8. A "Welcome" page will congratulate you.
9. Click "Home".

Actual Results:  
Only the first homepage tab loads.


Expected Results:  
All tabs of the homepage should appear just like they were in 1.5a.


Additional info:
In my Sm 1.5 profile, all tabs were listed in prefs.js as the values of "browser.startup.homepage", "browser.startup.homepage.1", etc., to "browser.startup.homepage.28"; and "browser.startup.homepage.count" was set at 29. They were set to a variety of file: http: and about: -protocol pages. Only the first one (a Bugzilla bug page) opened in suiterunner.

Comment 1

[Tony Mechelynck [:tonymec]]

P.S. Not sure under which component to file this bug.

Comment 2

[Tony Mechelynck [:tonymec]]

(In reply to comment #1)
> P.S. Not sure under which component to file this bug.
> 

Oops. Moved to "Profile Manager" component. If Bugzilla doesn't do it, then please, someone with higher privileges than mine should set "Assigned To" and/or "QA Contact" as they should be.

Comment 3

[Mark Banner (:standard8)]

I'll take a look at this soon

Comment 4

[Mark Banner (:standard8)]

Note to self: I probably need to implement bug 371309 at the same time as doing this.

Comment 5

[Mark Banner (:standard8)]

Attachment: Port bug 371309

This ports big 371309 (Migration Wizard explicitly sets a homepage which is wrong when using a distribution) to SeaMonkey, I'm doing it as the first part of fixing this bug. Possibly means there will be some extra work for the complete fix, but it hopefully will make things clearer as to what is actually happening.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 276683 [details] [diff] [review]
Port bug 371309

Discussion with Neil on IRC recommends that if possible we change Default to Current if migrating into an existing profile with the homepage set.

I need to check a few things first. So cancelling review for the  time being.

Comment 7

[Mark Banner (:standard8)]

Moving title around so that I can find the bug easier when I look for it in my lists.

Comment 8

[Mark Banner (:standard8)]

Attachment: Port bug 371309 v2

As comment 5, but additionally checks to see if the target profile already has a homepage or not and changes the displayed text appropriately.

Note I've also changed how the pref copying routines work so that we don't overwrite the existing prefs.js (completely that is) if we're migrating into an existing profile.

Comment 9

[[e-mail]]

Comment on attachment 278297 [details] [diff] [review]
Port bug 371309 v2

This keeps crashing when I try to run it. I don't think layout likes you calling ResetPrefs (and I blame layout, not you).

>-    if (!this._autoMigrate)
>+    if (!this._autoMigrate) {
>       this._wiz.advance();
>+      return;
>+    }
So after this point we can be sure that this._autoMigrate exists, right?

>+    homePageStart.setAttribute("value", "DEFAULT");
Why bother? You check for an empty value anyway. Might as well leave it empty.

>+      this._newHomePage = document.getElementById("homePageRadioGroup")
>+                                  .selectedItem.value;
Actually the radiogroup's value should equal that of its selected item.

>+    if (!this._autoMigrate)
>+      return false;
... which means we don't have to check it again here.

>+    var targetPrefsFile = this._autoMigrate.directory;
This is cloned every time, right?

Comment 10

[[e-mail]]

Comment on attachment 278297 [details] [diff] [review]
Port bug 371309 v2

Oh, and this doesn't actually fix the bug yet does it?

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #9)
> (From update of attachment 278297 [details] [diff] [review])
> This keeps crashing when I try to run it. I don't think layout likes you
> calling ResetPrefs (and I blame layout, not you).

Weird, I don't see any problems with that on Linux.

> >-    if (!this._autoMigrate)
> >+    if (!this._autoMigrate) {
> >       this._wiz.advance();
> >+      return;
> >+    }
> So after this point we can be sure that this._autoMigrate exists, right?

Correct.

> >+    homePageStart.setAttribute("value", "DEFAULT");
> Why bother? You check for an empty value anyway. Might as well leave it empty.

I'd copied it from the FF patch, but you're right, so I've dropped it.

> >+      this._newHomePage = document.getElementById("homePageRadioGroup")
> >+                                  .selectedItem.value;
> Actually the radiogroup's value should equal that of its selected item.

Fixed.

> >+    if (!this._autoMigrate)
> >+      return false;
> ... which means we don't have to check it again here.

Removed.

> >+    var targetPrefsFile = this._autoMigrate.directory;
> This is cloned every time, right?

http://mxr.mozilla.org/seamonkey/source/toolkit/xre/nsXREDirProvider.cpp#758

It implements nsIProfileStartup, so I believe that's the right function.

(In reply to comment #10)
> (From update of attachment 278297 [details] [diff] [review])
> Oh, and this doesn't actually fix the bug yet does it?

Correct, but I think the fix will be a lot simpler now.

Comment 12

[Mark Banner (:standard8)]

Attachment: Port bug 371309 v3 (checked in)

Revised patch based on comments.

Comment 13

[[e-mail]]

(In reply to comment #11)
>I'd copied it from the FF patch
That's always a bad move ;-)

Comment 14

[[e-mail]]

Comment on attachment 278397 [details] [diff] [review]
Port bug 371309 v3 (checked in)

>-        if (this._newHomePage) {
>+        // If _newHomePage is default, don't touch the current pref - 
>+        // then if a new profile it won't be set, if not a new profile
>+        // it'll be left as what the user wanted.
>+        if (this._newHomePage && this._newHomePage != "") {
The extra test is unnecessary. r=me with it reverted.

Comment 15

[Mark Banner (:standard8)]

Attachment: Implement the fix (checked in)

This fixes the bug.

I've dropped nsISuiteProfileMigrator::FORMDATA (as it was obsolete and left over as it was easier) and replaced it by HOMEPAGEDATA. This means we can reuse some of the existing functions to determine if we have home page data or not which makes it hang together better IMHO. It also means we don't have to have the extra "hack" for copying home page data in migration.js, and that home page will now be listed on the list of things that are migrated.

I created CopyHomePageData in nsNetscapeProfileMigratorBase as I'd expect a Firefox migrator to want to use it, and also I didn't feel it right to put the extra pref copies in TransformPreferences - as that is really just copying the core preferences.

Comment 16

[[e-mail]]

Comment on attachment 278405 [details] [diff] [review]
Implement the fix (checked in)

Layout's crashing on me again :-(

>+bool
PRBool

>+nsNetscapeProfileMigratorBase::CopyHomePageData(PRBool aReplace)
Presumably by this point we know we're always going to replace?

Comment 17

[Mark Banner (:standard8)]

(In reply to comment #16)
> (From update of attachment 278405 [details] [diff] [review])
> Layout's crashing on me again :-(
> 
> >+bool
> PRBool

opps.

> >+nsNetscapeProfileMigratorBase::CopyHomePageData(PRBool aReplace)
> Presumably by this point we know we're always going to replace?

Yes, migration.js:

+    if (this._autoMigrate) {
       this._itemsFlags = this._migrator.getMigrateData(this._selectedProfile,
                                                        this._autoMigrate);
+      if (!this._newHomePage)
+        this._itemsFlags &= ~nsISuiteProfileMigrator.HOMEPAGEDATA;
+    }

this._itemsFlags is then passed to nsSeamonkeyProfileMigrator::Migrate(...):

+  if (aItems & nsISuiteProfileMigrator::HOMEPAGEDATA)
+    COPY_DATA(CopyHomePageData, aReplace,
+              nsISuiteProfileMigrator::HOMEPAGEDATA);

Comment 18

[Mark Banner (:standard8)]

Both patches checked in -> fixed.