Like bug 918494.

We queue the async action to remove bookmarks at [1], then trigger the toast. From a UX perspective, the user should probably only receive the toast once the bookmark has actually been removed. [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=b72560289748#2431

Actually, there is an alternate code path to worry about [1] from bug 918494 so this applies to adding bookmarks as well. Two approaches that I see are to add the toasts to the tab methods, i.e. Tab.*BookmarkAndToast, or to pull the code out from the Tab class. I'm leaning towards the former. [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=b72560289748#2435

Moved the Toast notification to removeBookmark() method of Tab

Added fixes for both addBookmark() and removeBookmark().

Comment on attachment 8443652 [details] [diff] [review] 1024145.patch - Fixes for addBookmark and removeBookmark Review of attachment 8443652 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/BrowserApp.java @@ +2415,5 @@ > > return true; > } > > + void showBookmarkAddedToast(){ nit: It is only my opinion but `protected` is generally preferable. @@ +2452,3 @@ > item.setIcon(R.drawable.ic_menu_bookmark_add); > } else { > + tab.addBookmark(BrowserApp.this); nit: We prefer not to add excess whitespace to the end of lines - can you remove it here? Let me know if you're curious for an explanation. ::: mobile/android/base/Tab.java @@ +446,5 @@ > } > }); > } > > + public void addBookmark(final BrowserApp browserApp) { The Tab class should not have to know about BrowserApp - is there another way you can do this? Can you use a listener or pass in the ButtonToast (I think I prefer the former, because a tab should be able to add/remove a bookmark with explicitly notifying the user, and use the listener gives more flexibility to that end)? @@ +455,5 @@ > if (url == null) > return; > > BrowserDB.addBookmark(getContentResolver(), mTitle, url); > + nit: Excess end-of-line whitespace.

Added fixes as suggested by :mcomella

Comment on attachment 8443905 [details] [diff] [review] p2.patch - Added fixes so that Tab.java doesn't have to know about BrowserApp Review of attachment 8443905 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review until the questions are answered. ::: mobile/android/base/BrowserApp.java @@ +251,5 @@ > case MENU_UPDATED: > if (Tabs.getInstance().isSelectedTab(tab)) { > invalidateOptionsMenu(); > } > + if(data!=null) { nit: Coding style. We put spaces between operators (e.g. `data != null`) and between the if keyword and parens (e.g. `if (...`). Here and below. See the rest of the file for examples. ::: mobile/android/base/Tab.java @@ +454,5 @@ > if (url == null) > return; > > BrowserDB.addBookmark(getContentResolver(), mTitle, url); > + Tabs.getInstance().notifyListeners(Tab.this, Tabs.TabEvents.MENU_UPDATED, "Bookmark:Added"); Why are using the MENU_UPDATED event here? I think BrowserDB.addBookmark should notify the listeners of this event anyway.

Hey, Hathibelagal. Are you still working on this?

I am new to mozilla, can I take it up ? If yes then please provide repo, docs details to start on.

Hathibelagal, I'm unassigning you and assigning Ankur. Welcome to Bugzilla, Ankur! :) To set up a build environment, you can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "mcomella" and you can find me in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC Thanks and happy coding! ^_^

@Michael, thanks for sharing the instructions, it helped and now I am able to build and deploy fennec in my device.

Can you also share the approach for reproducing/debugging this bug ?

At first place, I am not absolutely clear about bug description, could you explain it from end-user persepective.

Currently, when bookmarks are removed, we are saving the data asynchronously. After the asynchronous action is queued, we display a toast saying the bookmark has been removed. However, we should really wait until *after* the bookmark is removed from the database, i.e. the asynchronous action completed, before displaying the toast. If the remove action is fast, it will be no different from the end-user perspective. If the remove action is slow, the users will experience a delay before the toast is delayed after removing the bookmark. However, this is more accurate to what is occuring in the system and gives us the opportunity to catch errors with removing the bookmark, and display an appropriate toast. Similarly, there are also some similar issues with adding bookmarks mentioned in comment 2. These can be reproduced by selecting, and then deselecting the bookmarks star from the options menu.

Can you give a walk through of the relevant code section.

(In reply to Ankur Garg from comment #15) > Can you give a walk through of the relevant code section. I'm not sure how to do that - do you have specific questions? comment 1 and comment 2 should point you in the right direction.

I was thinking of taking this one up, if no one else is working on it. (In reply to Michael Comella (:mcomella) from comment #7) > Why are using the MENU_UPDATED event here? I think BrowserDB.addBookmark > should notify the listeners of this event anyway. Following on from this, I couldn't find anywhere in the BrowserDB code that notifies the Tabs listeners when a bookmark is added/removed. Perhaps we could add BOOKMARK_ADDED/REMOVED to the TabEvent enum, rather than using MENU_UPDATED?

(In reply to Tristan Pollitt from comment #17) > I was thinking of taking this one up, if no one else is working on it. Ankur last commented only 2 days ago, so we should give him an opportunity to take a stab at this.

It's been a week since Ankur's last comment, so it seems like he's no longer working on it. Here's a patch I've created, which I've tested manually on Android 4.1.2, are there any automated tests that need to be updated or created for this?

Comment on attachment 8471710 [details] [diff] [review] bookmark.patch Review of attachment 8471710 [details] [diff] [review]: ----------------------------------------------------------------- I like it.

Try is closed (bug 1040308), so I'll push a test run once it opens back up.

Any updates on this?

My apologies, I didn't mean to clear my needinfo flag before, and I completely overlooked this! try push coming...

try: https://tbpl.mozilla.org/?tree=Try&rev=bfc3a8283020

Looking green, should be able to check-in :)