Closed Bug 710330 Opened 12 years ago Closed 12 years ago

Implement bookmarks and history import from Android system DBs

Categories

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

ARM
Android
defect

Tracking

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

VERIFIED FIXED
Firefox 16
Tracking Status
firefox15 --- fixed
firefox16 --- verified
firefox17 --- verified
blocking-fennec1.0 --- -
fennec 15+ ---

People

(Reporter: blassey, Assigned: gcp)

References

Details

(Keywords: late-l10n)

Attachments

(9 files, 5 obsolete files)

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
Blocks: 710331
Assignee: nobody → gpascutto
Priority: -- → P2
tracking-fennec: --- → 11+
Priority: P2 → P3
flagging for triage.  likely not 1.0 feature
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → -
tracking-fennec: 11+ → 14+
Depends on: 759393
Note: this will add android.permission.READ_HISTORY_BOOKMARKS so we need to document it.
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.
Note that comment 4 talks about XUL->Native migration - I'm mentioning it here because I've refactoring/reusing code from there.
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)
Attachment #628812 - Flags: review?(lucasr.at.mozilla)
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)
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.
>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
>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
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.)
>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.
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)
Attachment #631477 - Flags: review?(lucasr.at.mozilla)
Attachment #631478 - Flags: review?(lucasr.at.mozilla)
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)
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+
>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+
>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.
>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
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+
>"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.
Attached image Screenshot of UX
Attached image 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.
>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.
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)
tracking-fennec: 14+ → 15+
Attachment #634867 - Flags: review?(lucasr.at.mozilla) → review+
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
Attachment #636350 - Flags: review?(lucasr.at.mozilla)
>Backed out for native Android M7 failures:

They went green on retrigger an the code is unrelated, so I proclaim my innocence.
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.
Status: RESOLVED → VERIFIED
Depends on: 768900
Depends on: 769216
Blocks: 769896
Attachment #631476 - Flags: approval-mozilla-aurora?
Attachment #631477 - Flags: approval-mozilla-aurora?
Attachment #631478 - Flags: approval-mozilla-aurora?
Attachment #631481 - Flags: approval-mozilla-aurora?
Attachment #634629 - Flags: approval-mozilla-aurora?
Attachment #634867 - Flags: approval-mozilla-aurora?
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?
Keywords: late-l10n
Attachment #631476 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #631477 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #631478 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #631481 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #634629 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #634867 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #636350 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Flags: in-moztrap?(nicolae.cristian) → in-moztrap+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.