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

Status: RESOLVED FIXED

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

Description

[Adrian Tamas (:AdrianT)]

Attachment: 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

Comment 1

[:Margaret Leibovic]

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

Comment 2

[Adrian Tamas (:AdrianT)]

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.

Comment 3

[:Margaret Leibovic]

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.

Comment 4

[:Margaret Leibovic]

(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.

Comment 5

[:Margaret Leibovic]

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?

Comment 6

[:Margaret Leibovic]

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.

Comment 7

[Richard Newman [: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.

Comment 8

[:Margaret Leibovic]

Attachment: patch

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.

Comment 9

[Lucas Rocha (:lucasr)]

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

Comment 10

[:Margaret Leibovic]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5393a1a4e3ef