766940 - Unable to remove bookmarks from the menu, bookmark checkmark/filled star icon remain in view

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (06/21) - Screenshot

See screenshot.

Add a bookmark, remove a bookmark. 

Checkmark remains in view.

--
Nightly (06.21)
Galaxy Nexus (Android 4.0.4)

Comment 1

[:Margaret Leibovic]

Lucas and Sriram were talking about this on IRC yesterday morning. Maybe they already figured out what caused this.

Comment 2

[Aaron Train [:aaronmt]]

Adding Wes because I see lots of bookmark fun stuff on Inbound in bug 759041 (re comment #0, correction, filed against Inbound).

Comment 3

[Cristian Nicolae (:xti)]

Good build: 06/03

Bad build: 06/04

Possible regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d0ebcaa7efb5&tochange=dd6ec482a85d

--
Device: Galaxy Nexus
OS: Android 4.0.2

Comment 4

[Aaron Train [:aaronmt]]

I still can't remove a bookmark (the checkmark and now the star icon) remain.

Comment 5

[Aaron Train [:aaronmt]]

Upping to major loss as it's the primary UI for bookmark removal

Comment 6

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This depends on "tab's" bookmark state and not on the menu item.
Also, the checked state is made to be proper.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 639524 [details] [diff] [review]
Patch

We do the same kind of code in onPrepareOptionsMenu:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#592

Since the UI is now visible in the actionbar, we can't wait for onPrepareOptionsMenu to be called on it's own, so this patch works.

Could we just invalidate the menu here? Or is that overkill.

I don't like needing to update the UI all over the place. It doesn't scale well. It might be better to add an on change listener, so we can update the UI no matter how the tab bookmark state changes.

I assume we invalidate the menu when loading new pages? If so, let's do it here too, in case the UI changes we only need to update the code in the onPrepareOptionsMenu.

Thoughts?

Comment 8

[Sriram Ramasubramanian [:sriram]]

I thought of doing that, but in one of the patches, Brad said that it's an overkill to call "invalidateOptionsMenu()" and go through everything, for a single menu item change. Hence, I changed the part that does it for bookmarks alone.

I agree that it's twice the hassle. But it's more of performance to do the part we want.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 639524 [details] [diff] [review]
Patch

I disagree. In fact, as I look in the existing code, I see invalidateOptionsMenu used in many places to update a single menu... and I like it. It's also a pattern we seem to be using.

We should be centralizing the menu update code.

Please change this patch to remove the setChecked and setIcon calls, and add a single invalidateOptionsMenu call.

Comment 10

[Lucas Rocha (:lucasr)]

Assigning to Sriram as he's working on it.

Comment 11

[Tony Chung [:tchung]]

I can reproduce this on Fx15 beta 1, ASUS transformer.  Please fix for beta 2!

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Sriram - Can we get an updated patch? We need this for Fx15

Comment 13

[Sriram Ramasubramanian [:sriram]]

I tried using invalidateOptionsMenu(). That won't work.
We check for "tab.isBoomark()" and do a add/removeBookmark(). This basically sends a message to Gecko -- which responds -- which updates our Java side Tab. But invalidateOptionsMenu() is called before all these messaging happen. So, the state never gets updated.

The patch I posted doesn't wait for Gecko (just like now). It updates the state without waiting for Gecko.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

If we send "a message to Gecko -- which responds -- which updates our Java side Tab" then we should be able to update the menu from that, right?

Comment 15

[Sriram Ramasubramanian [:sriram]]

Basically we are updating a DB -- which informs a ContentResolver -- which calls an updateBoomark(). Do I want to do UI handling there?

If so, the optimization to make it look faster -- a tap and I bookmarked -- instead of waiting for all DB stuff to happen and then late update -- will be lost? Do we want to lose this "fast UI response" faking thing?

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Let me be clear: I do not want UI menu update code strung out all over the source code.

Comment 17

[Sriram Ramasubramanian [:sriram]]

I'll post a patch that might regress the time taken to update the star icon soon -- by calling invalidateOptionsMenu() from updateBookmark()

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Why can't we add something like this to the BrowserToolbar or BrowserApp?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#107

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #17)
> I'll post a patch that might regress the time taken to update the star icon
> soon -- by calling invalidateOptionsMenu() from updateBookmark()

If you mean Tab.updateBookmark() please don't post a patch. That is too tightly coupled and I will r- it too.

Comment 20

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #18)
> Why can't we add something like this to the BrowserToolbar or BrowserApp?
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.
> java#107

Doing something like this won't help either. If you look into updateBookmark(), it still calls the DB to know if the page is a bookmark or not and then updates its copy of mBookmark. Do we want to have an observer in BrowserApp/Toolbar that calls DB? The only place (other than my option 1) is to add it in Tab.updateBookmark() - after updating the mBookmark. I'm not up for adding a ContentObserver to BrowserApp/Toolbar unnecessarily, when we have that overhead with every Tab already.

Comment 21

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Another r- ready patch (;)) :
This does the invalidateOptionsMenu() in Tab.updateBookmark(). The speed of icon change didnt regress. (which raises a question: why did we do it in 2 places earlier then?)

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 644416 [details] [diff] [review]
Patch

