Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Margaret, Assigned: capella)

Tracking

({relnote, user-doc-needed})

Trunk
Firefox 25
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa+])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

6 years ago
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?
This will also allow us to remove an ugly bit of Makefile.in -- the logo.png copying.
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

Updated

6 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Assignee

Comment 4

6 years ago
Posted patch Patch (v1) (obsolete) — Splinter Review
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 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.
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

6 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 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+
Flagging for QA and documentation.
Whiteboard: [qa+]
Assignee

Comment 10

6 years ago
Posted patch Patch (v2) (obsolete) — Splinter Review
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 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-
(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.
Ah great, then it's only the assets/places.sqlite.zip that has to go. r+ with that fixed.
Assignee

Comment 14

6 years ago
Posted patch Patch (v3)Splinter Review
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)
Attachment #779799 - Flags: review?(gpascutto) → review+
https://hg.mozilla.org/mozilla-central/rev/f8024029da3a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
You need to log in before you can comment on or make changes to this bug.