Closed Bug 736237 Opened 8 years ago Closed 8 years ago

Profile Migration should specify a limit and be resumable

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- beta+

People

(Reporter: gcp, Assigned: gcp)

References

Details

Attachments

(2 files)

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.
blocking-fennec1.0: --- → ?
Assignee: nobody → gpascutto
blocking-fennec1.0: ? → beta+
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.
Attachment #611458 - Flags: review?(lucasr.at.mozilla)
Pretty noticeable speedup - might fix some other yank/ANR bugs.
Attachment #611466 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #611458 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attachment #611466 - Flags: review?(lucasr.at.mozilla) → review+
Duplicate of this bug: 741116
Status: NEW → ASSIGNED
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"
Depends on: 741836
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.
https://hg.mozilla.org/mozilla-central/rev/1540736ae993
https://hg.mozilla.org/mozilla-central/rev/b6cc76f45b95
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
You need to log in before you can comment on or make changes to this bug.