http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeListView.java#128. The HomeContextMenuInfo constructor currently contains logic to try and figure out cursor which has been passed in. Currently it can only handle a combined view cursor or a bookmarks table cursor. Similar cursors (e.g a reading_list table and bookmarks table cursor) may not always be easily distinguishable. Refactor creation flow of HomeContextMenuInfo to allow more flexibility.

Suggestion?: Remove cursor detection logic from HomeContextMenuInfo. Make home panel fragments responsible for creating HomeContextMenuInfo (via an interface to HomeListView). Since a home panel fragment loads the cursor, it knows whats inside it and can easily create HomeContextMenuInfo out if it (via an interface to the HomeListView).

Thanks for filing this bug! I actually mentioned something about this in bug 959777 comment 26, since we're thinking of using HomeListView for our dynamic panel views, in which case there would be yet another type of cursor flowing through HomeListView.

currently doing a try run

Comment on attachment 8367481 [details] [diff] [review] Create factory method for making menu info objects from cursors Review of attachment 8367481 [details] [diff] [review]: ----------------------------------------------------------------- Looking good, but I think we can do better. ::: mobile/android/base/home/BookmarksPanel.java @@ +65,5 @@ > final View view = inflater.inflate(R.layout.home_bookmarks_panel, container, false); > > mList = (BookmarksListView) view.findViewById(R.id.bookmarks_list); > > + ((HomeListView) mList).setContextMenuInfoFactory(new HomeListView.ContextMenuInfoFactory() { It would be nice if we could find a way to avoid needing to cast all of these lists, but I can't think of a better way to do it. ::: mobile/android/base/home/HomeContextMenuInfo.java @@ +38,5 @@ > + return historyId > -1; > + } > + > + public boolean isInReadingList() { > + return inReadingList; It seems odd to have a getter like this, given that the properties are all public. In fact, you don't even use this method ;) @@ +43,5 @@ > + } > + > + public String getDisplayTitle() { > + return TextUtils.isEmpty(title) ? > + StringUtils.stripCommonSubdomains(StringUtils.stripScheme(url, StringUtils.UrlFlags.STRIP_HTTPS)) : title; I think it would be clearer to break this up like: if (!TextUtils.isEmpty(title)) { return title; } return StringUtils...; ::: mobile/android/base/home/HomeFragment.java @@ +171,5 @@ > } > > if (itemId == R.id.home_remove) { > // Prioritize removing a history entry over a bookmark in the case of a combined item. > final int historyId = info.historyId; Instead of declaring this historyId, you can just use info.historyId directly down below, now that you have this hadHistoryId helper method.

Comment on attachment 8367481 [details] [diff] [review] Create factory method for making menu info objects from cursors Review of attachment 8367481 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/BookmarksPanel.java @@ +65,5 @@ > final View view = inflater.inflate(R.layout.home_bookmarks_panel, container, false); > > mList = (BookmarksListView) view.findViewById(R.id.bookmarks_list); > > + ((HomeListView) mList).setContextMenuInfoFactory(new HomeListView.ContextMenuInfoFactory() { alright will look into it ::: mobile/android/base/home/HomeContextMenuInfo.java @@ +38,5 @@ > + return historyId > -1; > + } > + > + public boolean isInReadingList() { > + return inReadingList; when the reading list content provider is ready it will contain something like: return readingListItemId > -1

(In reply to Sola Ogunsakin [:sola] from comment #5) > ::: mobile/android/base/home/HomeContextMenuInfo.java > @@ +38,5 @@ > > + return historyId > -1; > > + } > > + > > + public boolean isInReadingList() { > > + return inReadingList; > > when the reading list content provider is ready it will contain something > like: return readingListItemId > -1 Okay, well in that case you should use isInReadingList() instead of directly checking inReadingList in HomeFragment :)

Remove unnecessary list casting Remove unnecessary local vars

Comment on attachment 8367708 [details] [diff] [review] Implement changes Review of attachment 8367708 [details] [diff] [review]: ----------------------------------------------------------------- This is looking better, but unfortunately, some of my previous comments didn't get posted :( ::: mobile/android/base/home/HomeContextMenuInfo.java @@ +26,5 @@ > + public int historyId = -1; > + public int bookmarkId = -1; > + > + public HomeContextMenuInfo(View targetView, int position, long id) { > + super(targetView, position, id); If you're just calling super with the same parameters in the body of the constructor, I think you can get rid of this constructor. ::: mobile/android/base/home/HomeListView.java @@ +89,5 @@ > > // HomeListView could hold headers too. Add a context menu info only for its children. > if (item instanceof Cursor) { > Cursor cursor = (Cursor) item; > + if (cursor == null) return false; General style comment: brace single-line ifs, and put the body on a new line. You also probably need to make sure to set mContextMenuInfo to null in this case. However, I think you can get rid of this if you follow my suggestion below. @@ +103,5 @@ > + return showContextMenuForChild(HomeListView.this); > + } else { > + // We don't show a context menu for folders > + return false; > + } Grr... I think my browser closed in the middle of my review, and some of my comments were lost. I had made a comment here that I don't think HomeListView should know about bookmarks. The point of this refactoring is to make HomeListView more generic, and I think we should aim to remove all of the BrowserContract imports in this file. I think we could do something like have the factory return null if we shouldn't be showing a context menu, so in the case of bookmark folders, the factory in BookmarksPanel can check to see if the row is a folder, and return null in that case. ::: mobile/android/base/home/MostRecentPanel.java @@ +119,5 @@ > + if (cursor.isNull(bookmarkIdCol)) { > + info.bookmarkId = -1; > + } else { > + info.bookmarkId = cursor.getInt(bookmarkIdCol); > + } This logic is tricky. Please copy over the comments explaining it from the original code.

Comment on attachment 8367708 [details] [diff] [review] Implement changes Review of attachment 8367708 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/HomeContextMenuInfo.java @@ +26,5 @@ > + public int historyId = -1; > + public int bookmarkId = -1; > + > + public HomeContextMenuInfo(View targetView, int position, long id) { > + super(targetView, position, id); cant inherit the constructor, so we need to expose it

Comment on attachment 8368040 [details] [diff] [review] Implement changes Review of attachment 8368040 [details] [diff] [review]: ----------------------------------------------------------------- r+ with comments addressed. ::: mobile/android/base/home/BookmarksPanel.java @@ +68,5 @@ > > + mList.setContextMenuInfoFactory(new HomeListView.ContextMenuInfoFactory() { > + @Override > + public HomeContextMenuInfo makeInfoForCursor(View view, int position, long id, Cursor cursor) { > + final int typeCol = cursor.getColumnIndex(Bookmarks.TYPE); There should always be a type column in a bookmark cursor, so I think you could just do a getColumnIndexOrThrow here, and not bother checking if typeCol == -1. http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#70 @@ +75,5 @@ > + return null; > + } > + final HomeContextMenuInfo info = new HomeContextMenuInfo(view, position, id); > + info.url = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL)); > + info.title = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.TITLE)); Since Bookmarks extends URLColumns, you could actually just use Bookmarks.URL/Bookmarks.TITLE here. ::: mobile/android/base/home/HomeListView.java @@ -7,5 @@ > > import org.mozilla.gecko.R; > -import org.mozilla.gecko.db.BrowserContract.Bookmarks; > -import org.mozilla.gecko.db.BrowserContract.Combined; > -import org.mozilla.gecko.db.BrowserContract.URLColumns; Nice :) @@ +91,5 @@ > + mContextMenuInfo = mContextMenuInfoFactory.makeInfoForCursor(view, position, id, cursor); > + return showContextMenuForChild(HomeListView.this); > + } > + > + return false; Instead of adding another return statement here, you could just put the one at the bottom below the else statement. In fact, you don't even need an else statement. And to repeat what I said before you should make sure that you set mContextMenuInfo to null if you're returning false. So actually, I think the best way to do this would be: if (item != null && mContextMenuInfoFactory != null && item instanceof Cursor) { mContextMenuInfo = ... return show... } mContextMenuInfo = null; return false;