Closed Bug 968738 Opened 7 years ago Closed 7 years ago

Bookmarks opened from home screen are not marked as bookmarked

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox27 --- affected
firefox28 --- affected
firefox29 --- affected
firefox30 --- affected
fennec + ---

People

(Reporter: cos_flaviu, Assigned: bnicholson)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

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.
tracking-fennec: --- → ?
Assignee: nobody → bnicholson
tracking-fennec: ? → +
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.
Attachment #8372747 - Flags: review?(rnewman)
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.
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.
Attachment #8372747 - Attachment is obsolete: true
Attachment #8372747 - Flags: review?(rnewman)
Attachment #8373545 - Flags: review?(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.
Attachment #8373545 - Flags: review?(rnewman) → review+
Blocks: 970604
https://hg.mozilla.org/mozilla-central/rev/e2b1310b59a7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Blocks: 971232
Blocks: 971239
You need to log in before you can comment on or make changes to this bug.