Remove Windows code from Safari migrator

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Gijs, Assigned: jj, Mentored)

Tracking

Trunk
Firefox 51
Points:
---

Firefox Tracking Flags

(firefox49 wontfix, firefox51 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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

2 years ago
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!
Flags: needinfo?(gijskruitbosch+bugs)
(Reporter)

Comment 2

2 years ago
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(akshat.kedia)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
(Assignee)

Comment 6

2 years ago
Created attachment 8783795 [details] [diff] [review]
v1.diff

How about this one?
Attachment #8783795 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 7

2 years ago
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.
Attachment #8783795 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 8

2 years ago
Created attachment 8783996 [details] [diff] [review]
v2.diff

Updated the patch
Assignee: nobody → jinank94
Attachment #8783795 - Attachment is obsolete: true
Attachment #8783996 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 9

2 years ago
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.
Attachment #8783996 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 10

2 years ago
Created attachment 8784026 [details] [diff] [review]
v3.diff

Updated the patch.
Attachment #8783996 - Attachment is obsolete: true
Attachment #8784026 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 11

2 years ago
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.
Attachment #8784026 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 12

2 years ago
Created attachment 8784033 [details] [diff] [review]
v4.diff
Attachment #8784026 - Attachment is obsolete: true
Attachment #8784033 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 13

2 years ago
Comment on attachment 8784033 [details] [diff] [review]
v4.diff

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

Thanks!
Attachment #8784033 - Flags: review?(gijskruitbosch+bugs) → review+
(Reporter)

Updated

2 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed

Comment 14

2 years ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/fx-team/rev/3f7afc56fae2
Remove Windows code from Safari migrator. r=Gijs
Keywords: checkin-needed
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3f7afc56fae2
https://hg.mozilla.org/mozilla-central/rev/4e9b9e217312
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
status-firefox49: affected → wontfix
You need to log in before you can comment on or make changes to this bug.