bugs 891953, bug 895866, bug 895867, bug 904689 implemented empty screen states for each tab. Currently, there is none for bookmarks. For consistency should this be implemented? See screenshot. Ian?

Yeah, on the off chance that someone deletes all their bookmarks, we should show something. Mockup: http://cl.ly/image/2u2l3M191d27 Star assets: http://cl.ly/1e2j151u2j0H

CC'ed Chenxia

Sounds good - this makes me think we should also remove the empty pages for "Most Visited" now that it has the grid view and will never be completely empty.

Yeah, that's a good point too.

(In reply to Chenxia Liu [:liuche] from comment #3) > Sounds good - this makes me think we should also remove the empty pages for > "Most Visited" now that it has the grid view and will never be completely > empty. Oh does that invalidate a bug I filed over the weekend? bug 919227

Comment on attachment 809454 [details] [diff] [review] Bookmarks empty screen v1 This patch uses the same layout as the history empty screens, but because it's right next to the reading list screen, the spacing being a little off is very obvious. Based on the comments from bug 907852, it seems like I should use a Relative/FrameLayout for the Reading List empty screen, so that the spacers used to align the big empty icons will work the same way. The problem is with small screens/landscape - any suggestions on what to do with that, so that the reading list tip doesn't overlap the text + icon?

Comment on attachment 809453 [details] [diff] [review] Part 1: Remove empty screens for Top Sites v2 Sorry, I forgot that I had filed bug 919704 for this, so moving this patch over to there.

Comment on attachment 809454 [details] [diff] [review] Bookmarks empty screen v1 Review of attachment 809454 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/BookmarksPage.java @@ +158,5 @@ > + > + final TextView emptyText = (TextView) mEmptyView.findViewById(R.id.home_empty_text); > + emptyText.setText(R.string.home_bookmarks_empty); > + > + mList.setEmptyView(mEmptyView); I guess the initialization of mEmptyView should be immaterial of cursor. So the check should be inside the outer check on cursor. if (cursor is empty) { if (emptyView is null) { // initialize empty view. } list.setEmptyView(emptyView); }

Comment on attachment 817557 [details] [diff] [review] Implement empty screen state for bookmarks. Unbitrotting. I'd like to get this landed and then address the difference in alignment in a follow-up, actually. re comment #11: We only want to set the empty view when it's null (and hasn't been set before), so setEmptyView should actually be in the same block as the empty view initialization.

Comment on attachment 817557 [details] [diff] [review] Implement empty screen state for bookmarks. Review of attachment 817557 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/BookmarksPage.java @@ +152,5 @@ > + mList.setEmptyView(mEmptyView); > + } > + } > + > + Remove one of the two lines. The choice is yours ;)

26?

This has new strings, so we can't uplift to Aurora.

Comment on attachment 819276 [details] [diff] [review] Remove empty screen for Top Sites. Wrong bug.