Last Comment Bug 710330 - Implement bookmarks and history import from Android system DBs
: Implement bookmarks and history import from Android system DBs
Status: VERIFIED FIXED
: late-l10n
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P3 normal (vote)
: Firefox 16
Assigned To: Gian-Carlo Pascutto [:gcp]
:
: Sebastian Kaspari (:sebastian)
Mentors:
: 647336 (view as bug list)
Depends on: 759393 768900 769216
Blocks: 710331 769896
  Show dependency treegraph
 
Reported: 2011-12-13 12:05 PST by Brad Lassey [:blassey] (use needinfo?)
Modified: 2016-07-29 14:21 PDT (History)
13 users (show)
nicolae.cristian2010: in‑moztrap+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified
verified
-
15+


Attachments
Patch 1. Refactor batch update functions into LocalDB (29.35 KB, patch)
2012-05-31 10:59 PDT, Gian-Carlo Pascutto [:gcp]
lucasr.at.mozilla: review+
Details | Diff | Splinter Review
Patch 2. Add functionality to import Android bookmarks (5.97 KB, patch)
2012-05-31 11:01 PDT, Gian-Carlo Pascutto [:gcp]
lucasr.at.mozilla: review+
Details | Diff | Splinter Review
Patch 3. Add simple UX for import (9.64 KB, patch)
2012-05-31 11:04 PDT, Gian-Carlo Pascutto [:gcp]
no flags Details | Diff | Splinter Review
Patch 4. Allow duplicated bookmarks (1.82 KB, patch)
2012-05-31 11:07 PDT, Gian-Carlo Pascutto [:gcp]
no flags Details | Diff | Splinter Review
Patch 1. Extend UPDATE_OR_INSERT support (9.53 KB, patch)
2012-06-08 11:37 PDT, Gian-Carlo Pascutto [:gcp]
lucasr.at.mozilla: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Patch 2. Refactor batch update functions into LocalDB (24.60 KB, patch)
2012-06-08 11:38 PDT, Gian-Carlo Pascutto [:gcp]
lucasr.at.mozilla: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Patch 3. Import from Android functionality (6.01 KB, patch)
2012-06-08 11:39 PDT, Gian-Carlo Pascutto [:gcp]
lucasr.at.mozilla: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Patch 4. Temporary UX for feature (9.81 KB, patch)
2012-06-08 11:40 PDT, Gian-Carlo Pascutto [:gcp]
no flags Details | Diff | Splinter Review
Patch 5. Allow duplicate bookmarks if in another folder (1.63 KB, patch)
2012-06-08 11:40 PDT, Gian-Carlo Pascutto [:gcp]
lucasr.at.mozilla: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Patch 4. UX according to 759393 (15.23 KB, patch)
2012-06-19 15:41 PDT, Gian-Carlo Pascutto [:gcp]
lucasr.at.mozilla: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Screenshot of UX (56.55 KB, image/png)
2012-06-20 04:06 PDT, Gian-Carlo Pascutto [:gcp]
no flags Details
Screenshot of UX 2 (59.43 KB, image/png)
2012-06-20 04:07 PDT, Gian-Carlo Pascutto [:gcp]
no flags Details
Patch 6. Fix updating of bookmark folders and seperators (3.37 KB, patch)
2012-06-20 06:14 PDT, Gian-Carlo Pascutto [:gcp]
lucasr.at.mozilla: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Patch 7. Fix broken isBookmark logic (1.93 KB, patch)
2012-06-25 09:42 PDT, Gian-Carlo Pascutto [:gcp]
margaret.leibovic: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Brad Lassey [:blassey] (use needinfo?) 2011-12-13 12:05:38 PST
we basically want to enable a one time import on first run, let's let this bug be for the platform work to enable that I'll file another one for the UX
Comment 1 Kevin Brosnan [:kbrosnan] 2012-01-11 15:53:32 PST
*** Bug 647336 has been marked as a duplicate of this bug. ***
Comment 2 Tony Chung [:tchung] 2012-03-05 09:51:37 PST
flagging for triage.  likely not 1.0 feature
Comment 3 Gian-Carlo Pascutto [:gcp] 2012-05-30 03:45:26 PDT
Note: this will add android.permission.READ_HISTORY_BOOKMARKS so we need to document it.
Comment 4 Gian-Carlo Pascutto [:gcp] 2012-05-31 07:53:01 PDT
The current Migration code has a bit of a bug in that it might import [1] several "null" folders directly below Places, which as far as I understood from mak are things like the "left pane" in the "Show All Bookmarks/Library" view on Desktop, because those are also rooted directly below Places.

