862796 - About:home "bookmarks" panel

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

This is the bug to track work on the new "bookmarks" panel on about:home. 

The biggest change from our current bookmarks list will be that the "top sites" thumbnail grid will be moved to this section and be filled with the user's most frequently visited bookmarks instead. 

Mockups to follow shortly.

Comment 1

[Ian Barlow (:ibarlow)]

Attachment: Mockups of "Bookmarks" panel

Comment 2

[Ian Barlow (:ibarlow)]

Work in this bug should include

* Generating thumbnails by looking at the user's most visited bookmarks
* Restyling bookmarks list elements

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP

This is a very basic WIP for moving the BookmarksTab from awesomebar to a BookmarksPage in about:home.

Basically, BookmarksPage is a fragment (thanks to :bnicholson). The fragment exposes a ListView (the list of bookmarks currently). This would later change to show the top-sites too. The adapters are retained from the Awesomebar.

I'm using a custom view, PageTwoLineRow to show the rows in the list. This is a reusable view (hence in "widget" package), and could be used with the history, tabs from last time, addons pages too. The dependencies of "AwesomebarViewHolder" are removed, and the custom view, PageTwoLineRow, takes care of updating the text and bitmaps from a cursor.

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This is a (near) complete patch. Everything works as expected. I have to check the alignment of "bookmark star" icon used in history tab, as PageTwoLineRow will be used by all two-line-rows in about:home.

List of ToDos:
1. Switch to Tab should be fixed.
2. ContextMenu code from AwesomeBar should be copied to BrowserApp.
3. May be separate the ListView to a BookmarksListView, as BookmarksPage would have a TopSitesView too.

These could be followups.

The more the feedback, the better the cleanup :)
Meanwhile I'll check with Ian and update the layout of page_two_line_row.xml tomorrow.

Comment 5

[:Margaret Leibovic]

Comment on attachment 751924 [details] [diff] [review]
Patch

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

Nice, this is looking really good. I really like that you've been adding comments to document existing code. With a few tweaks, I think this would be fine to land on fig soon.

::: mobile/android/base/AwesomeBar.java
@@ -252,5 @@
> -        if (showReadingList) {
> -            BookmarksTab bookmarksTab = mAwesomeTabs.getBookmarksTab();
> -            bookmarksTab.setShowReadingList(true);
> -            mAwesomeTabs.setCurrentItemByTag(bookmarksTab.getTag());
> -        }

The code in AwesomeBar and AwesomeBarTabs shouldn't even be used anymore on fig... so I don't think we need to worry about making changes in here.

I was going to suggest we rip all the unused AwesomeBar stuff out in a separate bug, but I actually think it's nice that we can see the diff between BookmarksTab and BookmarksPage in this patch, so maybe we should leave that stuff around until we've scavenged it for as many parts as we're going to use.

::: mobile/android/base/home/BookmarksPage.java
@@ +45,5 @@
> + */
> +public class BookmarksPage extends Fragment {
> +    public static final String LOGTAG = "GeckoBookmarksPage";
> +
> +    private static final String EMPTY_STRING = "";

I feel like it's not really necessary to make this into a constant.

@@ +65,4 @@
>  
>      @Override
> +    public void onCreate(Bundle savedInstanceState) {
> +        super.onCreate(savedInstanceState);

Do you need to override this method if the only thing you're doing in it is calling its super implementation?

@@ +190,5 @@
> +
> +    /**
> +     * Internal class to handle different event listeners on the ListView.
> +     */
> +    private class Callbacks implements AdapterView.OnItemClickListener,

Cool, I think it's nice to make a class like this to handle events. However, I think a name like EventHandler would be better for this class, since Callbacks sounds more like a data structure.

@@ +209,5 @@
> +                return;
> +            }
> +
> +            Cursor cursor = mCursorAdapter.getCursor();
> +            if (cursor == null)

When would cursor be null here? I want to make sure we're not silently eating an error.

@@ +240,5 @@
> +                // take focus away from awesome bar to hide the keyboard
> +                view.requestFocus();
> +            }
> +            return false;
> +        }

Hm, it seems like this could be factored out to solve bug 871642. Out of scope for this bug, but something to remember for that bug :)

