699199 - Upgrade path from XUL to native fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Wesley Johnston (:wesj)]

If we upgrade users from XUL to Native Fennec they'll lose their previous Fennec history and bookmarks (unless they're using sync?), and gain the system history and bookmarks. Seems kinda like a crappy experience to me.

Ideally we should (during install?) move their places data into the system database.

Comment 1

[Jay Sullivan]

This is a terrible user experience. Can we investigate feasibility before we P2 this one?

Comment 2

[Martijn Wargers (dead)]

What about extensions? And master password and other data?

Comment 3

[Doug Turner (:dougt)]

gcp, any thoughts on this so far?

Comment 4

[Gian-Carlo Pascutto [:gcp]]

My current WIP does this after launch but it should probably be moved into the idle thread similar to this (reminder-to-self):
http://hg.mozilla.org/mozilla-central/rev/c65be44ac489

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Migrate bookmarks, history, favicons

- Migrates the most recent 300 history entries. This number is based on the comments on top of LocalBrowserDB which suggests only 250 are kept in history anyway. Limit can be adjusted/removed if this is a wrong assumption.

- There is a check if the user has a substantial (>50 entries) history that is newer than the places history. Because we can't set access times for history (not through BrowserDB, which seems to inherits that limitation from the Android API), we can't push old history entries below new ones. So if the user already has quite some new history, we don't attempt to recover the old places data. This can happen if the user is an early adopter of native, or if (s)he stopped using Fennec in favor of the Android browser before going back to native Fennec.