V/ProfileMigrator(27887): Name: places, pid=1, nid=0
V/ProfileMigrator(27887): Name: menu, pid=2, nid=3
V/ProfileMigrator(27887): Name: toolbar, pid=3, nid=2
V/ProfileMigrator(27887): Name: tags, pid=4, nid=4
V/ProfileMigrator(27887): Name: unfiled, pid=5, nid=5
V/ProfileMigrator(27887): Mobile root not found, is this a desktop profile?
I/ProfileMigrator(27887): Fetching bookmarks from places
...
I/ProfileMigrator(27887): Adding url=null, title=, parent=1, type=2
I/GeckoLocalBrowserDB(27887): Remapping parent=0 for url=null title=, type=0

These don't show up if they're under Desktop Bookmarks, but they do when put under Mobile. I suppose Margaret worked around this bug (knowingly or unknowingly) at some point.

[1] I say might import because I can see this with a desktop profile, but it's possible Sync successfully avoids syncing those "internal" folders, so you wouldn't see it with a real profile.
Comment 5 Gian-Carlo Pascutto [:gcp] 2012-05-31 07:53:49 PDT
Note that comment 4 talks about XUL->Native migration - I'm mentioning it here because I've refactoring/reusing code from there.
Comment 6 Gian-Carlo Pascutto [:gcp] 2012-05-31 10:59:41 PDT
Created attachment 628810 [details] [diff] [review]
Patch 1. Refactor batch update functions into LocalDB

This pulls as much common stuff out of Profile Migration and into LocalDB. Since it's sortof a wrapper around BrowserProvider, it seemed the most appropriate place. These functions are not reflected in BrowserDB as that probably makes no sense.

It basically allows bookmarks/history/favicon adding using transactions by passing in a Collection of ContentProviderOperations.
Comment 7 Gian-Carlo Pascutto [:gcp] 2012-05-31 11:01:18 PDT
Created attachment 628812 [details] [diff] [review]
Patch 2. Add functionality to import Android bookmarks
Comment 8 Gian-Carlo Pascutto [:gcp] 2012-05-31 11:04:56 PDT
Created attachment 628815 [details] [diff] [review]
Patch 3. Add simple UX for import

While we wait an official UX comment on bug 759393, this simply adds the function to the settings menu.
Comment 9 Gian-Carlo Pascutto [:gcp] 2012-05-31 11:07:26 PDT
Created attachment 628818 [details] [diff] [review]
Patch 4. Allow duplicated bookmarks

The old profile migration function didn't allow duplicated URL bookmarks. That is a bug. It gets worse if we allow Android Import. So this fixes it.

Obviously, QA will want to retest Migration after this bug lands :)
Comment 10 Lucas Rocha (:lucasr) 2012-06-06 06:50:10 PDT
Comment on attachment 628810 [details] [diff] [review]
Patch 1. Refactor batch update functions into LocalDB

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

