968738 - Bookmarks opened from home screen are not marked as bookmarked

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Flaviu Cos]

Environment: 
Device: Google Nexus 7 (Android 4.4.2);
Build: Nightly 30.0a1 (2014-02-04);

Steps to reproduce:
1. Load any page (e.g. ebay.com);
2. Bookmark the page;
3. On the toaster notification tap on Options;
4. Tap on Add to Home Screen;
5. Go to the home screen and tap on the bookmark;
6. Check if the opened page is bookmarked.

Expected result:
The page appears as bookmarked.

Actual result:
The page does not appear as bookmarked.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Don't short-circuit most tab updates if the URL is the same

I'm assuming this short-circuiting logic was to added prevent flickering favicons/page titles, but we don't want to miss other state updates.

Comment 2

[Brian Nicholson (:bnicholson)]

An overview of what's happening here:

1) All tabs loaded from Java are created as stubs, meaning their URLs are set before Gecko responds with a Tab:Added message (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#758).
2) We get a location changed event, but we return early because the URL is the same (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#612).
3) Consequently, updateBookmark never gets called.

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: Don't short-circuit most tab updates if the URL is the same, v2

Minor update that adds an updateBookmark() call to the Tab constructor so that we don't need to rely on the location change event for updates. This means we can avoid doing extra updateBookmark() calls if oldUrl == url, like we had before.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8373545 [details] [diff] [review]
Don't short-circuit most tab updates if the URL is the same, v2

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

Imma say this looks good to me, under the assumption that you've tested the crap out of this and we have several weeks to notice if titles or icons don't get updated correctly.

Thanks for doing the digging!

::: mobile/android/base/Tab.java
@@ +419,4 @@
>                  }
>  
> +                mBookmark = BrowserDB.isBookmark(getContentResolver(), url);
> +                mReadingListItem = BrowserDB.isReadingListItem(getContentResolver(), url);

/me files bug to only hit the DB *once* on every tab load.
/me files bug to not hit the DB on every tab load.

Comment 5

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/e2b1310b59a7

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e2b1310b59a7