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)
Tracking
(firefox15 fixed, firefox16 verified, firefox17 verified, blocking-fennec1.0 -, fennec15+)
VERIFIED
FIXED
Firefox 16
People
(Reporter: blassey, Assigned: gcp)
References
Details
(Keywords: late-l10n)
Attachments
(9 files, 5 obsolete files)
9.53 KB,
patch
|
lucasr
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
24.60 KB,
patch
|
lucasr
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
6.01 KB,
patch
|
lucasr
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.63 KB,
patch
|
lucasr
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
15.23 KB,
patch
|
lucasr
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
56.55 KB,
image/png
|
Details | |
59.43 KB,
image/png
|
Details | |
3.37 KB,
patch
|
lucasr
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.93 KB,
patch
|
Margaret
:
review+
akeybl
:
approval-mozilla-aurora+
|
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•12 years ago
|
Assignee: nobody → gpascutto
Priority: -- → P2
Reporter | ||
Updated•12 years ago
|
tracking-fennec: --- → 11+
Reporter | ||
Updated•12 years ago
|
Keywords: fennecnative-releaseblocker
Updated•12 years ago
|
Keywords: fennecnative-releaseblocker
Priority: P2 → P3
Reporter | ||
Updated•12 years ago
|
blocking-fennec1.0: ? → -
Reporter | ||
Updated•12 years ago
|
tracking-fennec: 11+ → 14+
Assignee | ||
Comment 3•12 years ago
|
||
Note: this will add android.permission.READ_HISTORY_BOOKMARKS so we need to document it.
Assignee | ||
Comment 4•12 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•12 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•12 years ago
|
||
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•12 years ago
|
||
Attachment #628812 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 8•12 years ago
|
||
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•12 years ago
|
||
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 10•12 years ago
|
||
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 11•12 years ago
|
||
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 12•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 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.
Comment 18•12 years ago
|
||
(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•12 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.
Comment 20•12 years ago
|
||
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•12 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•12 years ago
|
||
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•12 years ago
|
||
Attachment #631477 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 24•12 years ago
|
||
Attachment #631478 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 25•12 years ago
|
||
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•12 years ago
|
||
Attachment #631481 -
Flags: review?(lucasr.at.mozilla)
Comment 27•12 years ago
|
||
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 28•12 years ago
|
||
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•12 years ago
|
||
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 30•12 years ago
|
||
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•12 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 32•12 years ago
|
||
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•12 years ago
|
||
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•12 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 35•12 years ago
|
||
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•12 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•12 years ago
|
||
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 38•12 years ago
|
||
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•12 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•12 years ago
|
||
Assignee | ||
Comment 41•12 years ago
|
||
Comment 42•12 years ago
|
||
(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•12 years ago
|
||
(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•12 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•12 years ago
|
||
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•12 years ago
|
tracking-fennec: 14+ → 15+
Updated•12 years ago
|
Attachment #634867 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 46•12 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•12 years ago
|
Flags: in-moztrap?(fennec)
Comment 47•12 years ago
|
||
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•12 years ago
|
||
Attachment #636350 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 49•12 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•12 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+
Comment 51•12 years ago
|
||
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•12 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
Comment 53•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
status-firefox16:
--- → verified
Assignee | ||
Updated•12 years ago
|
Attachment #631476 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #631477 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #631478 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #631481 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #634629 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #634867 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 54•12 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?
Updated•12 years ago
|
Attachment #631476 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #631477 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #631478 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #631481 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #634629 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #634867 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #636350 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 55•12 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•12 years ago
|
status-firefox15:
--- → fixed
Updated•12 years ago
|
Flags: in-moztrap?(fennec) → in-moztrap?(nicolae.cristian)
Comment 56•12 years ago
|
||
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+
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•