r+ but I'd like to have my questions answered.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1294,5 @@
>  
>                  if (!values.containsKey(Bookmarks.POSITION)) {
>                      debug("Inserting bookmark with no position for URI");
> +                    values.put(Bookmarks.POSITION,
> +                               Long.toString(BrowserContract.Bookmarks.DEFAULT_POSITION));

Not a big deal but this looks like an unrelated change.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +624,5 @@
> +            cursor = cr.query(getAllHistoryUri(),
> +                              projection,
> +                              History.URL + " = ?",
> +                              new String[] { url },
> +                              null);

Isn't this code a bit racy? You check if a history entry exist then either update or insert but what if by the time you check for the existing entry it doesn't exist but then an entry is inserted before you actually run the batched operation?  Not sure how likely this is to happen. I guess the migration will happen before browser UI is shown or something? Are you preventing sync from running during the migration? (Maybe there's code for that in the following patches?)

@@ +733,5 @@
> +                cursor = cr.query(getAllBookmarksUri(),
> +                                  projection,
> +                                  Bookmarks.URL + " = ?",
> +                                  new String[] { url },
> +                                  null);

Same comment about this method being racy.
Comment 11 Lucas Rocha (:lucasr) 2012-06-06 06:54:19 PDT
Comment on attachment 628812 [details] [diff] [review]
Patch 2. Add functionality to import Android bookmarks

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

::: mobile/android/base/AndroidImport.java
@@ +68,5 @@
> +            } else {
> +                created = System.currentTimeMillis();
> +            }
> +            byte[] data = cursor.getBlob(faviconCol);
> +            mDB.updateBookmark(mCr, mOperations,

Looking at this  method in context now, I'd prefer it to be called updateBookmarkInBatch() or something.

@@ +103,5 @@
> +            String title = cursor.getString(titleCol);
> +            long date = cursor.getLong(dateCol);
> +            int visits = cursor.getInt(visitsCol);
> +            byte[] data = cursor.getBlob(faviconCol);
> +            mDB.updateBrowserHistory(mCr, mOperations, url, title, date, visits);

updateHistoryEntryInBatch()?

@@ +121,5 @@
> +            // We don't really care for the results, this is best-effort.
> +            mCr.applyBatch(BrowserContract.AUTHORITY, mOperations);
> +        } catch (RemoteException e) {
> +            Log.e(LOGTAG, "Remote exception while updating db: ", e);
> +            } catch (OperationApplicationException e) {

Fix indentation
Comment 12 Lucas Rocha (:lucasr) 2012-06-06 06:57:38 PDT
Comment on attachment 628812 [details] [diff] [review]
Patch 2. Add functionality to import Android bookmarks

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

::: mobile/android/base/AndroidImport.java
@@ +36,5 @@
> +    private ArrayList<ContentProviderOperation> mOperations;
> +    private ContentResolver mCr;
> +    private LocalBrowserDB mDB;
> +
> +    public AndroidImport(Context context, Runnable doneCallback) {

doneCallback doesn't match a Runnable very well. Maybe onDoneRunnable or something?
Comment 13 Lucas Rocha (:lucasr) 2012-06-06 06:59:44 PDT
Comment on attachment 628815 [details] [diff] [review]
Patch 3. Add simple UX for import

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

Shouldn't this pref have a confirmation dialog before actually importing? This needs feedback from UX before I give my r+.

::: mobile/android/base/AndroidImportPreference.java
@@ +32,5 @@
> +                                mContext.getString(R.string.bookmarkhistory_import_title),
> +                                mContext.getString(R.string.bookmarkhistory_import_wait),
> +                                true);
> +
> +        final Runnable stopCallback = new Runnable() {

onDoneRunnable instead of stopCallback?

::: mobile/android/base/Makefile.in
@@ +43,5 @@
>  
>  FENNEC_JAVA_FILES = \
>    AboutHomeContent.java \
>    AboutHomeSection.java \
> +  AndroidImport.java \

I guess the change to add AndroidImport.java to the Makefile should go with the previous patch?

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +155,5 @@
>  with that structure, consider a translation which ignores the preceding domain and
>  just addresses the organization to follow, e.g. "This site is run by " -->
>  <!ENTITY identity_run_by "which is run by">
> +
> +<!ENTITY bookmarkhistory_import_title "Importing bookmarks and history">

...from Android?
Comment 14 Lucas Rocha (:lucasr) 2012-06-06 07:03:18 PDT
Comment on attachment 628818 [details] [diff] [review]
Patch 4. Allow duplicated bookmarks

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

::: mobile/android/base/db/LocalBrowserDB.java
@@ +733,5 @@
>                  cursor = cr.query(getAllBookmarksUri(),
>                                    projection,
> +                                  Bookmarks.URL + " = ? AND " +
> +                                  Bookmarks.PARENT + " = ?",
> +                                  new String[] { url, Long.toString(parent) },

I don't see enough context to review this patch. This change might conflict with how we handle reading list items. Although reading list items are regular bookmarks, any checks for regular bookmarks should ignore bookmarks in the reading list special folder.
Comment 15 Lucas Rocha (:lucasr) 2012-06-06 07:04:48 PDT
Comment on attachment 628810 [details] [diff] [review]
Patch 1. Refactor batch update functions into LocalDB

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

r+ but I'd like to have my questions answered.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1294,5 @@
>  
>                  if (!values.containsKey(Bookmarks.POSITION)) {
>                      debug("Inserting bookmark with no position for URI");
> +                    values.put(Bookmarks.POSITION,
> +                               Long.toString(BrowserContract.Bookmarks.DEFAULT_POSITION));

Not a big deal but this looks like an unrelated change.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +624,5 @@
> +            cursor = cr.query(getAllHistoryUri(),
> +                              projection,
> +                              History.URL + " = ?",
> +                              new String[] { url },
> +                              null);

Isn't this code a bit racy? You check if a history entry exist then either update or insert but what if by the time you check for the existing entry it doesn't exist but then an entry is inserted before you actually run the batched operation?  Not sure how likely this is to happen. I guess the migration will happen before browser UI is shown or something? Are you preventing sync from running during the migration? (Maybe there's code for that in the following patches?)

@@ +711,5 @@
> +        values.put(Bookmarks.POSITION, position);
> +        // Restore deleted record if possible
> +        values.put(Bookmarks.IS_DELETED, 0);
> +
> +        // This assumes no real folder has a negative ID.

The Reading list special folder has negative id btw. But that probably doesn't matter in the context of this method as you're only dealing with regular bookmarks here. Might be worth adding a note about that though.

@@ +733,5 @@
> +                cursor = cr.query(getAllBookmarksUri(),
> +                                  projection,
> +                                  Bookmarks.URL + " = ?",
> +                                  new String[] { url },
> +                                  null);

Same comment about this method being racy.
Comment 16 Gian-Carlo Pascutto [:gcp] 2012-06-06 07:15:09 PDT
>Isn't this code a bit racy? ...I guess the migration will happen before browser UI 
>is shown or something? Are you preventing sync from running during the migration? 

That's a good catch. We can't ensure that Sync isn't going to run for this importing (even though the current UX locks the UI), and its an open bug to let Sync run during the current migration, so this is an issue that can't be ignored. r- :)

>Shouldn't this pref have a confirmation dialog before actually importing? This 
>needs feedback from UX before I give my r+.
>I guess the change to add AndroidImport.java to the Makefile should go with the 
>previous patch?

UX for this feature is bug 759393. This is "temporary UX" so it can be tested. Not sure if we can land things with "temporary UX". There's no point in compiling in the file if there's no way to access the feature.

>I don't see enough context to review this patch. This change might conflict with 
>how we handle reading list items. Although reading list items are regular 
>bookmarks, any checks for regular bookmarks should ignore bookmarks in the reading 
>list special folder.

I
Comment 17 Gian-Carlo Pascutto [:gcp] 2012-06-06 07:18:48 PDT
>I don't see enough context to review this patch. This change might conflict with 
>how we handle reading list items. Although reading list items are regular 
>bookmarks, any checks for regular bookmarks should ignore bookmarks in the reading 
>list special folder.

This is the generic batched "add a bookmark" path. If this can potentially affect reading mode, then the *current* m-c is necessarily buggy. And I think it is, because the current code doesn't allow you to add the same URL twice regardless of anything else, which would prevent you from adding two reading items or having the same bookmark in bookmarks and reading items.

However, this function is currently only used by Profile Migration, which runs before you can have reading mode items, so I guess we're lucky you can't hit that bug.
Comment 18 Richard Newman [:rnewman] 2012-06-06 10:09:28 PDT
(In reply to Lucas Rocha (:lucasr) from comment #15)

> Isn't this code a bit racy? You check if a history entry exist then either
> update or insert but what if by the time you check for the existing entry it
> doesn't exist but then an entry is inserted before you actually run the
> batched operation?

Sync suffers from the same race condition, because there's no transaction support in the ContentResolver/ContentProvider interface, and we need to query-then-update-or-insert.

(And not even UPDATE OR INSERT; we need to apply logic to the current contents of the DB, reconciling to compute the new record.)
Comment 19 Gian-Carlo Pascutto [:gcp] 2012-06-06 10:27:12 PDT
I think you mean that while there's transaction support, the API isn't powerful enough to do this correctly.

Lucas pointed me to a workaround that Margaret implemented, which is to set a flag in the URL that makes UPDATE fall back to INSERT:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java.in#1169

I'm going to implement this for all Sync/Migration related tables.
Comment 20 Richard Newman [:rnewman] 2012-06-06 21:19:31 PDT
Alas, I wrote a long comment for this, and something along the way ate it. Let's try again.

(In reply to Gian-Carlo Pascutto (:gcp) from comment #19)
> I think you mean that while there's transaction support, the API isn't
> powerful enough to do this correctly.

To clarify: the ContentResolver/ContentProvider interface doesn't expose transactional operations. Sync uses those interfaces, so even though the *implementation* of BrowserProvider can use transactions, its callers cannot do something like:

  cr.beginTransaction();
  cr.query(…);
  cr.update(…);
  cr.commitTransaction();

Sync does exactly this kind of thing, so it's always somewhat vulnerable to races.

> Lucas pointed me to a workaround that Margaret implemented, which is to set
> a flag in the URL that makes UPDATE fall back to INSERT:
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/
> BrowserProvider.java.in#1169
> 
> I'm going to implement this for all Sync/Migration related tables.

If you're doing that for the Sync half of the equation, I can save you some effort!

If this is a way of implementing UPDATE OR INSERT, then we don't really need it — we *always* need to query first so that we can reconcile. We'll never use UPDATE OR INSERT.

The only other situation in which an UPDATE might fall back to an INSERT is that Sync issues a query, the user deletes a bookmark, and then Sync tries to UPDATE the record that was just deleted.

We don't want this to fall back to an INSERT: if the user just deleted a bookmark, we want to detect that and propagate the deletion. We want this sync to fail so that we can get it right next time.

If there's a collision, we always want to run our reconciling logic against what's in the database, not blindly overwrite it.

(Similarly we don't want an INSERT to fall back to an UPDATE, but that's less common.)
Comment 21 Gian-Carlo Pascutto [:gcp] 2012-06-07 01:13:52 PDT
>To clarify: the ContentResolver/ContentProvider interface doesn't expose 
>transactional operations.

It exposes bulkInsert and applyBatch, which are transactional operations. As I said that default API isn't flexible enough to do query-then-update-or-insert.

With that URI flag trick you can use applyBatch + newUpdate and have the latter fall back to mean newInsert on failure. This doesn't require visibility of BrowserProvider internals (i.e. it works if you only see ContentResolver) although it's obviously BrowserProvider-specific.

>The only other situation in which an UPDATE might fall back to an INSERT is that 
>Sync issues a query, the user deletes a bookmark, and then Sync tries to UPDATE 
>the record that was just deleted.

Thinking about this from the import side (no need to think about it happening on the migration side as nothing will be running concurrently), the two cases would be:

1) Sync deletes a bookmark/history item just when we're updating it.
2) History expiration deletes a history item just when we're updating it.

I do think I'll need to handle this.
Comment 22 Gian-Carlo Pascutto [:gcp] 2012-06-08 11:37:54 PDT
Created attachment 631476 [details] [diff] [review]
Patch 1. Extend UPDATE_OR_INSERT support
Comment 23 Gian-Carlo Pascutto [:gcp] 2012-06-08 11:38:35 PDT
Created attachment 631477 [details] [diff] [review]
Patch 2. Refactor batch update functions into LocalDB
Comment 24 Gian-Carlo Pascutto [:gcp] 2012-06-08 11:39:01 PDT
Created attachment 631478 [details] [diff] [review]
Patch 3. Import from Android functionality
Comment 25 Gian-Carlo Pascutto [:gcp] 2012-06-08 11:40:26 PDT
Created attachment 631479 [details] [diff] [review]
Patch 4. Temporary UX for feature

Ian, are you ok with landing this as an entry in the settings menu until the UX is finalized?
Comment 26 Gian-Carlo Pascutto [:gcp] 2012-06-08 11:40:52 PDT
Created attachment 631481 [details] [diff] [review]
Patch 5. Allow duplicate bookmarks if in another folder
Comment 27 Lucas Rocha (:lucasr) 2012-06-11 06:23:27 PDT
Comment on attachment 631476 [details] [diff] [review]
Patch 1. Extend UPDATE_OR_INSERT support

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

Nice.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1908,5 @@
> +            return updated;
> +
> +        insertBookmark(uri, values);
> +
> +        // Return 0 if we added a new row

Is this some sort of convention? Returning 0 makes sense, just wondering. One could see an insert as a form of update to the database :-)
Comment 28 Lucas Rocha (:lucasr) 2012-06-11 06:24:16 PDT
Comment on attachment 631476 [details] [diff] [review]
Patch 1. Extend UPDATE_OR_INSERT support

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

Almost forgot: do we have tests for this code (insert or update in bookmarks) already?
Comment 29 Lucas Rocha (:lucasr) 2012-06-11 06:25:49 PDT
Comment on attachment 631477 [details] [diff] [review]
Patch 2. Refactor batch update functions into LocalDB

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

Cool.
Comment 30 Lucas Rocha (:lucasr) 2012-06-11 06:29:15 PDT
Comment on attachment 631478 [details] [diff] [review]
Patch 3. Import from Android functionality

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

::: mobile/android/base/AndroidImport.java
@@ +71,5 @@
> +            byte[] data = cursor.getBlob(faviconCol);
> +            mDB.updateBookmarkInBatch(mCr, mOperations,
> +                                      url, title, null, -1,
> +                                      created, created, BrowserContract.Bookmarks.DEFAULT_POSITION,
> +                               null, Bookmarks.TYPE_BOOKMARK);

Fix indentation?
Comment 31 Gian-Carlo Pascutto [:gcp] 2012-06-11 06:35:46 PDT
>Is this some sort of convention? Returning 0 makes sense, just wondering. One 
>could see an insert as a form of update to the database :-)

The behavior stays compatible with bug 741590 which introduced it. My best guess is that it makes it clear what happened: a 0 can only mean an insert happened - if we'd return 1 on insert this could have been a successful update *or* an insert. Now it's always the result from the update, which is also what the user asked, so it's consistent.
Comment 32 Lucas Rocha (:lucasr) 2012-06-11 06:39:12 PDT
Comment on attachment 631479 [details] [diff] [review]
Patch 4. Temporary UX for feature

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

What do you mean with "temporary UX"? I still think there should be a dialog confirming the operation before importing. Ian?

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +170,5 @@
>  just addresses the organization to follow, e.g. "This site is run by " -->
>  <!ENTITY identity_run_by "which is run by">
> +
> +<!ENTITY bookmarkhistory_import_title "Importing bookmarks and history
> +                                       from Android">

Are line breaks ignored when used in the UI?
Comment 33 Lucas Rocha (:lucasr) 2012-06-11 06:40:14 PDT
Comment on attachment 631481 [details] [diff] [review]
Patch 5. Allow duplicate bookmarks if in another folder

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

Nice.
Comment 34 Gian-Carlo Pascutto [:gcp] 2012-06-11 09:52:20 PDT
>Almost forgot: do we have tests for this code (insert or update in bookmarks) 
>already?

They're covered by the Profile Migration tests, which are still awaiting review:
https://bugzilla.mozilla.org/show_bug.cgi?id=750753
Comment 35 Richard Newman [:rnewman] 2012-06-11 13:47:01 PDT
Comment on attachment 631478 [details] [diff] [review]
Patch 3. Import from Android functionality

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

::: mobile/android/base/AndroidImport.java
@@ +63,5 @@
> +            String url = cursor.getString(urlCol);
> +            String title = cursor.getString(titleCol);
> +            long created = 0;
> +            if (createCol >= 0) {
> +                created = cursor.getLong(createCol);

If you're using this creation time as the modified time for the record, it probably won't get synced. That would be a bad thing.

You should always use the current time for newly added (from the perspective of Sync) records.

@@ +115,5 @@
> +        flushBatchOperations();
> +    }
> +
> +    protected void flushBatchOperations() {
> +        Log.i(LOGTAG, "Flushing " + mOperations.size() + " DB operations");

Debug.

@@ +123,5 @@
> +        } catch (RemoteException e) {
> +            Log.e(LOGTAG, "Remote exception while updating db: ", e);
> +        } catch (OperationApplicationException e) {
> +            // Bug 716729 means this happens even in normal circumstances
> +            Log.i(LOGTAG, "Error while applying database updates: ", e);

Then can we log at something lower than INFO? Seems like DEBUG would be a good fit.
Comment 36 Gian-Carlo Pascutto [:gcp] 2012-06-19 06:05:25 PDT
>Are line breaks ignored when used in the UI?

It's (apparently) a property of SGML, of which XML is a subset, that this kind of (repeated) whitespace is entirely ignored. And emacs, which formatted it like that automagically, knows about that :P
Comment 37 Gian-Carlo Pascutto [:gcp] 2012-06-19 15:41:30 PDT
Created attachment 634629 [details] [diff] [review]
Patch 4. UX according to 759393

MultiSelectListPreference is API level 11, so we have do the light version of that.
Comment 38 Lucas Rocha (:lucasr) 2012-06-20 03:25:22 PDT
Comment on attachment 634629 [details] [diff] [review]
Patch 4. UX according to 759393

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

Good. Just curious on how it looks: any screenshot?

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +175,5 @@
>  <!ENTITY identity_run_by "which is run by">
> +
> +<!ENTITY bookmarkhistory_button_import "Import">
> +<!ENTITY bookmarkhistory_import_title "Importing bookmarks and/or history
> +                                       from Android">

"and/or" sounds a bit weird. Is this the message that the UX guys suggested?
Comment 39 Gian-Carlo Pascutto [:gcp] 2012-06-20 03:46:39 PDT
>"and/or" sounds a bit weird. Is this the message that the UX guys suggested?

They didn't say anything about the progress dialog. The problem with putting "and" was that it might give the impression we're doing importing bookmarks (or history) even if the user deselected that option.

Maybe I should just put "or", which is "wrong" from a boolean perspective but will be less confusing.
Comment 40 Gian-Carlo Pascutto [:gcp] 2012-06-20 04:06:39 PDT
Created attachment 634839 [details]
Screenshot of UX
Comment 41 Gian-Carlo Pascutto [:gcp] 2012-06-20 04:07:06 PDT
Created attachment 634840 [details]
Screenshot of UX 2
Comment 42 Lucas Rocha (:lucasr) 2012-06-20 04:09:13 PDT
(In reply to Gian-Carlo Pascutto (:gcp) from comment #39)
> >"and/or" sounds a bit weird. Is this the message that the UX guys suggested?
> 
> They didn't say anything about the progress dialog. The problem with putting
> "and" was that it might give the impression we're doing importing bookmarks
> (or history) even if the user deselected that option.
> 
> Maybe I should just put "or", which is "wrong" from a boolean perspective
> but will be less confusing.

I'd probably add 3 strings for each case to avoid any confusion.
Comment 43 Lucas Rocha (:lucasr) 2012-06-20 04:10:16 PDT
(In reply to Gian-Carlo Pascutto (:gcp) from comment #41)
> Created attachment 634840 [details]
> Screenshot of UX 2

I'd probably center the progress spinner and its label in the dialog. Not a big deal though.
Comment 44 Gian-Carlo Pascutto [:gcp] 2012-06-20 04:15:35 PDT
>I'd probably center the progress spinner and its label in the dialog. Not a big 
>deal though.

It's Android's own (default) ProgressDialog with indefinite progress, I don't think we want to modify this.

Likewise Import/Cancel are in reversed order from bug 759393 because apparently "standard Android" puts them that way, and our other dialogs work this way too.
Comment 45 Gian-Carlo Pascutto [:gcp] 2012-06-20 06:14:46 PDT
Created attachment 634867 [details] [diff] [review]
Patch 6. Fix updating of bookmark folders and seperators

We can't update bookmark folders or separators based on their URL. Need to use the title for folders and the position for separators.
Comment 47 Ed Morley [:emorley] 2012-06-25 09:29:05 PDT
Backed out for native Android M7 failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c38e96f197e5

https://tbpl.mozilla.org/php/getParsedLog.php?id=12969768&tree=Mozilla-Inbound
{
3361 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/localstorage/test_localStorageOriginsDomainDiffs.html | localStorage different domains; Test timed out
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd380dbfcbf1
Comment 48 Gian-Carlo Pascutto [:gcp] 2012-06-25 09:42:21 PDT
Created attachment 636350 [details] [diff] [review]
Patch 7. Fix broken isBookmark logic
Comment 49 Gian-Carlo Pascutto [:gcp] 2012-06-25 09:47:25 PDT
>Backed out for native Android M7 failures:

They went green on retrigger an the code is unrelated, so I proclaim my innocence.
Comment 50 :Margaret Leibovic 2012-06-25 10:49:28 PDT
Comment on attachment 636350 [details] [diff] [review]
Patch 7. Fix broken isBookmark logic

Oof, how did we never notice this? I feel particularly bad because I just wrote a patch near this code.

This looks good to me. For the LocalBrowserDB method we could also query bookmarksUriWithLimit(1) instead of mBookmarksUriWithProfile to limit potential extra results.
Comment 51 Richard Newman [:rnewman] 2012-06-25 11:38:09 PDT
gcp: you might want to change:

    1.69 +            // Need to set it to the current time so Sync picks it up.
    1.70 +            long created = System.currentTimeMillis();
    1.71 +            byte[] data = cursor.getBlob(faviconCol);
    1.72 +            mDB.updateBookmarkInBatch(mCr, mOperations,
    1.73 +                                      url, title, null, -1,
    1.74 +                                      created, created, BrowserContract.Bookmarks.DEFAULT_POSITION,
    1.75 +                                      null, Bookmarks.TYPE_BOOKMARK);

Sync only cares about the modified time. You can preserve the creation time if you want it for other purposes.
Comment 54 Gian-Carlo Pascutto [:gcp] 2012-07-03 08:36:04 PDT
Comment on attachment 636350 [details] [diff] [review]
Patch 7. Fix broken isBookmark logic

[Approval Request Comment]
User impact if declined: No import functionality from stock Browser.
Testing completed (on m-c, etc.): Landed on m-c a while ago, QA did testing and found a bug (bug 768900)
Risk to taking this patch (and alternatives if risky): These significantly affect the Profile Migration code, so there is a chance they will break it. On the other hand, while developing the refactorings here I wrote+landed tests for Profile Migration and they found a bug in the existing code, which one of these patches fixes.
String or UUID changes made by this patch: Several UX changes with strings.
Comment 56 Cristian Nicolae (:xti) 2012-07-19 06:53:43 PDT
TCs were added in MozTrap and can be found here: https://moztrap.mozilla.org/manage/cases/?filter-suite=62

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