Last Comment Bug 727449 - Show only mobile bookmarks folder contents (no folder node) if there are no desktop bookmarks
: Show only mobile bookmarks folder contents (no folder node) if there are no d...
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: P2 normal (vote)
: Firefox 14
Assigned To: :Margaret Leibovic
:
Mentors:
Depends on: 737083 731267
Blocks: 722020 737203 737896
  Show dependency treegraph
 
Reported: 2012-02-15 07:37 PST by :Margaret Leibovic
Modified: 2012-06-06 00:28 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
+


Attachments
patch (4.15 KB, patch)
2012-03-14 17:15 PDT, :Margaret Leibovic
lucasr.at.mozilla: review-
Details | Diff | Splinter Review
patch v2 (9.16 KB, patch)
2012-03-15 13:13 PDT, :Margaret Leibovic
lucasr.at.mozilla: review-
Details | Diff | Splinter Review
patch v3 (9.61 KB, patch)
2012-03-16 13:44 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
patch v3 (9.60 KB, patch)
2012-03-16 14:35 PDT, :Margaret Leibovic
lucasr.at.mozilla: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
test fixes (3.87 KB, patch)
2012-03-19 10:31 PDT, :Margaret Leibovic
gbrown: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description :Margaret Leibovic 2012-02-15 07:37:42 PST
I'm splitting this off from bug 722020. See comments 23-25 in that bug for some context.