@@ +249,5 @@
> +            AdapterView.AdapterContextMenuInfo info = (AdapterView.AdapterContextMenuInfo) menuInfo;
> +            Object selectedItem = list.getItemAtPosition(info.position);
> +
> +            if (!(selectedItem instanceof Cursor)) {
> +                Log.e(LOGTAG, "item at " + info.position + " is not a Cursor");

When would this happen? When tapping on a header row? I don't know if it's worth logging an error in that case.

@@ +285,5 @@
> +
> +            // Hide "Remove" item if there isn't a valid history ID
> +            if (id < 0) {
> +                menu.findItem(R.id.remove_history).setVisible(false);
> +            }

Nit: It would be nice to be consistent about either always using or never using braces for one-line ifs. I think in general we've been using braces in Java, so maybe we should continue doing that.

@@ +347,5 @@
>              return true;
>          }
>  
> +        /**
> +         * Moves to parent folder, if one exists.

Looks like a copy/paste error on this comment.

@@ -397,3 @@
>                  }
>              });
> -            mQueryTask = null;

Why did you move this out of here and into refreshListWithCursor? I think it makes more sense to keep it associated with the AsyncTask itself.

::: mobile/android/base/home/HomePager.java
@@ +51,5 @@
>          mLoaded = true;
>          TabsAdapter adapter = new TabsAdapter(fm);
>  
>          // Add the pages to the adapter in order.
> +        adapter.addTab(Page.BOOKMARKS, BookmarksPage.class, null, getContext().getString(R.string.awesomebar_bookmarks_title));

I really like how simple it is to add a page here :)

::: mobile/android/base/widget/BookmarkFolderView.java
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko.widget;
> +
> +import org.mozilla.gecko.Favicons;

There are some unused imports in here.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Oooooh! Thanks for your comments. You caught many things :)

(In reply to :Margaret Leibovic from comment #5)
> Comment on attachment 751924 [details] [diff] [review]
> Patch
> 
> Review of attachment 751924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice, this is looking really good. I really like that you've been adding
> comments to document existing code. With a few tweaks, I think this would be
> fine to land on fig soon.
> 
> ::: mobile/android/base/AwesomeBar.java
> @@ -252,5 @@
> > -        if (showReadingList) {
> > -            BookmarksTab bookmarksTab = mAwesomeTabs.getBookmarksTab();
> > -            bookmarksTab.setShowReadingList(true);
> > -            mAwesomeTabs.setCurrentItemByTag(bookmarksTab.getTag());
> > -        }
> 
> The code in AwesomeBar and AwesomeBarTabs shouldn't even be used anymore on
> fig... so I don't think we need to worry about making changes in here.
> 
> I was going to suggest we rip all the unused AwesomeBar stuff out in a
> separate bug, but I actually think it's nice that we can see the diff
> between BookmarksTab and BookmarksPage in this patch, so maybe we should
> leave that stuff around until we've scavenged it for as many parts as we're
> going to use.

Agreed. But we don't have BookmarksTab anymore, and still we are adding AwesomeBar for compilation. So I wanted to hide the java errors, hence remove this chunk.

> 
> ::: mobile/android/base/home/BookmarksPage.java
> @@ +45,5 @@
> > + */
> > +public class BookmarksPage extends Fragment {
> > +    public static final String LOGTAG = "GeckoBookmarksPage";
> > +
> > +    private static final String EMPTY_STRING = "";
> 
> I feel like it's not really necessary to make this into a constant.
> 

It's highly recommended to do this in java. More like this: https://hg.mozilla.org/mozilla-central/file/tip/mobile/android/base/RemoteTabs.java#l31 (rnewman likes it :) ).

> @@ +65,4 @@
> >  
> >      @Override
> > +    public void onCreate(Bundle savedInstanceState) {
> > +        super.onCreate(savedInstanceState);
> 
> Do you need to override this method if the only thing you're doing in it is
> calling its super implementation?

Aah. I copied code from brian's and didn't check it in final go. :D
Will remove it.

> 
> @@ +190,5 @@
> > +
> > +    /**
> > +     * Internal class to handle different event listeners on the ListView.
> > +     */
> > +    private class Callbacks implements AdapterView.OnItemClickListener,
> 
> Cool, I think it's nice to make a class like this to handle events. However,
> I think a name like EventHandler would be better for this class, since
> Callbacks sounds more like a data structure.

I was going by Android's terminology. They tend to use Callbacks for such classes. I can go with EventHandler too, if that's fine with others.

> 
> @@ +209,5 @@
> > +                return;
> > +            }
> > +
> > +            Cursor cursor = mCursorAdapter.getCursor();
> > +            if (cursor == null)
> 
> When would cursor be null here? I want to make sure we're not silently
> eating an error.

When list view is first created, we initialize the adapter with a null cursor. May be we are checking that here. (I copy pasted it, and didn't want to regress). Anyone else, any thoughts?

> 
> @@ +240,5 @@
> > +                // take focus away from awesome bar to hide the keyboard
> > +                view.requestFocus();
> > +            }
> > +            return false;
> > +        }
> 
> Hm, it seems like this could be factored out to solve bug 871642. Out of
> scope for this bug, but something to remember for that bug :)
> 
> @@ +249,5 @@
> > +            AdapterView.AdapterContextMenuInfo info = (AdapterView.AdapterContextMenuInfo) menuInfo;
> > +            Object selectedItem = list.getItemAtPosition(info.position);
> > +
> > +            if (!(selectedItem instanceof Cursor)) {
> > +                Log.e(LOGTAG, "item at " + info.position + " is not a Cursor");
> 
> When would this happen? When tapping on a header row? I don't know if it's
> worth logging an error in that case.

Not sure. Copy pasted and never worked on this section :)
But somehow I feel, it should never be not a cursor. (double negation). Shall we take a risk and try removing this? If something fails, we have a fix and 24 more weeks in hand ;)

