If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove messy homepage-set code from migration.js

ASSIGNED
Assigned to

Status

()

Firefox
Migration
ASSIGNED
5 years ago
5 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 637691 [details] [diff] [review]
patch

The code for setting homepage in migration.js is insanely complex. I'm not sure if it was ever needed, but given that all migrators call doStartup very early (excluding the migrator from which you cannot import homepage), and given that my testing shows everything works as expected, I think we can remove it and move on.
Attachment #637691 - Flags: review?(mak77)
Blocks: 768814
Comment on attachment 637691 [details] [diff] [review]
patch

Matt will review this one.
Attachment #637691 - Flags: review?(mak77) → review?(mnoorenberghe+bmo)
Comment on attachment 637691 [details] [diff] [review]
patch

Review of attachment 637691 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/migration/content/migration.js
@@ +309,5 @@
>    onHomePageMigrationPageAdvanced: function ()
>    {
> +    let selectedItem =
> +      document.getElementById("homePageRadiogroup").selectedItem;
> +    if (selectedItem.id != "homePageSingleStart")

The old code was worried about not having a selected item.  Do we know that we are always guaranteed to have one?  If not, also check that selectedItem is truthy here: 
if (selectedItem && selectedItem.id != "homePageSingleStart")

@@ +310,5 @@
>    {
> +    let selectedItem =
> +      document.getElementById("homePageRadiogroup").selectedItem;
> +    if (selectedItem.id != "homePageSingleStart")
> +      this._newHomePage = selectedItem.value;

Nit: I think selectedHomePage would be more clear.

@@ -402,5 @@
> -            var dirSvc = Components.classes["@mozilla.org/file/directory_service;1"]
> -                                   .getService(Components.interfaces.nsIProperties);
> -            var prefFile = dirSvc.get("ProfDS", Components.interfaces.nsIFile);
> -            prefFile.append("prefs.js");
> -            prefSvc.savePrefFile(prefFile);

Why did you get rid of the savePrefFile call?  Are we guaranteed that it will be called after this (in case of an unclean shutdown)?
Attachment #637691 - Flags: review?(mnoorenberghe+bmo) → feedback+
You need to log in before you can comment on or make changes to this bug.