The idea is that if a user hasn't set up sync or migrated bookmarks, there won't be any bookmarks in the menu/toolbar/etc. folders, so we should just show the content of the mobile bookmarks folder in the main bookmarks list.
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2012-02-22 23:51:13 PST
needed for a betablocker
Comment 2 Wesley Johnston (:wesj) 2012-03-13 16:02:24 PDT
I think we should just show things in "Mobile bookmarks" all the time, and not show the "Desktop/Unfiled Menu/Toolbar" folder if its empty. This just feels like extra steps to get to what you want to me.
Comment 3 Richard Newman [:rnewman] 2012-03-13 16:22:49 PDT
(In reply to Wesley Johnston (:wesj) from comment #2)
> I think we should just show things in "Mobile bookmarks" all the time, and
> not show the "Desktop/Unfiled Menu/Toolbar" folder if its empty. This just
> feels like extra steps to get to what you want to me.

16:21:10 < rnewman> the alternative is that users with more than a handful of mobile bookmarks will have their other bookmarks below the fold
16:21:16 < rnewman> a tap is cheaper than a scroll
16:21:41 < rnewman> what we *should* do is remember your position in the bookmark folder stack
16:21:52 < rnewman> like your iPod remembers which album/artist/genre you were viewing
16:22:22 < rnewman> so you open Mobile Bookmarks *once*, but at least you get the visual affordance of seeing your desktop bookmarks
Comment 4 Richard Newman [:rnewman] 2012-03-13 16:26:34 PDT
One can easily imagine two classes of users:

* I don't look up mobile bookmarks; if I hit the Star, it's to get a page to show up on the home screen. If I access the bookmarks menu at all, it's to get to my *desktop* bookmarks.

* I don't care about my desktop bookmarks; I only care about mobile.

The solution for the second class of user, IMO, is "don't choose to sync them", or "have Fennec remember that you're browsing Mobile Bookmarks, and never hit 'back'". Dumping mobile bookmarks into the list without offering some way of organizing them just makes the first user unhappy, because now he has to scroll every time he views his bookmarks.
Comment 5 Madhava Enros [:madhava] 2012-03-14 14:03:52 PDT
Hi guys -

The design we had in XUL, which I think I described in the original bug on this topic was;

WHEN NO SYNC
- you see a list of your mobile bookmarks

WHEN SYNC
- in first position, a folder called "Desktop Bookmarks"
   - it contains the three top-level desktop folders (menu, toolbar, unsorted)
- after that, we have a list of your mobile bookmarks - NOT in a "Mobile Bookmarks" folder

What this means is that you are inconveniencing people who want their mobile bookmarks by only one list position (no additional tap, etc.). Desktop bookmarks are available to browse, but not over-emphasized.
Comment 6 Madhava Enros [:madhava] 2012-03-14 14:05:46 PDT
Yes - it was in bug 716918.
Comment 7 :Margaret Leibovic 2012-03-14 14:18:54 PDT
In bug 722020 we also discussed that it's not quite as simple as sync/no sync, since a user could have bookmarks in those other folders from a XUL profile migration, or from setting up sync at one point in the past then disabling it. However, we could probably just peek in those folders to see if there's anything in them, then use that to decide whether or not to show them. Just checking for any bookmark != mobile folder or special root folder, with parent != mobile folder, should suffice.
Comment 8 :Margaret Leibovic 2012-03-14 14:30:55 PDT
I think the fix for bug 731267 will involve explicitly deciding which folders we want to show in the root view, which should make it relatively easy to add some logic to use the mobile folder as our root view in the case where there are no desktop bookmarks.
Comment 9 Richard Newman [:rnewman] 2012-03-14 14:40:16 PDT
(In reply to Margaret Leibovic [:margaret] from comment #8)
> I think the fix for bug 731267 will involve explicitly deciding which
> folders we want to show in the root view, which should make it relatively
> easy to add some logic to use the mobile folder as our root view in the case
> where there are no desktop bookmarks.

Yup; it's a shift from "recursively display all children" to a more elective construction of a visual hierarchy -- both displaying only mobile bookmarks and displaying the various desktop sub-roots under a virtual folder.
Comment 10 :Margaret Leibovic 2012-03-14 17:15:03 PDT
Created attachment 606031 [details] [diff] [review]
patch

I'm concerned that this logic might belong in AwesomeBarTabs, rather than LocalBrowserDB, but I like that this approach avoids augmenting the BrowserDB API. If we do want to move this logic to AwesomeBarTabs, we might want to consider moving the logic in bug 731267 as well.

I'm also not sure if I should cache the result of desktopBookmarksExist. I definitely don't think I want to cache it if it's false, since the user might set up sync at any time, but perhaps it could be worth caching if it's true. In any case, I limited the query to try to make it as fast as possible.

In any case, this was the easy way to make a first pass at this bug, so I'll let you decide if I should change it :)
Comment 11 Richard Newman [:rnewman] 2012-03-14 17:58:49 PDT
(In reply to Margaret Leibovic [:margaret] from comment #10)

> I'm also not sure if I should cache the result of desktopBookmarksExist. I
> definitely don't think I want to cache it if it's false, since the user
> might set up sync at any time, but perhaps it could be worth caching if it's
> true. In any case, I limited the query to try to make it as fast as possible.

Even if true, it could be invalidated by a wipe from Sync. (E.g., if another Fennec device chooses Reset Sync > Replace other devices with my data.)

Your best bet is probably to cache, and periodically invalidate using some cheap SQLite mechanism, such as looking at the highest rowid in the sqlite_sequence table, or a limited query against an indexed column without a join.

Or even just invalidate asynchronously in the activity's onResume or onStart handler... it doesn't really matter if you're a few seconds behind.
Comment 12 Lucas Rocha (:lucasr) 2012-03-15 03:34:53 PDT
(In reply to Richard Newman [:rnewman] from comment #11)
> (In reply to Margaret Leibovic [:margaret] from comment #10)
> Or even just invalidate asynchronously in the activity's onResume or onStart
> handler... it doesn't really matter if you're a few seconds behind.

I tend to prefer this route. The important thing is that you don't do query every time you fetch the children of a bookmark folder. Doing it just once when you start awesomebar screen seems like a good compromise.
Comment 13 Lucas Rocha (:lucasr) 2012-03-15 03:56:41 PDT
Comment on attachment 606031 [details] [diff] [review]
patch

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

::: mobile/android/base/db/LocalBrowserDB.java
@@ +375,5 @@
>      }
>  
> +    // Returns true if any desktop bookmarks exist, which will be true if the user
> +    // has set up sync at one point, or done a profile migration from XUL fennec.
> +    private boolean desktopBookmarksExist(ContentResolver cr) {

This needs some form of caching. I recommend following rnewman's suggestion to invalidate the cached value on activity's onStart/onResume.

@@ +376,5 @@
>  
> +    // Returns true if any desktop bookmarks exist, which will be true if the user
> +    // has set up sync at one point, or done a profile migration from XUL fennec.
> +    private boolean desktopBookmarksExist(ContentResolver cr) {
> +        Cursor cursor = cr.query(bookmarksUriWithLimit(1),

I know you're just following what you've in the rest of this file but: you better enclose the cursor usage in a try block and closing the cursor in a finally block with a null check. Something like:

Cursor c = null;
int count = 0;
try {
    c = cr.query(...);
    count = c.getCount();
} finally {
    if (c != null)
        c.close();
}

Extra points if you write a follow-up patch that changes LocalBrowserDB to use same idiom everywhere :-)

@@ +378,5 @@
> +    // has set up sync at one point, or done a profile migration from XUL fennec.
> +    private boolean desktopBookmarksExist(ContentResolver cr) {
> +        Cursor cursor = cr.query(bookmarksUriWithLimit(1),
> +                                 new String[] { Bookmarks._ID },
> +                                 Bookmarks.PARENT + " <> ? AND " +

nit: any reason for not using "!="? The "<>" is less popular :-)
Comment 14 :Margaret Leibovic 2012-03-15 11:47:02 PDT
(In reply to Lucas Rocha (:lucasr) from comment #13)

> @@ +378,5 @@
> > +    // has set up sync at one point, or done a profile migration from XUL fennec.
> > +    private boolean desktopBookmarksExist(ContentResolver cr) {
> > +        Cursor cursor = cr.query(bookmarksUriWithLimit(1),
> > +                                 new String[] { Bookmarks._ID },
> > +                                 Bookmarks.PARENT + " <> ? AND " +
> 
> nit: any reason for not using "!="? The "<>" is less popular :-)

That's what I've used in the past, probably just from following prior examples. A little internet research led me to find that "<>" is part of the ANSI standard, while "!=" is not. However, they're both supported in sqlite, so I guess we don't really need to worry too much about it, unless we switch to some obscure non-sqlite db backend :)
Comment 15 :Margaret Leibovic 2012-03-15 13:13:25 PDT
Created attachment 606336 [details] [diff] [review]
patch v2

This patch adds caching. I found I had to add a new method to the BrowserDB API in order to invalidate the cached value from the AweseomeBar activity's onResume. Do you know of a better way to do this? I also feel like the method name I chose is kind of clunky, so I'm open to suggestions to improve that :)
Comment 16 :Margaret Leibovic 2012-03-15 13:15:08 PDT
Oh, I also got rid mTagsFolderId, which I forgot to remove in bug 731267. I could split that out into another patch if you want, but it's a 2-line change.
Comment 17 Richard Newman [:rnewman] 2012-03-16 00:52:55 PDT
(In reply to Margaret Leibovic [:margaret] from comment #14)

> That's what I've used in the past, probably just from following prior
> examples.

It's also what I've tended to use in the past, and what we use both in Sync and in Fennec's Profile Migrator.


> This patch adds caching. I found I had to add a new method to the BrowserDB
> API in order to invalidate the cached value from the AweseomeBar activity's
> onResume. Do you know of a better way to do this?

Not without building a hooks mechanism :/

> I also feel like the method name I chose is kind of clunky, so I'm open to suggestions to improve
> that :)

onAwesomeBarRedisplay? Something like that?

Presumably there's going to be more caching etc., so having this kind of informal peer-to-peer lifecycle observation pattern might make sense.


(In reply to Lucas Rocha (:lucasr) from comment #13)

> Cursor c = null;
> int count = 0;
> try {
>     c = cr.query(...);
>     count = c.getCount();
> } finally {
>     if (c != null)
>         c.close();
> }

Early-return, no count var, no != null style:

final Cursor c = cr.query();
if (c == null)
  return 0;
try {
  return c.getCount();
} finally {
  c.close();
}
Comment 18 Lucas Rocha (:lucasr) 2012-03-16 03:15:25 PDT
(In reply to Richard Newman [:rnewman] from comment #17)
> (In reply to Margaret Leibovic [:margaret] from comment #14)
> Early-return, no count var, no != null style:
> 
> final Cursor c = cr.query();
> if (c == null)
>   return 0;
> try {
>   return c.getCount();
> } finally {
>   c.close();
> }

I disagree on this null check on Cursor. If query ever returns null, this would be a bug needing investigation. We should not silently bail.
Comment 19 Lucas Rocha (:lucasr) 2012-03-16 04:11:44 PDT
Comment on attachment 606336 [details] [diff] [review]
patch v2

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

Looking good. Just giving r- to see a patch with the new method name in place.

::: mobile/android/base/db/BrowserDB.java
@@ +78,5 @@
>          public void clearHistory(ContentResolver cr);
>  
>          public Cursor getBookmarksInFolder(ContentResolver cr, long folderId);
>  
> +        public void invalidateDesktopBookmarksExist();

I agree with rnewman that a more general cache/life-cycle API for BrowserDB would be make more sense. I don't like the direct association with AwesomeBar though.

So, maybe something more generic like invalidateCachedState() (or something similar) should do it here. If we ever need more granularity on what cached data to invalidate, we can add an EnumSet argument later.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +393,5 @@
> +                c.close();
> +        }
> +
> +        // Cache result for future queries
> +        mDesktopBookmarksExist = count == 1;

nit: I tend to add ()'s around those expressions for better readability.
Comment 20 :Margaret Leibovic 2012-03-16 09:15:42 PDT
(In reply to Lucas Rocha (:lucasr) from comment #18)
> (In reply to Richard Newman [:rnewman] from comment #17)
> > (In reply to Margaret Leibovic [:margaret] from comment #14)
> > Early-return, no count var, no != null style:
> > 
> > final Cursor c = cr.query();
> > if (c == null)
> >   return 0;
> > try {
> >   return c.getCount();
> > } finally {
> >   c.close();
> > }
> 
> I disagree on this null check on Cursor. If query ever returns null, this
> would be a bug needing investigation. We should not silently bail.

So no null check at all? I added the null check initially because it was included in your snippet in comment 13.
Comment 21 Richard Newman [:rnewman] 2012-03-16 10:07:36 PDT
> > I disagree on this null check on Cursor. If query ever returns null, this
> > would be a bug needing investigation. We should not silently bail.

Then:

  if (c == null) {
    throw something.
  }

In Sync we have a QueryHelper that automatically throws checked NullCursorExceptions, so you can't ignore them.

My main point is that a null check in `finally` is unnecessary; you know up-front if your cursor is null.
Comment 22 :Margaret Leibovic 2012-03-16 13:44:16 PDT
Created attachment 606718 [details] [diff] [review]
patch v3

Updated the method name to invalidateCachedState(). I got rid of the null check all together, since we're not expecting the cursor to ever be null. Let me know if we'd prefer to do something more complicated like throw a different exception as rnewman mentioned. However, this would already throw a regular NPE if there was a problem, so throwing a different exception wouldn't do much unless we're adding code elsewhere to handle that case.
Comment 23 :Margaret Leibovic 2012-03-16 14:35:23 PDT
Created attachment 606740 [details] [diff] [review]
patch v3

Oops, missed one variable rename.
Comment 24 Lucas Rocha (:lucasr) 2012-03-19 04:04:53 PDT
Comment on attachment 606740 [details] [diff] [review]
patch v3

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

Good.
Comment 25 :Margaret Leibovic 2012-03-19 10:03:29 PDT
Gr, I just realized this is going to make testBookmark fail, since that expects folders to show up... I can update the tests for the no-desktop-bookmarks case, but I suppose it would be good to also test the UI when there are desktop bookmarks. I don't really want to block on that to land this bug, so I think I'll just update testBookmark to work with this new root folder view.
Comment 26 :Margaret Leibovic 2012-03-19 10:31:37 PDT
Created attachment 607209 [details] [diff] [review]
test fixes

I just updated testBookmark to not expect any folders in the root view (this is basically how it was before). I filed bug 737083 about adding tests to test the folder view (it's more complicated now, since we'll need to add bookmarks with explicit folder parents directly to the db).
Comment 29 :Margaret Leibovic 2012-03-22 10:21:56 PDT
Comment on attachment 606740 [details] [diff] [review]
patch v3

[Approval Request Comment]
Mobile only. This patch makes progress towards completing the bookmarks UI design.
Comment 30 :Margaret Leibovic 2012-03-22 10:22:44 PDT
Comment on attachment 607209 [details] [diff] [review]
test fixes

[Approval Request Comment]
The test changes need to go along with the patch.
Comment 31 Alex Keybl [:akeybl] 2012-03-22 13:40:34 PDT
Comment on attachment 606740 [details] [diff] [review]
patch v3

[Triage Comment]
Mobile only - approved for Aurora 13.
Comment 33 Paul Feher 2012-06-06 00:28:10 PDT
Verified/Fixed on:
Nightly Fennec 16.0a1 (2012-06-05)
Aurora Fennec 14.0a2 (2012-06-04)
Device: HTC Desire Z
OS: Android 2.3.3

Note You need to log in before you can comment on or make changes to this bug.