1276701 - Remove Windows code from Safari migrator

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[:Gijs (he/him)]

Safari for Windows has been dead for 3.5 years. The code is essentially dead weight, making the Safari migrator code harder to maintain. We should get rid of it.

90% of the changes will be in here:
https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/SafariProfileMigrator.js

removing specialcasing for mac/windows (removing only the if() and braces but keeping the content for mac, and removing the contents as well for windows), doublecheck tests continue to work. We should also remove safari as an option for the Windows-specific codepaths in some of the other files in the migration/ and migration/content directory. Please do ping me if you have questions. :-)

Comment 1

[Akshat Kedia [:aks]]

Hi Gijs,

I will like to work on this bug but before I assign it to myself, I use a Linux Machine, do I need to be a Windows or OS X user to fix and test this bug?

Thanks!

Comment 2

[:Gijs (he/him)]

(In reply to Akshat Kedia [:aks] from comment #1)
> Hi Gijs,
> 
> I will like to work on this bug but before I assign it to myself, I use a
> Linux Machine, do I need to be a Windows or OS X user to fix and test this
> bug?
> 
> Thanks!

Hi Akshat,

Sorry for the delay - I was away until today. While you could technically write the fix on a Linux machine, you couldn't test it on one - you'd need at least one of a Windows or an OS X machine. Maybe you could look into fixing bug 1258797 instead?

Comment 3

[Akshat Kedia [:aks]]

Hi Gijs,

Even for the bug 1258797 won't I need a Windows Machine to test the fix? If not I will like to take it up.

Comment 4

[:Gijs (he/him)]

(In reply to Akshat Kedia [:aks] from comment #3)
> Hi Gijs,
> 
> Even for the bug 1258797 won't I need a Windows Machine to test the fix? If
> not I will like to take it up.

Well, I assumed you wouldn't need a Windows machine, but on closer inspection it seems Linux allows pretty much anything in a filename, and so you wouldn't be able to get the same exception on Linux. :-(

You could potentially look at bug 877856 instead? There's some high-level guidance in bug 877856 comment 2, but I'm happy to give you more advice if you need it. Would that work?

Comment 5

[Akshat Kedia [:aks]]

Super! Sure I can work on that bug. Thanks! I will assign it to myself and let you know there on the comment thread if I need more advice.

Comment 6

[Jinank Jain[:jj]]

Attachment: v1.diff

How about this one?

Comment 7

[:Gijs (he/him)]

Comment on attachment 8783795 [details] [diff] [review]
v1.diff

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

This is on the right track, but there's a bunch more of it that can be removed. Look for all the comparisons with "macosx", and any other mentions in the comments of "OS X" or "Windows"

Finally you should make sure Safari is no longer offered on Windows by removing it here:

http://searchfox.org/mozilla-central/rev/bb22cc4067c3832b943507497389b0b13d6f3a2b/browser/components/migration/MigrationUtils.jsm#34

updating this:

http://searchfox.org/mozilla-central/rev/bb22cc4067c3832b943507497389b0b13d6f3a2b/browser/components/migration/MigrationUtils.jsm#517

removing it here:

http://searchfox.org/mozilla-central/rev/bb22cc4067c3832b943507497389b0b13d6f3a2b/browser/components/migration/content/migration.xul#42

and here:

http://searchfox.org/mozilla-central/rev/bb22cc4067c3832b943507497389b0b13d6f3a2b/browser/components/migration/moz.build#42

http://searchfox.org/mozilla-central/rev/bb22cc4067c3832b943507497389b0b13d6f3a2b/browser/components/migration/moz.build#48

::: browser/components/migration/SafariProfileMigrator.js
@@ -498,4 @@
>      let key;
>      if (this._dict.has("DownloadsPath"))
>        key = "DownloadsPath";
> -    else if (this._dict.has("DownloadPath"))

Here you should also remove the comment above this block. You can also simplify the code:

if (!this._dict.has("DownloadsPath"))
  return;

let downloadsFolder = FileUtils.File(this._dict.get("DownloadsPath"));

Now that the 'key' is always the same there's no real need to abstract it into a local variable anymore.

Comment 8

[Jinank Jain[:jj]]

Attachment: v2.diff

Updated the patch

Comment 9

[:Gijs (he/him)]

Comment on attachment 8783996 [details] [diff] [review]
v2.diff

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

Closer! All of these:

http://searchfox.org/mozilla-central/search?q=macosx&case=false&regexp=false&path=migration%2Fsaf

still need to be updated as well.

Comment 10

[Jinank Jain[:jj]]

Attachment: v3.diff

Updated the patch.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8784026 [details] [diff] [review]
v3.diff

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

Thanks!

r=me with the issues below fixed.

::: browser/components/migration/SafariProfileMigrator.js
@@ +567,4 @@
>  
>  SafariProfileMigrator.prototype.getResources = function SM_getResources() {
>    let profileDir;
> +  profileDir = FileUtils.getDir("ULibDir", ["Safari"], false);

Nit: just unify with the variable declaration, ie:

let profileDir = FileUtils.getDir("ULibDir", ["Safari"], false);

@@ +590,4 @@
>    pushProfileFileResource("ReadingList.plist", Bookmarks);
>  
>    let prefsDir;
> +  prefsDir = FileUtils.getDir("UsrPrfs", [], false);

Same here.

@@ +606,5 @@
>  };
>  
>  SafariProfileMigrator.prototype.getLastUsedDate = function SM_getLastUsedDate() {
>    let profileDir;
> +  profileDir = FileUtils.getDir("ULibDir", ["Safari"], false);

Annnd here.

@@ +623,4 @@
>    get: function get_mainPreferencesPropertyList() {
>      if (this._mainPreferencesPropertyList === undefined) {
>        let file;
> +      file = FileUtils.getDir("UsrPrfs", [], false);

and here.

Comment 12

[Jinank Jain[:jj]]

Attachment: v4.diff

Comment 13

[:Gijs (he/him)]

Comment on attachment 8784033 [details] [diff] [review]
v4.diff

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

Thanks!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/3f7afc56fae2
Remove Windows code from Safari migrator. r=Gijs

Comment 15

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/4e9b9e217312cbff683fd5ac2402eb58c5c370ec
Bug 1276701 - Follow-up to update package-manifest.in after removing the Safari migrator on Windows. r=bustage

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3f7afc56fae2
https://hg.mozilla.org/mozilla-central/rev/4e9b9e217312