* Go to google.com * Bookmark it * Click on the AwesomeBar, go to the Bookmarks tab and delete the newly created bookmark * Press the back button to get out of the AwesomeBar * Click on the menu button Now, the "Bookmark" item still says the page is bookmarked even though we just removed it. Mark, is a patch wanted for this or isn't it expected to go back to the same page?

Similar bug 721776

(In reply to Michael Kohler [:michaelkohler] from comment #0) > Now, the "Bookmark" item still says the page is bookmarked even though we > just removed it. > > Mark, is a patch wanted for this or isn't it expected to go back to the same > page? Sounds like the the menu should say the page is not bookmarked any longer. A patch would be great.

no longer necessary with bookmarks observer

as described in https://bugzilla.mozilla.org/show_bug.cgi?id=721776#c16

These patches implement a bookmark observer. I've removed the changes from bug 721776 since the observer fixes both cases (both the AwesomeBar list and the menu item).

Comment on attachment 595240 [details] [diff] [review] (1/4) bookmark change observer looks like a good idea. i want lucas to have a look too.

Comment on attachment 595241 [details] [diff] [review] (2/4) refactor tabs to use bookmarks observer >diff --git a/mobile/android/base/Tabs.java b/mobile/android/base/Tabs.java >+ public void closeTab(final Tab tab, Tab nextTab) { > int tabId = tab.getId(); >+ tab.onDestroy(); > removeTab(tabId); >- tab.removeAllDoorHangers(); > GeckoApp.mAppContext.mMainHandler.post(new Runnable() { > public void run() { >- GeckoApp.mAppContext.onTabsChanged(closedTab); >+ GeckoApp.mAppContext.onTabsChanged(tab); > GeckoApp.mBrowserToolbar.updateTabCountAndAnimate(Tabs.getInstance().getCount()); > GeckoApp.mDoorHangerPopup.updatePopup(); >- GeckoApp.mAppContext.hidePlugins(closedTab, true); >+ GeckoApp.mAppContext.hidePlugins(tab, true); > } > }); Can we do the tab.onDestroy() _after_ we have finished using it completely? I'd like to know that the tab is still whole until we are passing it around. We could call tab.onDestory() after the GeckoApp.mAppContext.hidePlugins(tab, true); call? I worry that more is added to onDestroy and then we have code try to access something that was freed.

Comment on attachment 595242 [details] [diff] [review] (3/4) remove refreshBookmarks() I must have missed where we are updating the cursor so the list is updated after removing the bookmark.

(In reply to Mark Finkle (:mfinkle) from comment #10) > Comment on attachment 595242 [details] [diff] [review] > (3/4) remove refreshBookmarks() > > I must have missed where we are updating the cursor so the list is updated > after removing the bookmark. That's the cool part. According to http://developer.android.com/reference/android/content/ContentResolver.html#notifyChange%28android.net.Uri,%20android.database.ContentObserver%29, notifyChange() will automatically notify CursorAdapters, which we use to populate the list.

Comment on attachment 595240 [details] [diff] [review] (1/4) bookmark change observer Review of attachment 595240 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/db/AndroidBrowserDB.java @@ +275,5 @@ > > if (updated == 0) > cr.insert(BOOKMARKS_CONTENT_URI_POST_11, values); > + > + cr.notifyChange(BOOKMARKS_CONTENT_URI_POST_11, null); Android's content provider does that for you. You don't need to explicitly notify changes here. ::: mobile/android/base/db/LocalBrowserDB.java @@ +368,5 @@ > cr.delete(appendProfile(Bookmarks.CONTENT_URI), > Bookmarks.URL + " = ?", > new String[] { uri }); > + > + cr.notifyChange(appendProfile(Bookmarks.CONTENT_URI), null); Those notifications should be implemented on ContentProvider level, not in LocalBrowserDB.

Comment on attachment 595243 [details] [diff] [review] (4/4) use geckoasynctask for bookmark removal Review of attachment 595243 [details] [diff] [review]: ----------------------------------------------------------------- What's the real benefit of using GeckoAsyncTask here? I'm generally against the whole idea of GeckoAsyncTask. It tries to mimic Android's AsyncTask API but it doesn't fully implement it (e.g. lacks cancellation support). This can be a great source of confusion. I'd prefer a simple API that ensures that we start AsyncTasks on main thread so that their onPostExecute() methods are called on the right thread. Anyway, this patch doesn't cause any harm, so giving a r+.

(In reply to Brian Nicholson (:bnicholson) from comment #11) > (In reply to Mark Finkle (:mfinkle) from comment #10) > > Comment on attachment 595242 [details] [diff] [review] > > (3/4) remove refreshBookmarks() > > > > I must have missed where we are updating the cursor so the list is updated > > after removing the bookmark. > > That's the cool part. According to > http://developer.android.com/reference/android/content/ContentResolver. > html#notifyChange%28android.net.Uri,%20android.database.ContentObserver%29, > notifyChange() will automatically notify CursorAdapters, which we use to > populate the list. ... and you tested this to work?

Comment on attachment 595242 [details] [diff] [review] (3/4) remove refreshBookmarks() r+ for being cool

(In reply to Lucas Rocha (:lucasr) from comment #12) > Comment on attachment 595240 [details] [diff] [review] > (1/4) bookmark change observer > > Review of attachment 595240 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/db/AndroidBrowserDB.java > @@ +275,5 @@ > > > > if (updated == 0) > > cr.insert(BOOKMARKS_CONTENT_URI_POST_11, values); > > + > > + cr.notifyChange(BOOKMARKS_CONTENT_URI_POST_11, null); > > Android's content provider does that for you. You don't need to explicitly > notify changes here. > > ::: mobile/android/base/db/LocalBrowserDB.java > @@ +368,5 @@ > > cr.delete(appendProfile(Bookmarks.CONTENT_URI), > > Bookmarks.URL + " = ?", > > new String[] { uri }); > > + > > + cr.notifyChange(appendProfile(Bookmarks.CONTENT_URI), null); > > Those notifications should be implemented on ContentProvider level, not in > LocalBrowserDB. Thanks, good tips. Here's an updated patch. Unfortunately, this bug doesn't play nicely with bug 716918. When bookmarks are changed while looking at the bookmarks tab in the AwesomeScreen, the view quickly disappears and is redrawn. This effect is really terrible when syncing since the adapter receives constant change notifications. I think this is caused by us returning an empty cursor in AwesomeBarTabs#getChildrenCursor() before updating the cursor asynchronously. I was able to fix this by changing getChildrenCursor() to be synchronous, but this obviously isn't ideal; let me know if you have any ideas for fixing it properly.

Comment on attachment 595594 [details] [diff] [review] (1/4) bookmark change observer v2 Review of attachment 595594 [details] [diff] [review]: ----------------------------------------------------------------- I'd prefer the changes to add the notifyChange() in the ContentProvider to go on a separate patch.

(In reply to Brian Nicholson (:bnicholson) from comment #16) > Created attachment 595594 [details] [diff] [review] > (1/4) bookmark change observer v2 > > (In reply to Lucas Rocha (:lucasr) from comment #12) > > Comment on attachment 595240 [details] [diff] [review] > > (1/4) bookmark change observer > > > > Review of attachment 595240 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: mobile/android/base/db/AndroidBrowserDB.java > > @@ +275,5 @@ > > > > > > if (updated == 0) > > > cr.insert(BOOKMARKS_CONTENT_URI_POST_11, values); > > > + > > > + cr.notifyChange(BOOKMARKS_CONTENT_URI_POST_11, null); > > > > Android's content provider does that for you. You don't need to explicitly > > notify changes here. > > > > ::: mobile/android/base/db/LocalBrowserDB.java > > @@ +368,5 @@ > > > cr.delete(appendProfile(Bookmarks.CONTENT_URI), > > > Bookmarks.URL + " = ?", > > > new String[] { uri }); > > > + > > > + cr.notifyChange(appendProfile(Bookmarks.CONTENT_URI), null); > > > > Those notifications should be implemented on ContentProvider level, not in > > LocalBrowserDB. > > Thanks, good tips. Here's an updated patch. > > Unfortunately, this bug doesn't play nicely with bug 716918. When bookmarks > are changed while looking at the bookmarks tab in the AwesomeScreen, the > view quickly disappears and is redrawn. This effect is really terrible when > syncing since the adapter receives constant change notifications. I think > this is caused by us returning an empty cursor in > AwesomeBarTabs#getChildrenCursor() before updating the cursor > asynchronously. I was able to fix this by changing getChildrenCursor() to be > synchronous, but this obviously isn't ideal; let me know if you have any > ideas for fixing it properly. Although relying on content observers is kind of cool, this is a bit worrying from a UX perspective for the reasons you've already mentioned (e.g. sync causing constant refreshes on the bookmarks tab). I think patch 3/4 would have to be changed to disable contentobserver on the bookmarks cursor adapter and only temporarily enable it when the user removes a bookmark. Or something like this? But even with that change, refreshing the whole list when removing a bookmark is not optimal. Maybe there's a way to simply destroy the specific view for the removed bookmark? In that case, you wouldn't have to rely on the ContentObserver anymore. If that's not possible, maybe you could find a way to simply refresh the respective children cursor instead of refreshing the whole bookmark list? As for the getChildrenCursor(), you can't really do synchronous queries there otherwise we'll get StrictMode exceptions. Need to find a way to keep doing it asynchronously without clearing the screen when a bookmark is removed. Just throwing ideas here. I feel that this bug needs further investigation before pushing the current patches.

Comment on attachment 595242 [details] [diff] [review] (3/4) remove refreshBookmarks() Just giving feedback- for the reasons described in comment 18.

See also bug 725540 for a recently recorded crash

The issue with the flashing screen was caused by autoRequery in CursorAdapter (http://developer.android.com/reference/android/widget/CursorAdapter.html#CursorAdapter%28android.content.Context,%20android.database.Cursor,%20boolean%29). The cursor adapter was receiving the change notification, then calling requery() on the cursor to refresh the view. requery() is synchronous, but we want to query the DB asynchronously, so we were passing back an empty MatrixCursor to use temporarily while we fetched the actual result asynchronously. This resulted in the flashing between the empty cursor and the actual list. Unfortunately, the CursorAdapter constructor that accepts an autoRequery boolean is hidden in ResourceCursorTreeAdapter (which is the parent of SimpleCursorTreeAdapter, which is the parent of our BookmarksListAdapter). I had to resort to reflection to disable it. To replace auto-requery, I added a ContentObserver that only updates the cursor asynchronously.

Comment on attachment 596865 [details] [diff] [review] (3/4) use custom observer for BookmarksListAdapter >diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java >+ try { >+ // use reflection to disable auto-requery >+ Class cls = Class.forName("android.widget.CursorTreeAdapter"); >+ Field field = cls.getDeclaredField("mAutoRequery"); >+ field.setAccessible(true); >+ field.set(mBookmarksAdapter, false); >+ } catch (Exception e) { >+ Log.e(LOGTAG, e.getMessage()); >+ } If this fails and we log, does it mean we get flashy updates? This isn't my favorite thing to do here, but it's not completely horrible. I think/hope we can remove this when the >+ // register an asynchronous custom observer to replace the synchronous auto-requery >+ mContentObserver = new ContentObserver(GeckoAppShell.getHandler()) { >+ public void onChange(boolean selfChange) { >+ // The group cursor doesn't ever need to change because it just holds the >+ // mobile/desktop folders, but we do need to update the children cursors. >+ Cursor groupCursor = mBookmarksAdapter.getCursor(); >+ groupCursor.moveToPosition(-1); >+ while (groupCursor.moveToNext()) { >+ String guid = groupCursor.getString(groupCursor.getColumnIndexOrThrow(Bookmarks.GUID)); >+ // We need to do this in a AsyncTask because we're on the main thread >+ new RefreshChildrenCursorTask(groupCursor.getPosition()).execute(guid); Can we rename this to RefreshChildCursorTask? It doesn't sound right as "Children"

oops: I think/hope we can remove this when the bookmark code is re-written to not use a tree cursor

According to margaret, we're killing the tree cursor in bug 722020. We can wait to hear an ETA from her to determine whether to wait or push this now.

(In reply to Brian Nicholson (:bnicholson) from comment #24) > According to margaret, we're killing the tree cursor in bug 722020. We can > wait to hear an ETA from her to determine whether to wait or push this now. It looks like that patch probably won't land until Wednesday at the earliest. I'm going to talk to Madhava tomorrow to sort out the UX details for that bug, so I can give you a better estimate after that.

Comment on attachment 596865 [details] [diff] [review] (3/4) use custom observer for BookmarksListAdapter Review of attachment 596865 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: mobile/android/base/AwesomeBarTabs.java @@ +286,5 @@ > + // use reflection to disable auto-requery > + Class cls = Class.forName("android.widget.CursorTreeAdapter"); > + Field field = cls.getDeclaredField("mAutoRequery"); > + field.setAccessible(true); > + field.set(mBookmarksAdapter, false); Ouch :-)

http://hg.mozilla.org/integration/mozilla-inbound/rev/d6b0a72dd512 http://hg.mozilla.org/integration/mozilla-inbound/rev/490c6baef353 http://hg.mozilla.org/integration/mozilla-inbound/rev/c430b0da8334 http://hg.mozilla.org/integration/mozilla-inbound/rev/e963253d4421 http://hg.mozilla.org/integration/mozilla-inbound/rev/0f5f2636d588

this is just a split from patch #3 to keep the CR stuff in a separate patch

Comment on attachment 595594 [details] [diff] [review] (1/4) bookmark change observer v2 [Approval Request Comment] Adds bookmark observer support. Low risk

Comment on attachment 595241 [details] [diff] [review] (2/4) refactor tabs to use bookmarks observer [Approval Request Comment] Fixes bookmarks menu item not updating. Low risk.

Comment on attachment 597237 [details] [diff] [review] (2.5/4) refactor content resolver [Approval Request Comment] Minor refactoring. Very low risk.

Comment on attachment 596865 [details] [diff] [review] (3/4) use custom observer for BookmarksListAdapter [Approval Request Comment] Uses observer to dynamically update AwesomeScreen results. Low risk.

Comment on attachment 595243 [details] [diff] [review] (4/4) use geckoasynctask for bookmark removal [Approval Request Comment] Minor refactoring. Very low risk.

http://hg.mozilla.org/mozilla-central/rev/d6b0a72dd512 http://hg.mozilla.org/mozilla-central/rev/490c6baef353 http://hg.mozilla.org/mozilla-central/rev/c430b0da8334 http://hg.mozilla.org/mozilla-central/rev/e963253d4421 http://hg.mozilla.org/mozilla-central/rev/0f5f2636d588

Comment on attachment 595594 [details] [diff] [review] (1/4) bookmark change observer v2 [Triage Comment] Mobile only - approved for Aurora 12 and Beta 11.

(at least) patch 1 of this set fails to apply: Justin@ORION /d/sources/mozilla-beta $ hg qpush applying bookmarks_observer.patch patching file mobile/android/base/db/LocalBrowserDB.java Hunk #1 FAILED at 42 Hunk #2 FAILED at 372 2 out of 2 hunks FAILED -- saving rejects to file mobile/android/base/db/LocalBrowserDB.java.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh bookmarks_observer.patch

(In reply to Justin Wood (:Callek) from comment #36) > (at least) patch 1 of this set fails to apply: > > Justin@ORION /d/sources/mozilla-beta > $ hg qpush > applying bookmarks_observer.patch > patching file mobile/android/base/db/LocalBrowserDB.java > Hunk #1 FAILED at 42 > Hunk #2 FAILED at 372 > 2 out of 2 hunks FAILED -- saving rejects to file > mobile/android/base/db/LocalBrowserDB.java.rej > patch failed, unable to continue (try -v) > patch failed, rejects left in working dir > errors during apply, please fix and refresh bookmarks_observer.patch Justin, I don't think it's worth pushing this to aurora or beta at this point...

Verified fixed on: Firefox 13.0a1 (2012-02-29) 20120229031108 http://hg.mozilla.org/mozilla-central/rev/30b4f99a137c -- Device: Samsung Galaxy S2 OS: Android 2.3.4

Comment on attachment 595594 [details] [diff] [review] (1/4) bookmark change observer v2 clearing beta approvals per akeybl on IRC, No Fennec Native Shipping for Gecko 11