Last Comment Bug 699199 - Upgrade path from XUL to native fennec
: Upgrade path from XUL to native fennec
Status: RESOLVED FIXED
[QA+]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 12
Assigned To: Gian-Carlo Pascutto [:gcp]
:
Mentors:
Depends on: 704547 707708 708967 711034 712718 713228 713282 713283 720461
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-02 12:59 PDT by Wesley Johnston (:wesj)
Modified: 2012-05-02 13:15 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
beta+
11+


Attachments
Patch 1. Migrate bookmarks, history, favicons (21.58 KB, patch)
2011-12-13 10:38 PST, Gian-Carlo Pascutto [:gcp]
no flags Details | Diff | Review
Patch 1. Migrate bookmarks, history, favicons (23.05 KB, patch)
2011-12-16 05:32 PST, Gian-Carlo Pascutto [:gcp]
blassey.bugs: review+
Details | Diff | Review

Description Wesley Johnston (:wesj) 2011-11-02 12:59:17 PDT
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 2011-11-03 11:51:30 PDT
This is a terrible user experience. Can we investigate feasibility before we P2 this one?
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-11-07 09:27:05 PST
What about extensions? And master password and other data?
Comment 3 Doug Turner (:dougt) 2011-11-21 21:56:37 PST
gcp, any thoughts on this so far?
Comment 4 Gian-Carlo Pascutto [:gcp] 2011-12-12 01:15:00 PST
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] 2011-12-13 10:38:03 PST
Created attachment 581324 [details] [diff] [review]
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] 2011-12-14 10:08:20 PST
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) 2011-12-14 13:01:28 PST
gcp, please file it.  it should have been disabled with MOZ_ANDROID_HISTORY.
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2011-12-14 19:15:50 PST
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] 2011-12-15 04:39:31 PST
>gcp, please file it.  it should have been disabled with MOZ_ANDROID_HISTORY.

File as bug 711034.
Comment 10 Gian-Carlo Pascutto [:gcp] 2011-12-15 04:52:34 PST
>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?) 2011-12-15 08:50:05 PST
> 
> >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] 2011-12-16 05:32:34 PST
Created attachment 582250 [details] [diff] [review]
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?) 2011-12-16 14:06:58 PST
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] 2011-12-17 03:09:34 PST
>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 16 Ed Morley [:emorley] 2011-12-20 05:54:03 PST
https://hg.mozilla.org/mozilla-central/rev/5fe7f595a70e
Comment 17 Brad Lassey [:blassey] (use needinfo?) 2012-01-09 11:29:57 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/5fe7f595a70e

Note You need to log in before you can comment on or make changes to this bug.