Closed
Bug 886587
Opened 8 years ago
Closed 8 years ago
Remove profile migrator
Categories
(Firefox for Android Graveyard :: Data Providers, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 25
People
(Reporter: Margaret, Assigned: capella)
References
Details
(Keywords: relnote, user-doc-needed, Whiteboard: [qa+])
Attachments
(1 file, 2 obsolete files)
87.97 KB,
patch
|
gcp
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
This will also allow us to remove an ugly bit of Makefile.in -- the logo.png copying.
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
Recently add, must be preserved / moved https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=878674&attachment=766600
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•8 years ago
|
||
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
Attachment #775757 -
Flags: feedback?(margaret.leibovic)
Comment 5•8 years ago
|
||
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•8 years ago
|
||
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.
Reporter | ||
Comment 7•8 years ago
|
||
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 :)
Attachment #775757 -
Flags: feedback?(rnewman)
Attachment #775757 -
Flags: feedback?(margaret.leibovic)
Attachment #775757 -
Flags: feedback+
Comment 8•8 years ago
|
||
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.
Attachment #775757 -
Flags: feedback?(rnewman) → feedback+
Comment 9•8 years ago
|
||
Flagging for QA and documentation.
Keywords: relnote,
user-doc-needed
Whiteboard: [qa+]
Assignee | ||
Comment 10•8 years ago
|
||
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.
Attachment #775757 -
Attachment is obsolete: true
Attachment #779719 -
Flags: review?(gpascutto)
Comment 11•8 years ago
|
||
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.
Attachment #779719 -
Flags: review?(gpascutto) → review-
Comment 12•8 years ago
|
||
(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•8 years ago
|
||
Ah great, then it's only the assets/places.sqlite.zip that has to go. r+ with that fixed.
Assignee | ||
Comment 14•8 years ago
|
||
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
Attachment #779719 -
Attachment is obsolete: true
Attachment #779799 -
Flags: review?(gpascutto)
Updated•8 years ago
|
Attachment #779799 -
Flags: review?(gpascutto) → review+
Assignee | ||
Comment 15•8 years ago
|
||
lets go through try first https://tbpl.mozilla.org/?tree=Try&rev=07485a17983e
Assignee | ||
Comment 16•8 years ago
|
||
And on to inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/f8024029da3a
Comment 17•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f8024029da3a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Updated•2 months 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
•