736237 - Profile Migration should specify a limit and be resumable

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

We should allow callers of Profile Migration to specify a limit on the number of (history) entries to convert. We should support subsequent calls to resume the migration where we left off. This will allow the CP/Sync to take over after the initial migration.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Make Profile migration resumable

Change Profile Migration to store its current progress in prefences. Do an initial migration of bookmarks + 2000 entries. Do not delete the database until we are completely done with migrating.

Bug 725150 will follow up with patches that expose this to Sync in the BrowserProvider.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 2. Avoid about:home refreshing during migration

Pretty noticeable speedup - might fix some other yank/ANR bugs.

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 611458 [details] [diff] [review]
Patch 1. Make Profile migration resumable

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

Looks good. I wonder if the batching should also be done for bookmarks as well? Some people have a long list of bookmarks. No big deal though.

::: mobile/android/base/ProfileMigrator.java
@@ +96,5 @@
> +    // Maximum number of history entries to fetch at once.
> +    // This limits the max memory use to about 10M (empirically), so we don't OOM.
> +    private static final int HISTORY_MAX_BATCH = 5000;
> +
> +    private static final String MIGRATE_BOOKMARKS_DONE = "bookmarks_done";

PREFS_MIGRATE_BOOKMARKS_DONE for clarity?

@@ +97,5 @@
> +    // This limits the max memory use to about 10M (empirically), so we don't OOM.
> +    private static final int HISTORY_MAX_BATCH = 5000;
> +
> +    private static final String MIGRATE_BOOKMARKS_DONE = "bookmarks_done";
> +    private static final String MIGRATE_HISTORY_DONE = "history_done";

PREFS_MIGRATE_HISTORY_DONE

@@ +99,5 @@
> +
> +    private static final String MIGRATE_BOOKMARKS_DONE = "bookmarks_done";
> +    private static final String MIGRATE_HISTORY_DONE = "history_done";
> +    // Number of history entries already migrated.
> +    private static final String MIGRATE_HISTORY_COUNT = "history_count";

PREFS_MIGRATE_HISTORY_COUNT

@@ +212,5 @@
> +    }
> +
> +    // Has migration entirely finished?
> +    public boolean hasMigrationFinished() {
> +        if (isBookmarksMigrated() && isHistoryMigrated()) {

return (isBookmarksMigrated() && isHistoryMigrated()); would be simpler.

@@ +258,5 @@
>          private Map<Long, Long> mRerootMap;
>          private ArrayList<ContentProviderOperation> mOperations;
> +        private int mMaxEntries;
> +
> +        public PlacesRunnable(int limit) {

Rename limit to maxEntries then? Or rename mMaxEntries to mLimit?

@@ +579,5 @@
> +            int totalEntries = currentEntries + queryResultEntries;
> +            setMigratedHistoryEntries(totalEntries);
> +
> +            // Reached the end of the history list? Then stop.
> +            if (queryResultEntries < mMaxEntries) {

Shouldn't it be <=?

@@ +866,5 @@
> +                    migrateBookmarks(db);
> +                    setMigratedBookmarks();
> +                } else {
> +                    Log.i(LOGTAG, "Bookmarks already migrated. Skipping...");
> +                }

nit: add empty line here.

@@ +877,4 @@
>                  db.close();
>  
> +                // Clean up if we finished this run.
> +                if (isHistoryMigrated()) {

Shouldn't this check isBookmarksMigrated as well?

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 611466 [details] [diff] [review]
Patch 2. Avoid about:home refreshing during migration

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

Nice.

::: mobile/android/base/GeckoApp.java
@@ +2362,5 @@
> +                    public void run() {
> +                        mAboutHomeContent.update(GeckoApp.mAppContext,
> +                            EnumSet.of(AboutHomeContent.UpdateFlags.TOP_SITES));
> +                    }
> +                });

Maybe factor out the code to update about home into a separate method and use it everwhere in GeckoApp? Also, you have to check if mAboutHomeContent is not null as it might not be present in certain cases.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 611458 [details] [diff] [review]
Patch 1. Make Profile migration resumable

>diff --git a/mobile/android/base/ProfileMigrator.java b/mobile/android/base/ProfileMigrator.java

>+    private static final String PREFS_NAME = "ProfMigr";

Can we use "ProfileMigrator"

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Pushed with comments addressed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6cc76f45b95
https://hg.mozilla.org/integration/mozilla-inbound/rev/1540736ae993

Bug 741836 is still a bit of a concern, but giving this wider testing may help identify what's causing it.

Comment 8

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/1540736ae993
https://hg.mozilla.org/mozilla-central/rev/b6cc76f45b95