Closed Bug 907172 Opened 7 years ago Closed 7 years ago

[FIG] The desktop bookmarks are displayed only after app restart

Categories

(Firefox for Android :: Awesomescreen, defect, P1)

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: AdrianT, Assigned: Margaret)

References

Details

Attachments

(2 files)

Attached image screenshot
Asus Transformer TF101
Android 4.0.4
Figgy Fennec 2013-08-19

Steps to reproduce:
1) Setup sync on the device from the settings menu
2) When asked what to do choose to go to settings -> check to make sure sync is complete in the system accounts
3) Go back to Firefox and open about:home

Expected results:
The Desktop Bookmarks folder is displayed in the bookmarks page

Actual results:
All the bookmarks and history is imported but the Desktop Bookmarks folder is not displayed until I restart the app - please see the screenshot with the synced tabs but no Default Bookmarks folder in about:home
Priority: -- → P1
Are you sure this has to do with restarting the app, as opposed to just closing and reopening about:home? Also, syncing isn't necessarily immediate, so there definitely can be a delay before your desktop bookmarks appear.

Regardless, we probably do need to add an observer like this:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#174
Assignee: nobody → margaret.leibovic
I have tried opening new about:home instances, loading bookmarks from top bookmarks and then going into edit mode, changing tabs, reloading about:home.

Also after setting up sync I went to settings and waited for sync to complete and then went back to firefox. Without restarting firefox I went back after more then 5 min and the folder was still not displayed. I can check more tomorrow when I'll be back at the office.
I started tried reproducing this, and I definitely didn't need to restart to see the change, I just had to reload about:home, which I need by navigating to a page, then opening a new tab.

However, I will still look into adding an observer.
(In reply to :Margaret Leibovic from comment #3)
> I started tried reproducing this, and I definitely didn't need to restart to
> see the change, I just had to reload about:home, which I need by navigating
> to a page, then opening a new tab.

Oh, nevermind, the thubnails section updated, but not the list down below.
Thinking about this more, we shouldn't need to use a content observer to update the content of a cursor adapter, that happens automatically. For example, when the user deletes a bookmark, the list updates itself automatically without us doing anything.

I found that what's going on here is that we cache the return value of desktopBookmarksExist() in LocalBrowserDB:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#427

When I commented out those two lines, I found that the bookmarks list updated as expected after syncing, without even needing to reload the page.

I don't think we want to stop caching this value, especially since this method is called every time we query for the list of bookmarks, but maybe there's a way we can invalidate this value after a sync occurs.

rnewman, do you have any ideas about how we could do that?
Flags: needinfo?(rnewman)
Well, look here, we do have some code that invalidates this in Awesomebar.java:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AwesomeBar.java#463

We probably need to find a place to move that for the new about:home.
Flags: needinfo?(rnewman)
Yeah, that's not strictly "invalidate after sync", it's "invalidate when the activity changes", which will suffice right now but will rapidly diverge from correctness as we do more frequent syncs.

One approach would be to implement Bug 809214 and then watch that (with a content observer). There are others.
Attached patch patchSplinter Review
See bug 727449 for the history behind this.

I tried adding this line in a functionally similar place to where it currently exists on m-c, but there may be a better place for it. I was debating whether or not to put this in HomePager.show, since this method also invalidates mReadingListItemsExist, which would affect the reading list page as well, but I like that HomePager doesn't know about BrowserDB right now. I'm open to suggestions if you know of a better place to put this.
Attachment #793177 - Flags: review?(lucasr.at.mozilla)
tracking-fennec: --- → ?
Hardware: ARM → All
Comment on attachment 793177 [details] [diff] [review]
patch

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

Nice catch.

::: mobile/android/base/home/BookmarksPage.java
@@ +148,5 @@
>              }
>          });
>          mList.setAdapter(mListAdapter);
>  
> +        // Invlidate the cached value that keeps track of whether or

Invalidate
Attachment #793177 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/5393a1a4e3ef
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.