- No attempt is made to recover bookmark menu structure (again, because BrowserDB/Android doesn't offer an API for it that I know). 

- Maybe we should check if Sync is enabled? Does it make any sense to attempt to migrate places if the user is using Sync?

- The code to remove the lib cache is included, but this causes current m-c to crash, so its commented out. Is this because the dependent bug (forgot the #) hasn't landed yet?

- We currently keep the places.sqlite but place a marker that the migration has completed, and will re-migrate if places.sqlite is updated by anything. Because different Fennec versions are in different Android users anyway, I guess we might as well delete the file entirely? Users can't switch back/forth between native and XUL in the same profile, right?

- This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=708280#c9.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

If places.sqlite is no longer used, we should file a follow-up bug to stop it from being loaded and used, right? I see about 500k RAM SQLite overhead for it, it's also recreated automatically.

Comment 7

[Doug Turner (:dougt)]

gcp, please file it.  it should have been disabled with MOZ_ANDROID_HISTORY.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 581324 [details] [diff] [review]
Patch 1. Migrate bookmarks, history, favicons

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

::: mobile/android/base/ProfileMigrator.java
@@ +76,5 @@
> +       before migration. Overwriting their new history will just be
> +       annoying.
> +    */
> +    private static final int MAX_HISTORY_TO_REPLACE = 50;
> +    private static final int MAX_HISTORY_TO_CONVERT = 300;

What is the danger/annoyance of pulling in the old history? It shouldn't overwrite the existing history, right?

@@ +168,5 @@
> +
> +            cursor.moveToFirst();
> +            while (!cursor.isAfterLast()) {
> +                history.add(new HistoryEntry(cursor.getString(urlCol),
> +                                             /* can't get title */ null,

why can't we get the title?

@@ +225,5 @@
> +                            // Is a lot of it newer than the places DB?
> +                            if (androidDate > newestPlacesDate) {
> +                                // Don't convert, bail out.
> +                                cursor.close();
> +                                return;

I don't get this part. Why do we care how much history is in the native database? Or how old it is?

@@ +317,5 @@
> +
> +        SQLiteDatabase openPlaces(String dbPath) throws SQLiteException {
> +            /* http://stackoverflow.com/questions/2528489/no-such-table-android-metadata-whats-the-problem */
> +            SQLiteDatabase db = SQLiteDatabase.openDatabase(
> +                                                            dbPath,

move this up a line

@@ +395,5 @@
> +        }
> +
> +        @Override
> +        protected void onPostExecute(Void result) {
> +            Log.i(LOGTAG, "onPostExecute");

if there's nothing to do in onPostExecute() then use a Runnable an not an AsyncTask

Comment 9

[Gian-Carlo Pascutto [:gcp]]

>gcp, please file it.  it should have been disabled with MOZ_ANDROID_HISTORY.

File as bug 711034.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

>What is the danger/annoyance of pulling in the old history? It shouldn't overwrite 
>the existing history, right?

I thought it might, based on this comment:

public class LocalBrowserDB implements BrowserDB.BrowserDBIface {
    // Same as android.provider.Browser for consistency
    private static final int MAX_HISTORY_COUNT = 250;

which seemed to imply to me the history was limited to 250 entries.


>why can't we get the title?

Oops, that's no longer correct. I used getAllVisitedHistory before, which doesn't return the "title" column. But getRecentHistory should.

>I don't get this part. Why do we care how much history is in the native database? 
>Or how old it is?

I tried to explain this in comment 5. The problem is that, as far as I can tell, there's no way to *set* the visitdate when adding a history entry. So if we start importing places, all other history will be buried under whatever is in places, regardless of how new the native history is or how old places is. If the user was mostly using Firefox when (s)he migrates, there is no problem. But there are 2 cases in which this might be quite annoying:

a) A user tried XUL Fennec, decided it sucks, and switched back to the Android Browser. Now that he realizes that Native Fennec is the shit, (s)he updates. We risk burying his recent browsing history under the old history.

b) Same situation, but with a user that was using Native Nightlies for a while.

Let me reverse the question: in which situation, where the user has a lot of new native history, and the places data is old, does it make sense to import places on top of it?

Basically, it's a heuristic added to annoy people less. I can get rid of this easily if you think it's risky or superfluous.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

> 
> >I don't get this part. Why do we care how much history is in the native database? 
> >Or how old it is?
> 
> I tried to explain this in comment 5. The problem is that, as far as I can
> tell, there's no way to *set* the visitdate when adding a history entry. 
We can certainly set the visitdate for our local DB. I suspect we can for the system DB as well, but that's less of a concern right now.

> So
> if we start importing places, all other history will be buried under
> whatever is in places, regardless of how new the native history is or how
> old places is. If the user was mostly using Firefox when (s)he migrates,
> there is no problem. But there are 2 cases in which this might be quite
> annoying:
> 
> a) A user tried XUL Fennec, decided it sucks, and switched back to the
> Android Browser. Now that he realizes that Native Fennec is the shit, (s)he
> updates. We risk burying his recent browsing history under the old history.
> 
> b) Same situation, but with a user that was using Native Nightlies for a
> while.
> 
> Let me reverse the question: in which situation, where the user has a lot of
> new native history, and the places data is old, does it make sense to import
> places on top of it?
> 
> Basically, it's a heuristic added to annoy people less. I can get rid of
> this easily if you think it's risky or superfluous.

It certainly adds complexity for an unknown benefit. I'd rather you drop it for now and if this becomes an issue in testing we can do something as a follow up.

Comment 12

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Migrate bookmarks, history, favicons

>We can certainly set the visitdate for our local DB. I suspect we can for the system DB as 
>well, but that's less of a concern right now.

I extended the BrowserDB interface, local implementation and Android DB implementation to give access to that. This makes the previous logic unneeded. Instead, we remember a big chunk of Android history, and check for each URL if its Places access time is more recent than the one in the Android history. If so (or if it didn't exist), we update it.

Fixing bug 711034 allows us to remove places.sqlite (and friends) so no need to put marker files any more.

I needed to modify GlobalHistory a bit so the call that has to go to Gecko is separate from all the other processing. There's still a problem there that seems to be producing: https://bugzilla.mozilla.org/show_bug.cgi?id=711034#c6 
It's not specific to my code; it simply wasn't observed before because bug 711034 stopped the call to Gecko from being compiled, so all the notifyUriVisited calls were NOPs.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 582250 [details] [diff] [review]
Patch 1. Migrate bookmarks, history, favicons

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

::: mobile/android/base/GlobalHistory.java
@@ +106,5 @@
>              }
>          };
>      }
>  
> +    public void addToGecko(String uri) {

rename to addDontUpdateVisited()

Comment 14

[Gian-Carlo Pascutto [:gcp]]

>rename to addDontUpdateVisited()

That's rather strange, because it *does* update visited info...just for Gecko and not Android. Hence the "addToGecko" name. Maybe name it "addDontUpdateAndroid"?

Comment 15

[Gian-Carlo Pascutto [:gcp]]

https://tbpl.mozilla.org/?tree=Try&rev=98d12c3443ee

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fe7f595a70e

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5fe7f595a70e

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5fe7f595a70e