Implement bookmarks and history import from Android system DBs

VERIFIED FIXED in Firefox 15

Status

()

Firefox for Android
General
P3
normal
VERIFIED FIXED
6 years ago
10 months ago

People

(Reporter: blassey, Assigned: gcp)

Tracking

({late-l10n})

unspecified
Firefox 16
ARM
Android
late-l10n
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(firefox15 fixed, firefox16 verified, firefox17 verified, blocking-fennec1.0 -, fennec15+)

Details

Attachments

(9 attachments, 5 obsolete attachments)

9.53 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
24.60 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
6.01 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
1.63 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
15.23 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
56.55 KB, image/png
Details
59.43 KB, image/png
Details
3.37 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
1.93 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
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
(Reporter)

Updated

6 years ago
Blocks: 710331
(Reporter)

Updated

6 years ago
Assignee: nobody → gpascutto
Priority: -- → P2
(Reporter)

Updated

6 years ago
tracking-fennec: --- → 11+
Duplicate of this bug: 647336
(Reporter)

Updated

5 years ago
Keywords: fennecnative-releaseblocker

Updated

5 years ago
Keywords: fennecnative-releaseblocker
Priority: P2 → P3

Comment 2

5 years ago
flagging for triage.  likely not 1.0 feature
blocking-fennec1.0: --- → ?
(Reporter)

Updated

5 years ago
blocking-fennec1.0: ? → -
(Reporter)

Updated

5 years ago
tracking-fennec: 11+ → 14+
(Assignee)

Updated

5 years ago
Depends on: 759393
(Assignee)

Comment 3

5 years ago
Note: this will add android.permission.READ_HISTORY_BOOKMARKS so we need to document it.
(Assignee)

Comment 4

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

Comment 5

5 years ago
Note that comment 4 talks about XUL->Native migration - I'm mentioning it here because I've refactoring/reusing code from there.
(Assignee)

Comment 6

5 years ago
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.
Attachment #628810 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 7

5 years ago
Created attachment 628812 [details] [diff] [review]
Patch 2. Add functionality to import Android bookmarks
Attachment #628812 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 8

5 years ago
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.
Attachment #628815 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 9

5 years ago
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 :)
Attachment #628818 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #628810 - Flags: review?(lucasr.at.mozilla) → review+
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
Attachment #628812 - Flags: review?(lucasr.at.mozilla) → review+
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 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 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 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.
(Assignee)

Comment 16

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

Comment 17

5 years ago
>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.
(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.)
Status: NEW → ASSIGNED
Summary: implement bookmarks and history import from Android System DBs → Implement bookmarks and history import from Android system DBs
(Assignee)

Comment 19

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

Comment 21

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

Comment 22

5 years ago
Created attachment 631476 [details] [diff] [review]
Patch 1. Extend UPDATE_OR_INSERT support
Attachment #628810 - Attachment is obsolete: true
Attachment #628812 - Attachment is obsolete: true
Attachment #628815 - Attachment is obsolete: true
Attachment #628818 - Attachment is obsolete: true
Attachment #628815 - Flags: review?(lucasr.at.mozilla)
Attachment #628818 - Flags: review?(lucasr.at.mozilla)
Attachment #631476 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 23

5 years ago
Created attachment 631477 [details] [diff] [review]
Patch 2. Refactor batch update functions into LocalDB
Attachment #631477 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 24

5 years ago
Created attachment 631478 [details] [diff] [review]
Patch 3. Import from Android functionality
Attachment #631478 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 25

5 years ago
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?
Attachment #631479 - Flags: review?(lucasr.at.mozilla)
Attachment #631479 - Flags: feedback?(ibarlow)
(Assignee)

Comment 26

5 years ago
Created attachment 631481 [details] [diff] [review]
Patch 5. Allow duplicate bookmarks if in another folder
Attachment #631481 - Flags: review?(lucasr.at.mozilla)
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 :-)
Attachment #631476 - Flags: review?(lucasr.at.mozilla) → review+
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 on attachment 631477 [details] [diff] [review]
Patch 2. Refactor batch update functions into LocalDB

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

Cool.
Attachment #631477 - Flags: review?(lucasr.at.mozilla) → review+
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?
Attachment #631478 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 31

5 years ago
>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 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 on attachment 631481 [details] [diff] [review]
Patch 5. Allow duplicate bookmarks if in another folder

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

