Upgrade path from XUL to native fennec

RESOLVED FIXED in Firefox 11

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
6 years ago
11 months ago

People

(Reporter: wesj, Assigned: gcp)

Tracking

unspecified
Firefox 12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, blocking-fennec1.0 beta+, fennec11+)

Details

(Whiteboard: [QA+])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
Priority: -- → P2

Comment 1

6 years ago
This is a terrible user experience. Can we investigate feasibility before we P2 this one?
Priority: P2 → P1
What about extensions? And master password and other data?

Updated

6 years ago
Assignee: nobody → gpascutto

Comment 3

6 years ago
gcp, any thoughts on this so far?

Updated

6 years ago
Depends on: 707708
(Assignee)

Comment 4

6 years ago
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
Component: General → Evangelism
(Assignee)

Updated

6 years ago
Component: Evangelism → General
(Assignee)

Updated

6 years ago
Depends on: 708967
Depends on: 704547
(Assignee)

Comment 5

6 years ago
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.
Attachment #581324 - Flags: review?(blassey.bugs)
Attachment #581324 - Flags: feedback?(lucasr.at.mozilla)
(Assignee)

Comment 6

6 years ago
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

6 years ago
gcp, please file it.  it should have been disabled with MOZ_ANDROID_HISTORY.
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
(Assignee)

Comment 9

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

File as bug 711034.
(Assignee)

Comment 10

6 years ago
>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.
> 
> >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.
(Assignee)

Updated

6 years ago
Depends on: 711034
(Assignee)

Comment 12

6 years ago
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.
Attachment #581324 - Attachment is obsolete: true
Attachment #581324 - Flags: review?(blassey.bugs)
Attachment #581324 - Flags: feedback?(lucasr.at.mozilla)
Attachment #582250 - Flags: review?(blassey.bugs)
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()
Attachment #582250 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 14

6 years ago
>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"?
(Assignee)

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fe7f595a70e
https://hg.mozilla.org/mozilla-central/rev/5fe7f595a70e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Whiteboard: [QA+]
Depends on: 712718

Updated

6 years ago
Depends on: 713228

Updated

6 years ago
Depends on: 713282

Updated

6 years ago
Depends on: 713283
tracking-fennec: --- → 11+
https://hg.mozilla.org/releases/mozilla-aurora/rev/5fe7f595a70e
status-firefox11: --- → fixed
Depends on: 720461
blocking-fennec1.0: --- → beta+
Target Milestone: --- → Firefox 12
You need to log in before you can comment on or make changes to this bug.