Closed Bug 1276701 Opened 4 years ago Closed 4 years ago

Remove Windows code from Safari migrator

Categories

(Firefox :: Migration, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox49 --- wontfix
firefox51 --- fixed

People

(Reporter: Gijs, Assigned: jj, Mentored)

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 3 obsolete files)

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. :-)
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)
(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)
Attached patch v1.diff (obsolete) — Splinter Review
How about this one?
Attachment #8783795 - Flags: review?(gijskruitbosch+bugs)
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)
Attached patch v2.diff (obsolete) — Splinter Review
Updated the patch
Assignee: nobody → jinank94
Attachment #8783795 - Attachment is obsolete: true
Attachment #8783996 - Flags: review?(gijskruitbosch+bugs)
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)
Attached patch v3.diff (obsolete) — Splinter Review
Updated the patch.
Attachment #8783996 - Attachment is obsolete: true
Attachment #8784026 - Flags: review?(gijskruitbosch+bugs)
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+
Attached patch v4.diffSplinter Review
Attachment #8784026 - Attachment is obsolete: true
Attachment #8784033 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8784033 [details] [diff] [review]
v4.diff

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

Thanks!
Attachment #8784033 - Flags: review?(gijskruitbosch+bugs) → review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/3f7afc56fae2
https://hg.mozilla.org/mozilla-central/rev/4e9b9e217312
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.