This is better, but we can make it even better:
* Add BOOKMARK as a TabEvent here http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#354
* Fire the tab listener, passing BOOKMARK, instead of directly calling invalidateOptionsMenu, like we do here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#207
* Add a case to handle BOOKMARK here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#65
(you can collapse the START and STOP cases along with it

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

[triage comment]
tracking for both 15/16 just to be on the safe side.

Comment 24

[Lucas Rocha (:lucasr)]

Just quick note: this bug also has to account for the reading list menu item (which behaves pretty much the same way than the bookmark one).

Comment 25

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Invalidate options menu for soft menu button

This is technically a separate bug, but I'll just put it here. I noticed that the "Reading List" menu item is always greyed out on the Nexus; we need to invalidate it here like we do in other places.

Comment 26

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 645093 [details] [diff] [review]
Part 1: Invalidate options menu for soft menu button

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

This looks good to me.

Comment 27

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Notify MENU_UPDATED TabEvent when menu items change

Implemented comment 22. I made a generic MENU_UPDATED event so it could be used by both bookmarks and reader mode.

Comment 28

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Notify MENU_UPDATED TabEvent when menu items change, v2

I merged updateBookmark() and updateReadingListItem() into a single method. They do virtually the same thing, so we might as well handle them both simultaneously for performance reasons (and avoid invalidating the menu twice every time). And a reading list item is technically a bookmark anyway.

Comment 29

[Matt Brubeck (:mbrubeck)]

Comment on attachment 645102 [details] [diff] [review]
Part 2: Notify MENU_UPDATED TabEvent when menu items change, v2

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

::: mobile/android/base/GeckoApp.java
@@ +448,5 @@
>      }
>  
>      @Override
>      public void invalidateOptionsMenu() {
> +        Log.d("BRN", "invalidate");

Remember to remove this.

Comment 30

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #28)
> Created attachment 645102 [details] [diff] [review]
> Part 2: Notify MENU_UPDATED TabEvent when menu items change, v2
> 
> I merged updateBookmark() and updateReadingListItem() into a single method.
> They do virtually the same thing, so we might as well handle them both
> simultaneously for performance reasons (and avoid invalidating the menu
> twice every time). And a reading list item is technically a bookmark anyway.

Brian, I think your patch is missing the case when the tab gets a Content:ReaderEnabled event and enables the the reading list menu item, no?

Comment 31

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Notify MENU_UPDATED TabEvent when menu items change, v3

(In reply to Lucas Rocha (:lucasr) from comment #30)
> Brian, I think your patch is missing the case when the tab gets a
> Content:ReaderEnabled event and enables the the reading list menu item, no?

Thanks. I had filed bug 776740 for this, but since this is just because of the Content:ReaderEnabled message like you said, I'll just go ahead and fix it here.

Comment 32

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #31)
> Created attachment 645455 [details] [diff] [review]
> Part 2: Notify MENU_UPDATED TabEvent when menu items change, v3
> 
> (In reply to Lucas Rocha (:lucasr) from comment #30)
> > Brian, I think your patch is missing the case when the tab gets a
> > Content:ReaderEnabled event and enables the the reading list menu item, no?
> 
> Thanks. I had filed bug 776740 for this, but since this is just because of
> the Content:ReaderEnabled message like you said, I'll just go ahead and fix
> it here.

I confirm that your patch fixes bug 767959 too. Add a reference to it on the commit message.

Comment 33

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8d64c2ce1cc0
http://hg.mozilla.org/integration/mozilla-inbound/rev/9ce69db8aac3

Comment 34

[Brian Nicholson (:bnicholson)]

Comment on attachment 645093 [details] [diff] [review]
Part 1: Invalidate options menu for soft menu button

[Approval Request Comment]
Bug caused by (feature/regressing bug #): ?
User impact if declined: menu will not be updated on phones with soft menu button
Testing completed (on m-c, etc.): just landed m-i
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 35

[Brian Nicholson (:bnicholson)]

Comment on attachment 645455 [details] [diff] [review]
Part 2: Notify MENU_UPDATED TabEvent when menu items change, v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): ?
User impact if declined: bookmarks cannot be removed from menu
Testing completed (on m-c, etc.): just landed m-i
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8d64c2ce1cc0
https://hg.mozilla.org/mozilla-central/rev/9ce69db8aac3

Comment 38

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/01d29af0802e
http://hg.mozilla.org/releases/mozilla-aurora/rev/36e6fae94685

Comment 39

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/releases/mozilla-beta/rev/3034c31ae012
http://hg.mozilla.org/releases/mozilla-beta/rev/f8f9a93c6460