Nice.
Attachment #631481 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 34

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

Comment 36

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

Comment 37

5 years ago
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.
Attachment #631479 - Attachment is obsolete: true
Attachment #631479 - Flags: review?(lucasr.at.mozilla)
Attachment #631479 - Flags: feedback?(ibarlow)
Attachment #634629 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #634629 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 39

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

Comment 40

5 years ago
Created attachment 634839 [details]
Screenshot of UX
(Assignee)

Comment 41

5 years ago
Created attachment 634840 [details]
Screenshot of UX 2
(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.
(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.
(Assignee)

Comment 44

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

Comment 45

5 years ago
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.
Attachment #634867 - Flags: review?(lucasr.at.mozilla)
(Reporter)

Updated

5 years ago
tracking-fennec: 14+ → 15+

Updated

5 years ago
Attachment #634867 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 46

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3da23241bba
https://hg.mozilla.org/integration/mozilla-inbound/rev/28754a456feb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a942ab4e7089
https://hg.mozilla.org/integration/mozilla-inbound/rev/d74e5dfa1626
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e795daed2f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c38e96f197e5

Updated

5 years ago
Flags: in-moztrap?(fennec)
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
(Assignee)

Comment 48

5 years ago
Created attachment 636350 [details] [diff] [review]
Patch 7. Fix broken isBookmark logic
Attachment #636350 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 49

5 years ago
>Backed out for native Android M7 failures:

They went green on retrigger an the code is unrelated, so I proclaim my innocence.

Comment 50

5 years ago
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.
Attachment #636350 - Flags: review?(lucasr.at.mozilla) → review+
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.
(Assignee)

Comment 52

5 years ago
Pushed with nits addressed. Thanks for pointing that one out rnewman, I've been looking at code like that for so long now that my brain must be half-fried.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb22a2cbd33
https://hg.mozilla.org/integration/mozilla-inbound/rev/217cf49f5f6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d5067a298dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/75c4ef84ea24
https://hg.mozilla.org/integration/mozilla-inbound/rev/34302be5af12
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c268dafb5a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/aff898bb348b
https://hg.mozilla.org/mozilla-central/rev/2bb22a2cbd33
https://hg.mozilla.org/mozilla-central/rev/217cf49f5f6f
https://hg.mozilla.org/mozilla-central/rev/6d5067a298dd
https://hg.mozilla.org/mozilla-central/rev/75c4ef84ea24
https://hg.mozilla.org/mozilla-central/rev/34302be5af12
https://hg.mozilla.org/mozilla-central/rev/8c268dafb5a4
https://hg.mozilla.org/mozilla-central/rev/aff898bb348b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16

Updated

5 years ago
Status: RESOLVED → VERIFIED
status-firefox16: --- → verified

Updated

5 years ago
Depends on: 768900

Updated

5 years ago
Depends on: 769216
(Assignee)

Updated

5 years ago
Blocks: 769896
(Assignee)

Updated

5 years ago
Attachment #631476 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #631477 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #631478 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #631481 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #634629 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #634867 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 54

5 years ago
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.
Attachment #636350 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Keywords: late-l10n

Updated

5 years ago
Attachment #631476 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #631477 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #631478 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #631481 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #634629 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #634867 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #636350 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 55

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/a2f85664f600
https://hg.mozilla.org/releases/mozilla-aurora/rev/dd7da389584f
https://hg.mozilla.org/releases/mozilla-aurora/rev/78cd60e01156
https://hg.mozilla.org/releases/mozilla-aurora/rev/6e20e6674811
https://hg.mozilla.org/releases/mozilla-aurora/rev/f57a59d18ede
https://hg.mozilla.org/releases/mozilla-aurora/rev/10c8f750741a
https://hg.mozilla.org/releases/mozilla-aurora/rev/fe6a55929aec
(Assignee)

Updated

5 years ago
status-firefox15: --- → fixed

Updated

5 years ago
Flags: in-moztrap?(fennec) → in-moztrap?(nicolae.cristian)
TCs were added in MozTrap and can be found here: https://moztrap.mozilla.org/manage/cases/?filter-suite=62
status-firefox17: --- → verified
Flags: in-moztrap?(nicolae.cristian) → in-moztrap+
You need to log in before you can comment on or make changes to this bug.