> 
> @@ +285,5 @@
> > +
> > +            // Hide "Remove" item if there isn't a valid history ID
> > +            if (id < 0) {
> > +                menu.findItem(R.id.remove_history).setVisible(false);
> > +            }
> 
> Nit: It would be nice to be consistent about either always using or never
> using braces for one-line ifs. I think in general we've been using braces in
> Java, so maybe we should continue doing that.

Copy paste. :) I generally don't use braces for one-line if statements (eclipse guy). But kats and bnicholson like it. I can check and change it for the entire file.

> 
> @@ +347,5 @@
> >              return true;
> >          }
> >  
> > +        /**
> > +         * Moves to parent folder, if one exists.
> 
> Looks like a copy/paste error on this comment.

:P

> 
> @@ -397,3 @@
> >                  }
> >              });
> > -            mQueryTask = null;
> 
> Why did you move this out of here and into refreshListWithCursor? I think it
> makes more sense to keep it associated with the AsyncTask itself.

I didn't like this piece of code which read like,

mQueryTask = new BookmarkQueryTask();

and in BookmarkQueryTask we do,

{
  ...
  mQueryTask = null;
}

which doesn't feel like a class and it's instance. That's why I removed that entire chunk and moved it out, to make it feel like a callback. (Owner started some work, it completed, let the owner clean up the mess -- rather than, some work completed, and work will cleanup owner's stuff).

> 
> ::: mobile/android/base/home/HomePager.java
> @@ +51,5 @@
> >          mLoaded = true;
> >          TabsAdapter adapter = new TabsAdapter(fm);
> >  
> >          // Add the pages to the adapter in order.
> > +        adapter.addTab(Page.BOOKMARKS, BookmarksPage.class, null, getContext().getString(R.string.awesomebar_bookmarks_title));
> 
> I really like how simple it is to add a page here :)
> 
> ::: mobile/android/base/widget/BookmarkFolderView.java
> @@ +4,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +package org.mozilla.gecko.widget;
> > +
> > +import org.mozilla.gecko.Favicons;
> 
> There are some unused imports in here.

This is why I should use bnicholson's eclipse plugin! :(

Comment 7

[Mark Erikson]

If the Firefox Mobile bookmarks display code is being rewritten... can you implement a way to sort or edit the display order?  I had previously requested this as bug #762298, but there's been no followup to that in the last year.

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 751924 [details] [diff] [review]
Patch

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

Margaret has already covered this patch pretty well. I'm a little concerned about the life cycle behaviour of this fragment and its cursor in the context of the viewpager. Please, test things like: while the page is visible, leave the app then return to it to ensure the cursor is correctly restored.

::: mobile/android/base/AwesomeBar.java
@@ -252,5 @@
> -        if (showReadingList) {
> -            BookmarksTab bookmarksTab = mAwesomeTabs.getBookmarksTab();
> -            bookmarksTab.setShowReadingList(true);
> -            mAwesomeTabs.setCurrentItemByTag(bookmarksTab.getTag());
> -        }

FWIW, I agree with Margaret here.

@@ +674,5 @@
>                      private boolean mInReadingList;
>  
>                      @Override
>                      public void onPreExecute() {
> +                        //mInReadingList = mAwesomeTabs.isInReadingList();

Simply remove it?

::: mobile/android/base/db/LocalBrowserDB.java
@@ -371,5 @@
>      @Override
>      public Cursor getBookmarksInFolder(ContentResolver cr, long folderId) {
>          Cursor c = null;
>          boolean addDesktopFolder = false;
> -        boolean addReadingListFolder = false;

The changes to remove the reading list bits from LocalBrowserDB should probably go on a separate patch. This is a self-contained change that can happen before your patch here.

::: mobile/android/base/home/BookmarksPage.java
@@ +57,5 @@
> +    // The view shown by the fragment.
> +    private ListView mList;
> +
> +    // Callbacks for the events on the list view.
> +    private Callbacks mCallbacks;

What's the advantage of this over just making BookmarksPage implement the listener interfaces directly?

@@ +69,5 @@
>      }
>  
>      @Override
> +    public void onAttach(Activity activity) {
> +        super.onAttach(activity);

Maybe it would be more future-proof to do the initial cursor refresh here instead of onCreateView(). This is because the adapter might detach and attach fragments depending on the active page on the ViewPager. Generally speaking, onAttach() and onDetach() usually "revert" each other. Just keep in mind that the fragment's view is only created after onAttach.

@@ +121,5 @@
> +        mList.setOnCreateContextMenuListener(mCallbacks);
> +        mList.setOnKeyListener(GamepadUtils.getListItemClickDispatcher());
> +
> +        mQueryTask = new BookmarksQueryTask();
> +        mQueryTask.execute();

I'd really like us to start using AsyncTaskLoader instead of plain AsyncTasks. Please file a follow-up.

::: mobile/android/base/resources/values/styles.xml
@@ +70,5 @@
>      </style>
>  
> +    <style name="Widget.PageTwoLineRow" />
> +
> +    <style name="Widget.PageTwoLineRow.Title">

It's wrong to use the '.' to define a 'namescape' for the styles here. The '.' should only be used for inheritance. Let's use '_' instead (that's what Android does btw). The same should probably be done on all of our styles. Please file a follow-up.

::: mobile/android/base/widget/BookmarkFolderView.java
@@ +2,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko.widget;

I would keep this view in the abouthome package instead of widget as this is not a general purpose component. We can move it on a follow-up bug though as we don't have an abouthome package yet.

::: mobile/android/base/widget/PageTwoLineRow.java
@@ +2,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko.widget;

This is still specific to about:home pages, no? I'd just move this to an "abouthome" package. I generally prefer to start private and only move to a more general-purpose package if/when we need it.

@@ +8,5 @@
> +import org.mozilla.gecko.Favicons;
> +import org.mozilla.gecko.R;
> +import org.mozilla.gecko.db.BrowserDB.URLColumns;
> +import org.mozilla.gecko.gfx.BitmapUtils;
> +import org.mozilla.gecko.widget.FaviconView;

No need to import it I guess.

@@ +22,5 @@
> +import android.widget.ImageView;
> +import android.widget.TextView;
> +import android.widget.RelativeLayout;
> +
> +public class PageTwoLineRow extends RelativeLayout {

PageTwoLineRow sounds a bit weird :-) Maybe TwoLinePageRow?

@@ +45,5 @@
> +        mUrl = (TextView) findViewById(R.id.url);
> +        mFavicon = (FaviconView) findViewById(R.id.favicon);
> +    }
> +
> +    public void updateViewsWithCursor(Cursor cursor) {

I'd rename this to simply updateFromCursor(). Maybe you should handle the null cursor case at the top of the method?

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 751924 [details] [diff] [review]
Patch

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

::: mobile/android/base/home/BookmarksPage.java
@@ +48,5 @@
> +
> +    private static final String EMPTY_STRING = "";
> +
> +    private int mFolderId = Bookmarks.FIXED_ROOT_ID;
> +    private String mFolderTitle = EMPTY_STRING;

I would just use null here. To me, an empty string means that we have a title, but the title is empty. Null, on the other hand, means we have no title at all (which is the case here).

@@ +79,5 @@
>  
> +        // Can't use getters for adapter. It will create one if null.
> +        if (mCursorAdapter != null && mList != null) {
> +            mList.setAdapter(null);
> +            final Cursor cursor = mCursorAdapter.getCursor();

All of this manual cursor management is a mess. We should move toward using CursorLoaders, which take care of closing cursors when they're no longer being used.

@@ +391,4 @@
>          public String getFolderTitle(int position) {
>              Cursor c = getCursor();
>              if (!c.moveToPosition(position))
> +                return EMPTY_STRING;

I'd use null here for the same reason as above. You aren't returning a title that's empty; you're returning the non-existence of a title.

@@ +461,5 @@
>  
>          @Override
>          protected void onPostExecute(final Cursor cursor) {
>              // Hack: force this to the main thread, even though it should already be on it
>              ThreadUtils.postToUiThread(new Runnable() {

I know this isn't new code, but what's going on here? Why wouldn't we be on the UI thread already in onPostExecute()?

Comment 10

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #9)

> @@ +461,5 @@
> >  
> >          @Override
> >          protected void onPostExecute(final Cursor cursor) {
> >              // Hack: force this to the main thread, even though it should already be on it
> >              ThreadUtils.postToUiThread(new Runnable() {
> 
> I know this isn't new code, but what's going on here? Why wouldn't we be on
> the UI thread already in onPostExecute()?

Some history:
https://bugzilla.mozilla.org/show_bug.cgi?id=746946#c23

Maybe this issue is fixed by now? I agree we should figure out what's going on here and take this rewrite as an opportunity to fix hacks.

Comment 11

[Brian Nicholson (:bnicholson)]

(In reply to :Margaret Leibovic from comment #10)
> 
> Some history:
> https://bugzilla.mozilla.org/show_bug.cgi?id=746946#c23
> 
> Maybe this issue is fixed by now?

Oh...yeah, that was an Android bug that we fixed back in bug 750358 (the follow-up to the one you linked to). It should be safe to remove these hacks now.

Comment 12

[:Margaret Leibovic]

Given the changes in bug 871645, we should look through this patch to make sure we don't re-introduce any "awesome" variable names here.

Comment 13

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #10)
> (In reply to Brian Nicholson (:bnicholson) from comment #9)
> 
> > @@ +461,5 @@
> > >  
> > >          @Override
> > >          protected void onPostExecute(final Cursor cursor) {
> > >              // Hack: force this to the main thread, even though it should already be on it
> > >              ThreadUtils.postToUiThread(new Runnable() {
> > 
> > I know this isn't new code, but what's going on here? Why wouldn't we be on
> > the UI thread already in onPostExecute()?
> 
> Some history:
> https://bugzilla.mozilla.org/show_bug.cgi?id=746946#c23
> 
> Maybe this issue is fixed by now? I agree we should figure out what's going
> on here and take this rewrite as an opportunity to fix hacks.

I'll remove this.

Comment 14

[Sriram Ramasubramanian [:sriram]]

(In reply to Brian Nicholson (:bnicholson) from comment #9)
> Comment on attachment 751924 [details] [diff] [review]
> Patch
> 
> Review of attachment 751924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +79,5 @@
> >  
> > +        // Can't use getters for adapter. It will create one if null.
> > +        if (mCursorAdapter != null && mList != null) {
> > +            mList.setAdapter(null);
> > +            final Cursor cursor = mCursorAdapter.getCursor();
> 
> All of this manual cursor management is a mess. We should move toward using
> CursorLoaders, which take care of closing cursors when they're no longer
> being used.

CursorLoaders? :O Could this be a followup?
I want to keep this patch simple-yet-cleanup enough not to break existing stuff.

Comment 15

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #8)
> Comment on attachment 751924 [details] [diff] [review]
> Patch
> 
> Review of attachment 751924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Margaret has already covered this patch pretty well. I'm a little concerned
> about the life cycle behaviour of this fragment and its cursor in the
> context of the viewpager. Please, test things like: while the page is
> visible, leave the app then return to it to ensure the cursor is correctly
> restored.
> 
> ::: mobile/android/base/AwesomeBar.java
> @@ -252,5 @@
> > -        if (showReadingList) {
> > -            BookmarksTab bookmarksTab = mAwesomeTabs.getBookmarksTab();
> > -            bookmarksTab.setShowReadingList(true);
> > -            mAwesomeTabs.setCurrentItemByTag(bookmarksTab.getTag());
> > -        }
> 

Shall I remove AwesomeBar.java from Makefile? I am removing BookmarksTab, and renaming it to a BookmarksPage. Hence this piece of code will give compilation error if this is not removed.

> FWIW, I agree with Margaret here.
> 
> @@ +674,5 @@
> >                      private boolean mInReadingList;
> >  
> >                      @Override
> >                      public void onPreExecute() {
> > +                        //mInReadingList = mAwesomeTabs.isInReadingList();
> 
> Simply remove it?
> 

Will do it.

> ::: mobile/android/base/db/LocalBrowserDB.java
> @@ -371,5 @@
> >      @Override
> >      public Cursor getBookmarksInFolder(ContentResolver cr, long folderId) {
> >          Cursor c = null;
> >          boolean addDesktopFolder = false;
> > -        boolean addReadingListFolder = false;
> 
> The changes to remove the reading list bits from LocalBrowserDB should
> probably go on a separate patch. This is a self-contained change that can
> happen before your patch here.

I'll split it. But this is the minimal cleanup that was required to avoid compilation errors.

> 
> ::: mobile/android/base/home/BookmarksPage.java
> @@ +57,5 @@
> > +    // The view shown by the fragment.
> > +    private ListView mList;
> > +
> > +    // Callbacks for the events on the list view.
> > +    private Callbacks mCallbacks;
> 
> What's the advantage of this over just making BookmarksPage implement the
> listener interfaces directly?

Whenever the code blocks for implementation runs longer, its better to factor out to a separate inner class. This is also useful in cases when we need to re-use the event listener. Usually its good for a class to implement interfaces by itself, if it makes sense to declare it that way. e.g. TabsTray implements OnTabsChangedListener. Put in other words: If it makes sense for the "documentation" of the class to show that it implements an interface, then the class can implement it. If not, it's better to have a private class that implements the event listeners.

Examples:
AbsListView implements a bunch of interfaces: http://developer.android.com/reference/android/widget/AbsListView.html . These make sense from an architectural point of view.
However, there are internal listeners, that aren't needed to be exposed: https://github.com/android/platform_frameworks_base/blob/master/core/java/android/widget/AbsListView.java#L5994

Similary, ActivityChooserView implements ActivityChooserModelClient. This looks good architecturally. But the actual View.OnClickListener are taken care of my an internal callback:
https://github.com/android/platform_frameworks_base/blob/master/core/java/android/widget/ActivityChooserView.java#L61 and
https://github.com/android/platform_frameworks_base/blob/master/core/java/android/widget/ActivityChooserView.java#L525
Exposing those 4 listeners at the class level wouldn't look good.

In short, this is just for good readability, abstraction and re-use of code recommended in Java.

> 
> @@ +69,5 @@
> >      }
> >  
> >      @Override
> > +    public void onAttach(Activity activity) {
> > +        super.onAttach(activity);
> 
> Maybe it would be more future-proof to do the initial cursor refresh here
> instead of onCreateView(). This is because the adapter might detach and
> attach fragments depending on the active page on the ViewPager. Generally
> speaking, onAttach() and onDetach() usually "revert" each other. Just keep
> in mind that the fragment's view is only created after onAttach.

Will do it.

> 
> @@ +121,5 @@
> > +        mList.setOnCreateContextMenuListener(mCallbacks);
> > +        mList.setOnKeyListener(GamepadUtils.getListItemClickDispatcher());
> > +
> > +        mQueryTask = new BookmarksQueryTask();
> > +        mQueryTask.execute();
> 
> I'd really like us to start using AsyncTaskLoader instead of plain
> AsyncTasks. Please file a follow-up.

Sure.

> 
> ::: mobile/android/base/resources/values/styles.xml
> @@ +70,5 @@
> >      </style>
> >  
> > +    <style name="Widget.PageTwoLineRow" />
> > +
> > +    <style name="Widget.PageTwoLineRow.Title">
> 
> It's wrong to use the '.' to define a 'namescape' for the styles here. The
> '.' should only be used for inheritance. Let's use '_' instead (that's what
> Android does btw). The same should probably be done on all of our styles.
> Please file a follow-up.

Actually, Android uses "." for namespacing. It doesn't use "_". It took cue from Android for the styles file.

Examples:
https://github.com/android/platform_frameworks_base/blob/master/core/res/res/values/styles.xml#L75
https://github.com/android/platform_frameworks_base/blob/master/core/res/res/values/styles.xml#L79

https://github.com/android/platform_frameworks_base/blob/master/core/res/res/values/styles.xml#L925
https://github.com/android/platform_frameworks_base/blob/master/core/res/res/values/styles.xml#L942

I haven't seen styles using a "_" convention. This is because, "." are converted to "_" while creating a shadowed class name in the R.java. So, a "TextAppearance.Medium" will become  "private final class TextAppearance_Medium (extends TextAppearance) { .. } "

http://developer.android.com/reference/android/R.style.html#Animation_Activity

I feel a "." follows the same conventions as android.

> 
> ::: mobile/android/base/widget/BookmarkFolderView.java
> @@ +2,5 @@
> > + * This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +package org.mozilla.gecko.widget;
> 
> I would keep this view in the abouthome package instead of widget as this is
> not a general purpose component. We can move it on a follow-up bug though as
> we don't have an abouthome package yet.

This is a re-usable class independent of "home" package. This is similar to TwoWayView.java or ThumbnailView.java. I somehow don't like having this under the "home" package we have. "home" can have top level components like "BookmarksPage", "HistoryPage" and so on. But the small individual components that can be re-used should probably be in "widget" package. That's why I'm having "PageTwoLineRow" in "widget" package.

> 
> ::: mobile/android/base/widget/PageTwoLineRow.java
> @@ +2,5 @@
> > + * This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +package org.mozilla.gecko.widget;
> 
> This is still specific to about:home pages, no? I'd just move this to an
> "abouthome" package. I generally prefer to start private and only move to a
> more general-purpose package if/when we need it.
> 

Same explanation as BookmarkFolderView. But, I am fine with moving this to "home" package if that's needed.

> @@ +8,5 @@
> > +import org.mozilla.gecko.Favicons;
> > +import org.mozilla.gecko.R;
> > +import org.mozilla.gecko.db.BrowserDB.URLColumns;
> > +import org.mozilla.gecko.gfx.BitmapUtils;
> > +import org.mozilla.gecko.widget.FaviconView;
> 
> No need to import it I guess.

Will cleanup.

> 
> @@ +22,5 @@
> > +import android.widget.ImageView;
> > +import android.widget.TextView;
> > +import android.widget.RelativeLayout;
> > +
> > +public class PageTwoLineRow extends RelativeLayout {
> 
> PageTwoLineRow sounds a bit weird :-) Maybe TwoLinePageRow?

Sure. :) I wanted to specify "Page" first. I don't why. The name felt weird for me too though. :D

> 
> @@ +45,5 @@
> > +        mUrl = (TextView) findViewById(R.id.url);
> > +        mFavicon = (FaviconView) findViewById(R.id.favicon);
> > +    }
> > +
> > +    public void updateViewsWithCursor(Cursor cursor) {
> 
> I'd rename this to simply updateFromCursor(). Maybe you should handle the
> null cursor case at the top of the method?

I think I had that piece of code. However we never pass in null cursor. So, I removed it as it was an unnecessary check.

Comment 16

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 1: LocalBrowser DB cleanup

This is a small patch for cleaning up LocalBrowser DB so that it won't give reading-list items with the cursor.

Comment 17

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 2: Bookmarks page

This patch cleans up the code with the feedback comments.
Major ones:
1. BookmarksTab is left behind and a brand new BookmarksPage is created (as per discussion in IRC).
2. Renamed Callbacks to EventListeners
3. Renamed PageTwoLineRow to TwoLinePageRow.
4. Moved TwoLinePageRow and BookmarkFolderView to home/.
5. Removed the unnecessary postToUiThread().
6. Added { .. } for single line if's.
7. Removed EMPTY_STRING
8. Cleaned up two_line_page_row.xml to be a LinearLayout after discussing with Ian.

ToDo's (as followups):
1. Add ContextMenu code from AwesomeBar to BrowserApp.
2. Implement Switch-to-tab.
3. Make TwoLinePageRow capable of handling bookmark/reading-list icon and switch-to-tab icon.
4. CursorLoaders instead of Cursors.

Comment 18

[Brian Nicholson (:bnicholson)]

Comment on attachment 755604 [details] [diff] [review]
Part 1: LocalBrowser DB cleanup

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

I think there's still more cleanup to be done. What about mReadingListItemsExist and readingListItemsExist()?

Comment 19

[Sriram Ramasubramanian [:sriram]]

(In reply to Brian Nicholson (:bnicholson) from comment #18)
> Comment on attachment 755604 [details] [diff] [review]
> Part 1: LocalBrowser DB cleanup
> 
> Review of attachment 755604 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think there's still more cleanup to be done. What about
> mReadingListItemsExist and readingListItemsExist()?

I thought of removing only the parts needed to get things compiling fine. And I thought the rest were scheduled for a followup (as per Margaret) where we go and look at the file closely.
Do you want me to do a complete cleanup? I can do that. (And remove everything reading-list).

Comment 20

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 1: LocalBrowser DB cleanup

This cleans up a bit more.

Comment 21

[Brian Nicholson (:bnicholson)]

Comment on attachment 755606 [details] [diff] [review]
Part 2: Bookmarks page

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

::: mobile/android/base/home/BookmarkFolderView.java
@@ +16,5 @@
> +
> +    private boolean mIsOpen = false;
> +
> +    public BookmarkFolderView(Context context) {
> +        this(context, null);

Why wouldn't you call super(context) instead?

::: mobile/android/base/home/BookmarksPage.java
@@ +57,5 @@
> +    // Folder title for the currently shown list of bookmarks.
> +    private BookmarkFolderView mFolderView;
> +
> +    // Event handlers for the events on the list view.
> +    private EventHandlers mEventHandlers;

No need for this to be class-level since it's only used in once place.

@@ +65,5 @@
> +        super.onDetach();
> +
> +        // Can't use getters for adapter. It will create one if null.
> +        if (mCursorAdapter != null && mList != null) {
> +            mList.setAdapter(null);

onDestroyView() is executed before onDetach(), so this block will never be entered (since you set mList = null in onDetach()). Since you're setting mList to null, there's no need to worry about clearing its adapter. Just remove this line and remove "mList != null" from the if condition.

@@ +72,5 @@
> +            ThreadUtils.postToBackgroundThread(new Runnable() {
> +                @Override
> +                public void run() {
> +                    if (cursor != null && !cursor.isClosed())
> +                        cursor.close();

Since the fragment is being detached, I'd set mCursorAdapter to null right before closing its cursor. Also, onDetach() is the inverse of onAttach(), so if you're going to close the cursor here, it would make more sense to initialize the adapter in onAttach() instead of onCreateView().

@@ +81,5 @@
> +
> +    @Override
> +    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
> +        super.onCreateView(inflater, container, savedInstanceState);
> + 

Nit: trailing whitespace

@@ +92,5 @@
> +    @Override
> +    public void onViewCreated(View view, Bundle savedInstanceState) {
> +        super.onViewCreated(view, savedInstanceState);
> +
> +        mEventHandlers = new EventHandlers();

I'd make this variable local since it's not being used anywhere else.

@@ +138,5 @@
> +                                .commitAllowingStateLoss();
> +        }
> +    }
> +
> +    private BookmarksListAdapter getCursorAdapter(Cursor c) {

Can you just drop this method and merge its logic into refreshListWithCursor()? That's the only place that calls it, and this method as written is kind of weird.

@@ +145,5 @@
> +        } else if (c != null) {
> +            mCursorAdapter.changeCursor(c);
> +        } else {
> +            // do a quick return if just asking for the cached adapter
> +            return mCursorAdapter;

Remove this else block; mCursorAdapter is already returned right below this.

@@ +171,5 @@
> +
> +        // This will update the cursorAdapter to use the new one if it already exists.
> +        mList.setAdapter(getCursorAdapter(cursor));
> +
> +        // Rest the task.

s/Rest/Reset/

@@ +185,5 @@
> +        @Override
> +        public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> +            final ListView list = (ListView) parent;
> +
> +            if (mCursorAdapter == null) {

Is this check necessary? We're guaranteed to always have a CursorAdapter when we create the view.

@@ +221,5 @@
> +                String url = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL));
> +                Tabs.getInstance().loadUrl(url);
> +             }
> +         }
> + 

Nit: trailing whitespace

@@ +272,5 @@
> +            if (id < 0) {
> +                menu.findItem(R.id.remove_history).setVisible(false);
> +            }
> +            menu.setHeaderTitle(title);
> +        

Nit: trailing whitespace

@@ +282,5 @@
> +
> +    /**
> +     * Adapter to back the ListView with a list of bookmarks.
> +     */
> +    private class BookmarksListAdapter extends SimpleCursorAdapter {

Can this class be static? The context is being passed to the constructor, and I don't see it accessing any internal methods/fields in BookmarksPage.

@@ +293,5 @@
> +        // back up the folder heirarchy.
> +        private LinkedList<Pair<Integer, String>> mParentStack;
> +
> +        public BookmarksListAdapter(Context context, Cursor c) {
> +            super(context, -1, c, new String[] {}, new int[] {});

There's only one place we construct a BookmarksListAdapter, and we always give it a null cursor, so we might as well drop the cursor parameter in the constructor here.

@@ +302,5 @@
> +            Pair<Integer, String> rootFolder = new Pair<Integer, String>(mFolderId, mFolderTitle);
> +            mParentStack.addFirst(rootFolder);
> +        }
> +
> +        // Refresh the current folder my executing a new task.

s/my/by/

@@ +322,5 @@
> +         * Moves to parent folder, if one exists.
> +         *
> +         * @return true if moving to parent folder was successful, false otherwise.
> +         */
> +        public boolean moveToParentFolder() {

Doesn't look like we need this boolean return value.

@@ +351,5 @@
> +         */
> +        @Override
> +        public int getItemViewType(int position) {
> +            Cursor c = getCursor();
> + 

Nit: trailing whitespace

@@ +415,5 @@
> +            if (convertView == null) {
> +                if (viewType == VIEW_TYPE_ITEM)
> +                    convertView = LayoutInflater.from(parent.getContext()).inflate(R.layout.bookmark_item_row, null);
> +                else
> +                    convertView = LayoutInflater.from(parent.getContext()).inflate(R.layout.bookmark_folder_row, null);

Nit: braces

::: mobile/android/base/home/TwoLinePageRow.java
@@ +68,5 @@
> +        }
> +
> +        // Update the url with "Switch to tab" if needed.
> +        Integer tabId = null;
> +        if (tabId != null) {

Can you add a FIXME here?

::: mobile/android/base/resources/values/styles.xml
@@ +186,5 @@
> +
> +    <style name="TextAppearance.Widget.TwoLinePageRow.Title" parent="TextAppearance.Medium"/>
> +
> +    <style name="TextAppearance.Widget.TwoLinePageRow.Url" parent="TextAppearance.Micro">
> +        <item name="android:textColor">#777777</item>

Should this color be factored out?

Comment 22

[Sriram Ramasubramanian [:sriram]]

::: mobile/android/base/resources/values/styles.xml
@@ +186,5 @@
> +
> +    <style name="TextAppearance.Widget.TwoLinePageRow.Title" parent="TextAppearance.Medium"/>
> +
> +    <style name="TextAppearance.Widget.TwoLinePageRow.Url" parent="TextAppearance.Micro">
> +        <item name="android:textColor">#777777</item>

Should this color be factored out?

Will use ?android:attr/textColorSecondary instead. That used to be #666666, and was used with about:home alone.

Comment 23

[Sriram Ramasubramanian [:sriram]]

@@ +282,5 @@
> +
> +    /**
> +     * Adapter to back the ListView with a list of bookmarks.
> +     */
> +    private class BookmarksListAdapter extends SimpleCursorAdapter {

Can this class be static? The context is being passed to the constructor, and I don't see it accessing any internal methods/fields in BookmarksPage.

Nope. We use mFolderId and mQueryTask from outside :(

Comment 24

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/5ac643b8155b
http://hg.mozilla.org/projects/fig/rev/e4c63bf2c0e8

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5ac643b8155b
https://hg.mozilla.org/mozilla-central/rev/e4c63bf2c0e8