See specs. Maybe needs asset.

Update https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=35321&edit=1 when this bug is resolved.

Soft blocker nom? A bookmark indicator would certainly make the awesomescreen a little more useful, and consistent with the desktop awesomebar (and xul fennec). I can provide assets / designs right away if we mark this as a blocker

FYI: I have a patch ready to submit. Just need assets and specs from Ian.

(In reply to Lucas Rocha (:lucasr) from comment #4) > FYI: I have a patch ready to submit. Just need assets and specs from Ian. If you don't get assets soon, would you mind posting a WIP? I imagine you're adding something to the query results to let us know if an item is a bookmark or not, and similar logic is also needed for bug 736272, so I can probably work on top of your patch here.

As an aside, also included here is how we want to treat open local and remote tabs in the awesomebar results. Assets are coming right up.

Good to go!

(In reply to Ian Barlow (:ibarlow) from comment #6) > Created attachment 614632 [details] > Mockup of bookmark star location > > As an aside, also included here is how we want to treat open local and > remote tabs in the awesomebar results. > > Assets are coming right up. Thanks for assets, Ian. The design for the bookmark star has a caveat though: we don't show favicons for pages with no favicon. We used to show a default favicon but we don't do it anymore. So, if we have a bookmark without a favicon (which is not that rare) we're show the star on top of whitespace. Showing a default favicon just to show the star on top is not ideal I guess. My suggestion is to move the star to the right side of the row. Maybe somewhere on the top-right corner? Let me know how you prefer to do it.

(In reply to Margaret Leibovic [:margaret] from comment #5) > (In reply to Lucas Rocha (:lucasr) from comment #4) > > FYI: I have a patch ready to submit. Just need assets and specs from Ian. > > If you don't get assets soon, would you mind posting a WIP? I imagine you're > adding something to the query results to let us know if an item is a > bookmark or not, and similar logic is also needed for bug 736272, so I can > probably work on top of your patch here. I'm simply checking the value of bookmark_id. If it's greater than 0, the url is a bookmark. You can probably apply the same logic on bug 736272.

Lucas, moving the star to the top right is fine for now. In the longer intend to rework the look of empty favicons, but for now we can just move the star. You can give it the same top and outside margin as the favicons have.

sorry, that was meant to say "In the longer term, I intend to rework..."

Ian, here's the screenshot for UI validation.

Looks great!

Comment on attachment 616562 [details] [diff] [review] (1/2) Show star on "Top Sites" tab rows that are bookmarks This looks good. I have one concern about the bookmark id value, but that's just more about understanding why this works. >diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java >@@ -943,16 +948,24 @@ public class AwesomeBarTabs extends TabH >+ private void updateBookmarkStar(ImageView starView, Cursor cursor) { >+ int bookmarkIdIndex = cursor.getColumnIndexOrThrow(Combined.BOOKMARK_ID); >+ long id = cursor.getLong(bookmarkIdIndex); >+ >+ int visibility = (id == 0 ? View.GONE : View.VISIBLE); Following what I talked about in bug 736272 comment 7, I wonder why cursor.getLong(bookmarkIdIndex) is returning 0 for non-bookmarks. It seems this is probably just what the getLong implementation does for null values. The docs say this behavior is "implementation-defined": http://developer.android.com/reference/android/database/Cursor.html#getLong%28int%29. Is it possible for us to get back different Cursor implementations here? We should just make sure we are always going to get 0 like we're expecting. There will never be an entry with bookmark_id = 0 because that is our fixed bookmarks root, so this check shouldn't cause problems, but I think you should add a comment about why we're checking for 0.

Comment on attachment 616563 [details] [diff] [review] (2/2) Show star on "History" tab rows that are bookmarks This also looks good, with the same comment I had about the last patch. OOC, do we want to consider showing stars in the bookmarks tab as well? I know it's not that useful because they will all have stars, but it will be consistent with the other tabs. >diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java >@@ -170,16 +171,20 @@ public class AwesomeBarTabs extends TabH >+ Long bookmarkId = (Long) historyItem.get(Combined.BOOKMARK_ID); >+ int visibility = (bookmarkId == 0 ? View.GONE : View.VISIBLE); >+ viewHolder.starView.setVisibility(visibility); My last comment also applies here.

Comment on attachment 616562 [details] [diff] [review] (1/2) Show star on "Top Sites" tab rows that are bookmarks [Approval Request Comment] Soft blocker, fairly low risk. Nice UX improvement (you can see which entries from awesomebar are bookmarks).

Comment on attachment 616562 [details] [diff] [review] (1/2) Show star on "Top Sites" tab rows that are bookmarks Let me land this m-c before requesting aurora approval. Sorry for the spam.

(In reply to Margaret Leibovic [:margaret] from comment #18) > Comment on attachment 616563 [details] [diff] [review] > (2/2) Show star on "History" tab rows that are bookmarks > > This also looks good, with the same comment I had about the last patch. OOC, > do we want to consider showing stars in the bookmarks tab as well? I know > it's not that useful because they will all have stars, but it will be > consistent with the other tabs. Good point. I'll check with Ian.

I think we can hide them in the Bookmarks tab. The stars are useful to differentiate bookmarked content from normal content, but it just adds visual clutter if everything has a star on it :)

(In reply to Margaret Leibovic [:margaret] from comment #17) > Comment on attachment 616562 [details] [diff] [review] > (1/2) Show star on "Top Sites" tab rows that are bookmarks > > This looks good. I have one concern about the bookmark id value, but that's > just more about understanding why this works. > > >diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java > >@@ -943,16 +948,24 @@ public class AwesomeBarTabs extends TabH > > >+ private void updateBookmarkStar(ImageView starView, Cursor cursor) { > >+ int bookmarkIdIndex = cursor.getColumnIndexOrThrow(Combined.BOOKMARK_ID); > >+ long id = cursor.getLong(bookmarkIdIndex); > >+ > >+ int visibility = (id == 0 ? View.GONE : View.VISIBLE); > > Following what I talked about in bug 736272 comment 7, I wonder why > cursor.getLong(bookmarkIdIndex) is returning 0 for non-bookmarks. It seems > this is probably just what the getLong implementation does for null values. > The docs say this behavior is "implementation-defined": > http://developer.android.com/reference/android/database/Cursor. > html#getLong%28int%29. Is it possible for us to get back different Cursor > implementations here? We should just make sure we are always going to get 0 > like we're expecting. Yeah, I actually expected it to throw if the value is null on database (this is what the docs say). But it's actually returning 0. > There will never be an entry with bookmark_id = 0 because that is our fixed > bookmarks root, so this check shouldn't cause problems, but I think you > should add a comment about why we're checking for 0. Ok, added a comment.

Pushed: http://hg.mozilla.org/integration/mozilla-inbound/rev/ecb25f038e12 http://hg.mozilla.org/integration/mozilla-inbound/rev/e36768ec309b

Comment on attachment 616562 [details] [diff] [review] (1/2) Show star on "Top Sites" tab rows that are bookmarks Mobile-only, soft blocker. Important UX improvement: makes it visually clear which entries in AwesomeBar are bookmarks. Low risk patch.

Comment on attachment 616563 [details] [diff] [review] (2/2) Show star on "History" tab rows that are bookmarks Mobile-only, soft blocker. Important UX improvement: makes it visually clear which entries in AwesomeBar are bookmarks. Low risk patch.

Comment on attachment 616562 [details] [diff] [review] (1/2) Show star on "Top Sites" tab rows that are bookmarks Given that this feature will still involved some follow up fixes (bug 748583 and bug 748765), I'd like to cancel the aurora request. This is not beta or 1.0 material.

Comment on attachment 616563 [details] [diff] [review] (2/2) Show star on "History" tab rows that are bookmarks Given that this feature will still involved some follow up fixes (bug 748583 and bug 748765), I'd like to cancel the aurora request. This is not beta or 1.0 material.

Stars are displayed both in Top Sites and History for visited bookmarked pages as it's expected. Closing bug as verified fixed on: Firefox 15.0a1 (2012-05-28) Device: Galaxy Nexus OS: Android 4.0.2

A new test case was created to cover the feature in the BFTs run in the Bookmarks testsuite: https://moztrap.mozilla.org/manage/cases/_detail/6346/