371309 - Migration Wizard explicitly sets a homepage which is wrong when using a distribution

Status: RESOLVED FIXED

(Firefox :: Migration, defect, P1)

Description

[Basil Hashem [:baz]]

The desired behavior is for the migration wizard to set the homepage to the
build default (which may have been overridden by an extension, such as a partner branding DEX).

Note that this includes changing the text in the wizard to an appropriate string.

Comment 1

[Benjamin Smedberg]

The solution to this bug involves

1) changing the homepage migration text to say something generic "Use the Firefox start page", instead of the current specific "Use Firefox start page provided by XXX".

2) Making the import wizard not set a homepage pref (a userpref) in that case, but use whatever default pref is appropriate.

Comment 2

[Benjamin Smedberg]

Attachment: Use clearUserPref to select the default homepage, instead of setting a userpref, rev. 1

Gavin, cbeard/pkim/mconnor are going to come up with a final string to use here, but I'd like to go ahead and land this and get testing on trunk.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 256661 [details] [diff] [review]
Use clearUserPref to select the default homepage, instead of setting a userpref, rev. 1

>Index: browser/components/migration/content/migration.js

>-    if (!pageTitle || !pageDesc || !startPages || startPages < 1)
>+    if (!pageTitle || !pageDesc )
>       this._wiz.advance();

This is a separate issue (and perhaps can go in another bug if you wish), but shouldn't this also return? Once we advance we don't need to worry about the rest of the page setup, and not doing so means we end up calling onHomePageMigrationPageAdvanced more than once. This hasn't caused any problems because 1) this case is never hit, in practice and 2) that whole function is wrapped in a try/catch, but it's pretty clearly incorrect, as far as I can tell.

>+    numberOfChoices++;

You can get rid of this and just consolidate the last two if/then blocks in this function.

It looks like you forgot to remove the radio buttons from migration.xul.

Otherwise I think this looks fine, though I haven't tested it.

Comment 4

[Benjamin Smedberg]

Attachment: Use clearUserPref to select the default homepage, instead of setting a userpref, rev. 1.1

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 256840 [details] [diff] [review]
Use clearUserPref to select the default homepage, instead of setting a userpref, rev. 1.1

>Index: browser/components/migration/content/migration.js

>   // 4 - Home Page Selection
>   onHomePageMigrationPageShow: function ()
>   {
>     // only want this on the first run
>     if (!this._autoMigrate)
>       this._wiz.advance();

Another case of a missing return :(

>     try {
>       var bundle = document.getElementById("brandBundle");
>       var pageTitle = bundle.getString("homePageMigrationPageTitle");
>       var pageDesc = bundle.getString("homePageMigrationDescription");
>-      var startPages = bundle.getString("homePageOptionCount");
>     } catch(ex) {}
> 
>-    if (!pageTitle || !pageDesc || !startPages || startPages < 1)
>+    if (!pageTitle || !pageDesc ) {
>       this._wiz.advance();
>+      return;
>+    }

Actually, now that you're making homePageSingleStart mandatory, you might as well do the same for homePageMigrationPageTitle and homePageMigrationDescription and remove these checks and the try/catch, no?

>     var i, mainStr, radioItem, radioItemId, radioItemLabel, radioItemValue;

"mainStr" is the only one of these that is still used.

>       oldHomePage.removeAttribute("hidden");

You can get rid of this line, and the corresponding hidden="true" attribute in the XUL file.

>   onHomePageMigrationPageAdvanced: function ()

>       if (radioGroup.selectedItem.id == "homePageMultipleStartMain")

You can remove this line and the next, since you've removed the homePageMultipleStartMain element.

>+            if (this._newHomePage == "DEFAULT") {
>+              try {
>+                prefBranch.clearUserPref("browser.startup.homepage");
>+              }
>+              catch (e) { }
>+            }

I wasn't sure why we even needed to clear the user pref here, since when this runs on the first start there should be no value to clear, but I had forgotten about -migration. Unfortunately, I couldn't get this to work; running an existing profile with "-migration" and selecting "Firefox start" doesn't seem to have any effect (doesn't reset the homepage). Shouldn't it?

Comment 6

[Benjamin Smedberg]

> I wasn't sure why we even needed to clear the user pref here, since when this
> runs on the first start there should be no value to clear, but I had forgotten
> about -migration. Unfortunately, I couldn't get this to work; running an

Actually -migration (or the Import menu item) are not the issue. The issue is that the mozilla-based profile migrators may copy prefs.js from the other app's profile during import. This means that there may be a homepage pref set that we need to clear.

I'll fix the other cases and resubmit.

Comment 7

[Benjamin Smedberg]

Attachment: Use clearUserPref to select the default homepage, instead of setting a userpref, rev. 1.2

I've kept the hidden="true" and removeAttribute("hidden") bits because without them, radio selection was all screwed up. I'm not sure why.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 256948 [details] [diff] [review]
Use clearUserPref to select the default homepage, instead of setting a userpref, rev. 1.2

(In reply to comment #6)
> Actually -migration (or the Import menu item) are not the issue. The issue is
> that the mozilla-based profile migrators may copy prefs.js from the other
> app's profile during import. This means that there may be a homepage pref set
> that we need to clear.

I guess I wasn't able to hit that case in testing, then (I tested importing a SeaMonkey profile, from scratch and from -migration). Either way I guess it can't really hurt.

(In reply to comment #7)
> I've kept the hidden="true" and removeAttribute("hidden") bits because without
> them, radio selection was all screwed up. I'm not sure why.

Strange bug indeed - I wasn't able to figure out the root cause, but luckily it seems to be fixed by the patch in bug 371260. Followup bug to clean this up, maybe?

You could also move this dialog's strings to a DTD now since they're no longer dynamic, but that's probably more trouble than it's worth at this point (though probably also followup-worthy).

Comment 9

[Benjamin Smedberg]

Fixed on trunk, with the string from bug 372409. Followup on the radio selection issues is bug 372733. I didn't file a followup about DTDifiation, because I believe Axel's l20n work will make that moot.

Comment 10

[Benjamin Smedberg]

Attachment: Fix builds with generic branding, rev. 1

The homepage migration strings are still optional, because they don't exist in builds without official branding.

Comment 11

[Benjamin Smedberg]

Attachment: Branch rollup patch, rev. 1

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 257411 [details] [diff] [review]
Branch rollup patch, rev. 1

let's get this in on 1.8 branch

Comment 13

[Benjamin Smedberg]

Fixed on MOZILLA_1_8_BRANCH. Couldn't mark the fixed1.8.1.4 keyword because it doesn't exist yet.

Comment 14

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.4 using 1.8 branch nightly builds on XP and Vista:

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.4pre) Gecko/2007042805 BonEcho/2.0.0.4pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.4pre) Gecko/2007042805 BonEcho/2.0.0.4pre

The Homepage migration wizard don`t exist on this builds, it uses the default Mozilla Homepage when importing settings like from IE7.