Closed
Bug 742815
Opened 12 years ago
Closed 12 years ago
Probing migration status can launch multiple migrations
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(blocking-fennec1.0 beta+)
RESOLVED
FIXED
Firefox 14
Tracking | Status | |
---|---|---|
blocking-fennec1.0 | --- | beta+ |
People
(Reporter: gcp, Assigned: gcp)
References
Details
Attachments
(1 file, 1 obsolete file)
6.65 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
https://bugzilla.mozilla.org/show_bug.cgi?id=725150#c28 rnewman pointed out that we can unintentionally launch multiple migrations. Unfortunately I already landed the code by then, so let's fix that issue here.
Updated•12 years ago
|
Assignee | ||
Comment 2•12 years ago
|
||
Regarding the remarks in bug 725150: https://bugzilla.mozilla.org/show_bug.cgi?id=725150#c28 >? Seems like you want s/hasMigrationFinished/isHistoryMigrated, right? >And do you really intend for both to be migrated when you check one? (That is, for >launch() to affect both?) Profile Migration just keeps working as it always has: it migrates bookmarks first and then part of history. If called again (ENSURE_HISTORY_MIGRATED), it will migrate more history until the old places database is exhausted. It's still supposed to do a good job even if the user never bothers with Sync. Because of this hasMigrationFinished implies isHistoryMigrated, and it's very much intentional that launching it will affect both bookmarks and history, if appropriate. I changed the (hasMigrationFinished/iHistoryMigrated) anyway in this patch to make it more symmetric and perhaps more future-proof.
Assignee: nobody → gpascutto
Attachment #613381 -
Flags: review?(lucasr.at.mozilla)
Comment 3•12 years ago
|
||
(In reply to Gian-Carlo Pascutto (:gcp) from comment #2) > Because of this hasMigrationFinished implies isHistoryMigrated, and > it's very much intentional that launching it will affect both bookmarks and > history, if appropriate. I see. Just wanted to make sure that you'd be getting the expected behavior when we call isBookmarksMigrated, get "no", then come right back and check history a few milliseconds later!
Assignee | ||
Comment 4•12 years ago
|
||
>Just wanted to make sure that you'd be getting the expected behavior when we call
>isBookmarksMigrated, get "no", then come right back and check history a few
>milliseconds later!
In normal operation, I don't think it is possible for Sync to call and get a "no" to isBookmarksMigrated, because that runs at the first Fennec start. It would require Sync to be set up first (and create the profile?) and start the initial migration before Fennec ever had a chance to run.
Even if it did so, it would only happen once, so I think we're good.
Comment 5•12 years ago
|
||
Comment on attachment 613381 [details] [diff] [review] Patch 1. Fix double launch on status probe Review of attachment 613381 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: mobile/android/base/db/BrowserProvider.java.in @@ +1466,3 @@ > > + if (needBookmarks || needHistory) { > + migrator.launch(); nit: add empty line here. @@ +1466,5 @@ > > + if (needBookmarks || needHistory) { > + migrator.launch(); > + needBookmarks = wantBookmarks && !migrator.isBookmarksMigrated(); > + needHistory = wantHistory && !migrator.isHistoryMigrated(); nit: add empty line here.
Attachment #613381 -
Flags: review?(lucasr.at.mozilla) → review+
Updated•12 years ago
|
blocking-fennec1.0: --- → beta+
Assignee | ||
Comment 6•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d047ef4fe29
Assignee | ||
Comment 7•12 years ago
|
||
This code is bugged: it doesn't respect the projection that is passed it when it's setting the results in the output cursor. I'm going to backout.
Assignee | ||
Comment 8•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/966e23109ac2
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #613381 -
Attachment is obsolete: true
Attachment #613956 -
Flags: review?(lucasr.at.mozilla)
Comment 10•12 years ago
|
||
Comment on attachment 613956 [details] [diff] [review] Patch 1. v2 Fix double launch on status probe Review of attachment 613956 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: mobile/android/base/db/BrowserProvider.java.in @@ +1495,2 @@ > > + boolean needBookmarks = wantBookmarks && !migrator.isBookmarksMigrated(); Wondering: shouldn't this be "areBookmarksMigrated() for correctness? :-) Also, needsBookmarks maybe?
Attachment #613956 -
Flags: review?(lucasr.at.mozilla) → review+
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3d047ef4fe29
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Comment 12•12 years ago
|
||
Backed out: https://hg.mozilla.org/mozilla-central/rev/966e23109ac2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 13•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa8facffe7ff
Status: REOPENED → NEW
Target Milestone: Firefox 14 → ---
Comment 14•12 years ago
|
||
(Setting inbound flags.)
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 14
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/aa8facffe7ff
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•