886587 - Remove profile migrator

Status: RESOLVED FIXED

(Firefox for Android :: Data Providers, defect)

Description

[:Margaret Leibovic]

It's been almost a year since we shipped Fennec native, I think it's time to retire the XUL profile migrator.

gcp, do you see any reason to keep it around?

Comment 1

[Nick Alexander :nalexander [he/him]]

This will also allow us to remove an ugly bit of Makefile.in -- the logo.png copying.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Not really. I suppose people still stuck on XUL aren't going to get an update :-)

There's a small part that does some moving around of profiles on the sdcard to internal storage, that was probably a more recent bug and something you might have to keep for a bit longer.

Comment 3

[Mark Capella [:capella]]

Recently add, must be preserved / moved https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=878674&attachment=766600

Comment 4

[Mark Capella [:capella]]

Attachment: Patch (v1)

Woo hoo! Hack all the thingz !!

First cut patch ... let me know if I took out something we like :-)

I've left that critical bit from bug 878674 as a standalone module ProfileMigrator.java ... we might could move that into GeckoApp ... we'd have to move the CLEANUP_VERSION prefInt() also ... not sure what to do with any leftover ProfileMigrator.xml file though ... ignore it?



https://tbpl.mozilla.org/?tree=Try&rev=3fa917d366a5

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 775757 [details] [diff] [review]
Patch (v1)

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

You'll need to remove the places.sqlite in the assets dir that the test is using as well.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Also, you need to remove SetupScreen and associated assets, ProfileMigrator is AFAIK the only thing using it.

>not sure what to do with any leftover ProfileMigrator.xml file though

You mean inside shared_prefs? I wouldn't particularly bother personally. Though given that there's still a cleanup pass left over you could also delete them.

Comment 7

[:Margaret Leibovic]

Comment on attachment 775757 [details] [diff] [review]
Patch (v1)

This is looking good to me, but let's get rnewman's feedback on the sync bits. You can also let gcp do the honors of the final review :)

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 775757 [details] [diff] [review]
Patch (v1)

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

Pity we don't already have tests for the cleanup part of migration. Still, what's done is done!

Note that anything under m/a/b/sync needs a corresponding pull req in a-s (and also an import of BrowserContract, which I'll take care of).

Hooray for killing code!

::: mobile/android/base/GeckoProfile.java
@@ +130,5 @@
>              return mDir;
>          }
>  
>          try {
> +            // Check if a profile with this name already exists

Period.

::: mobile/android/base/sync/repositories/android/FennecControlHelper.java
@@ -1,5 @@
> -/* This Source Code Form is subject to the terms of the Mozilla Public
> - * License, v. 2.0. If a copy of the MPL was not distributed with this
> - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> -
> -package org.mozilla.gecko.sync.repositories.android;

Upstream.

::: mobile/android/base/sync/stage/AndroidBrowserBookmarksServerSyncStage.java
@@ -71,5 @@
> -    boolean migrated = FennecControlHelper.areBookmarksMigrated(session.getContext());
> -    if (!migrated) {
> -      Logger.warn(LOG_TAG, "Not enabling bookmarks engine since Fennec bookmarks are not migrated.");
> -    }
> -    return super.isEnabled() && migrated;

Please submit a pull request for this change upstream, too. (Same bug number.)

::: mobile/android/base/sync/stage/AndroidBrowserHistoryServerSyncStage.java
@@ -63,5 @@
>    protected boolean isEnabled() throws MetaGlobalException {
>      if (session == null || session.getContext() == null) {
>        return false;
>      }
> -    boolean migrated = FennecControlHelper.isHistoryMigrated(session.getContext());

Ditto.

Comment 9

[Richard Newman [:rnewman]]

Flagging for QA and documentation.

Comment 10

[Mark Capella [:capella]]

Attachment: Patch (v2)

Ok ... with all the work that rnewman did, and this patch, everything should be taken care of, except (iiuc) some cleanup code into GeckApp's DeferredCleanupTask() to remove the old places.sqlite DB which I thought to do in a follow on patch.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 779719 [details] [diff] [review]
Patch (v2)

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

Please see previous comments. You need to remove SetupScreen and assets, and the places.sqlite in assets that's used for tests (it might be in the assets zipfile).

>some cleanup code into GeckApp's DeferredCleanupTask() to remove the old places.sqlite DB which I thought to do in a follow on patch.

There is no need to do that, because ProfileMigrator will either have already removed it or the user won't have had a previous version anyway. You need to remove the one used for the test you removed.

::: mobile/android/base/GeckoApp.java
@@ +168,5 @@
>      static private final String LOCATION_URL = "https://location.services.mozilla.com/v1/submit";
>  
> +    // Delay before running one-time "cleanup" tasks that may be needed
> +    // after a version upgrade.
> +    private static final int CLEANUP_DEFERRAL_SECONDS = 15;

nit: use the same modifier structure as the other constants above.

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #11)
> Comment on attachment 779719 [details] [diff] [review]
> Patch (v2)
> 
> Review of attachment 779719 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see previous comments. You need to remove SetupScreen and assets, and

<snip>

Removing SetupScreen and friends helped with one of my tickets, so I removed them in Bug 895670.

Comment 13

[Gian-Carlo Pascutto [:gcp]]

Ah great, then it's only the assets/places.sqlite.zip that has to go. r+ with that fixed.

Comment 14

[Mark Capella [:capella]]

Attachment: Patch (v3)

I saw that |Removing SetupScreen and friends| had been done but I didn't credit nalexander, so thanks for that :)

As for the |places.sqlite| bit, it took a second for my brain to catch up to what we were talking about ... this should do it  :-D

Comment 15

[Mark Capella [:capella]]

lets go through try first https://tbpl.mozilla.org/?tree=Try&rev=07485a17983e

Comment 16

[Mark Capella [:capella]]

And on to inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/f8024029da3a

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f8024029da3a