This is similar to bug 885590, but for all other places we use SimpleCursorAdapter. Those places include BrowserSearch, HistoryPage, and LastTabsPage. Basically, we don't take advantage of the benefits of SimpleCursorAdapter, so it makes more sense for use to just use CursorAdapter.

Replaced SimpleCursorAdapter to CursorAdapter. Now that adapter is super simple. More changes: 1. The time to load a view by storing a reference to LayoutInflater, and not storing a reference is --- 3ms! So, there is no point in storing a LayoutInflater anywhere, ever! 2. Whenever possible it is better to make an inner class static. http://stackoverflow.com/questions/3106912/why-does-android-prefer-static-classes

HistoryAdapter is the model for the HistoryListView. There was no reason for the HistoryListView to have methods related to its model outside the adapter. This patches moves all the "history section" based methods inside the adapter. Voila! The adapter is now static and :bnicholson is happy!

Comment on attachment 781505 [details] [diff] [review] Part 4: Use CursorAdapter in HistoryPage Ideally we need to override only bindView() and newView(). However, the cursor doesn't have any information about the "history sections". Hence the getView() is overriden to take care of this scenario. This logic is same as in BookmarksPage.

Comment on attachment 781505 [details] [diff] [review] Part 4: Use CursorAdapter in HistoryPage Review of attachment 781505 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/HistoryPage.java @@ +248,4 @@ > > + @Override > + public View newView(Context context, Cursor cursor, ViewGroup parent) { > + return LayoutInflater.from(mContext).inflate(R.layout.home_item_row, parent, false); This could be just "context". I'll make that change.

I tried making SearchAdapter a static class. That's a bigger mess touching many things outside. I don't want to touch it yet. I've made the CursorLoader changes.

I am motivated to move the Adapters out. This way, they'll always remain independent of the Views they back and force anyone not to make them dependent. I made it for TopBookmarksAdapter and BookmarksListAdapter. These two adapters can follow suit.

Comment on attachment 781493 [details] [diff] [review] Part 1: CusorAdapter in LastTabsPage Review of attachment 781493 [details] [diff] [review]: ----------------------------------------------------------------- Nice.

Comment on attachment 781504 [details] [diff] [review] Part 3: Make HistoryAdapter static I still want to think about this one a bit, and I'd like to know what lucasr's opinion is as well.

https://hg.mozilla.org/projects/fig/rev/0e590de383a8 https://hg.mozilla.org/projects/fig/rev/1b246e4cd75a

Comment on attachment 781510 [details] [diff] [review] Part 5: Use CusorLoader with BrowserSearch Review of attachment 781510 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/BrowserSearch.java @@ +640,3 @@ > > @Override > public View getView(int position, View convertView, ViewGroup parent) { Can you add a comment about why we need to override getView? It isn't always necessary for implementing a CursorAdapter, so adding a comment that this is special might prevent some bad copypasta in the future :) @@ +663,5 @@ > mAnimateSuggestions = false; > } > > return row; > } else { I know it's not your code, but we don't need an else statement, since we're returning in the if statement.

(In reply to :Margaret Leibovic from comment #12) > Comment on attachment 781510 [details] [diff] [review] > Part 5: Use CusorLoader with BrowserSearch > > Review of attachment 781510 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/home/BrowserSearch.java > @@ +640,3 @@ > > > > @Override > > public View getView(int position, View convertView, ViewGroup parent) { > > Can you add a comment about why we need to override getView? It isn't always > necessary for implementing a CursorAdapter, so adding a comment that this is > special might prevent some bad copypasta in the future :) > > @@ +663,5 @@ > > mAnimateSuggestions = false; > > } > > > > return row; > > } else { > > I know it's not your code, but we don't need an else statement, since we're > returning in the if statement. The else statement does a return too. It calls a super.getView(). Also, I would like to have the else statement here as it makes the code flow logically.

(In reply to Sriram Ramasubramanian [:sriram] from comment #13) > > @@ +663,5 @@ > > > mAnimateSuggestions = false; > > > } > > > > > > return row; > > > } else { > > > > I know it's not your code, but we don't need an else statement, since we're > > returning in the if statement. > > The else statement does a return too. It calls a super.getView(). Also, I > would like to have the else statement here as it makes the code flow > logically. Well, yes, the method does return at the end. It's just not necessary to have an else because the only time we end up below the if is if we weren't in it. It's not a big deal, I just prefer to avoid unnecessary indentation.

Comment on attachment 781504 [details] [diff] [review] Part 3: Make HistoryAdapter static Review of attachment 781504 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine. ::: mobile/android/base/home/HistoryPage.java @@ +287,5 @@ > + > + return sectionsBefore; > + } > + > + private HistorySection getHistorySectionForTime(long from, long time) { This method can be made static.

Comment on attachment 781510 [details] [diff] [review] Part 5: Use CusorLoader with BrowserSearch Review of attachment 781510 [details] [diff] [review]: ----------------------------------------------------------------- This is a case where I think using CursorLoader actually makes code harder to read. Handling everything in getView() makes things simpler. Now you're splitting the responsibility of inflating and binding to separate methods for only one of the view types, which is harder to understand imo.

Comment on attachment 781505 [details] [diff] [review] Part 4: Use CursorAdapter in HistoryPage Review of attachment 781505 [details] [diff] [review]: ----------------------------------------------------------------- Same here. I don't think this patch improves readability. It just makes splits responsibility of creating/binding views into different methods for only one type of view. I'd prefer to keep everything in getView() instead.

(In reply to Lucas Rocha (:lucasr) from comment #17) > Comment on attachment 781510 [details] [diff] [review] > Part 5: Use CusorLoader with BrowserSearch > > Review of attachment 781510 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is a case where I think using CursorLoader actually makes code harder > to read. Handling everything in getView() makes things simpler. Now you're > splitting the responsibility of inflating and binding to separate methods > for only one of the view types, which is harder to understand imo. I agree that CursorAdapter isn't the best in this case. That's because the CursorAdapter is used to use it only with cursors. But then we store our data in other places too, along with a cursor, and the list rows are depending on both these kinds of data. So, just a cursor-based adapter cannot help us here. But, using a SimpleCursorAdapter is more than what we need. We don't use the layout passed to it. We don't allow it to do stuff that it is intended to do.

(In reply to Lucas Rocha (:lucasr) from comment #18) > Comment on attachment 781505 [details] [diff] [review] > Part 4: Use CursorAdapter in HistoryPage > > Review of attachment 781505 [details] [diff] [review]: > ----------------------------------------------------------------- > > Same here. I don't think this patch improves readability. It just makes > splits responsibility of creating/binding views into different methods for > only one type of view. I'd prefer to keep everything in getView() instead. I pushed the patch as I got an r+ already ;)

(In reply to Sriram Ramasubramanian [:sriram] from comment #20) > (In reply to Lucas Rocha (:lucasr) from comment #18) > > Comment on attachment 781505 [details] [diff] [review] > > Part 4: Use CursorAdapter in HistoryPage > > > > Review of attachment 781505 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Same here. I don't think this patch improves readability. It just makes > > splits responsibility of creating/binding views into different methods for > > only one type of view. I'd prefer to keep everything in getView() instead. > > I pushed the patch as I got an r+ already ;) Lucas still has the right to dislike it :) I don't have a big opinion either way, but we can always back this out or file a follow-up to change it if there's a better solution.

(In reply to :Margaret Leibovic from comment #21) > (In reply to Sriram Ramasubramanian [:sriram] from comment #20) > > (In reply to Lucas Rocha (:lucasr) from comment #18) > > > Comment on attachment 781505 [details] [diff] [review] > > > Part 4: Use CursorAdapter in HistoryPage > > > > > > Review of attachment 781505 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > Same here. I don't think this patch improves readability. It just makes > > > splits responsibility of creating/binding views into different methods for > > > only one type of view. I'd prefer to keep everything in getView() instead. > > > > I pushed the patch as I got an r+ already ;) > > Lucas still has the right to dislike it :) I don't have a big opinion either > way, but we can always back this out or file a follow-up to change it if > there's a better solution. If a better solution is recommended, I can push it.

Comment on attachment 781505 [details] [diff] [review] Part 4: Use CursorAdapter in HistoryPage Giving r- just to make it more clear that I don't agree that this patch is an improvement over the current code. Keeping everything in getView() is easier to understand for adapters with multiple view types. You can still inherit from CursorAdapter if you want.

Comment on attachment 781510 [details] [diff] [review] Part 5: Use CusorLoader with BrowserSearch Review of attachment 781510 [details] [diff] [review]: ----------------------------------------------------------------- Ditto.

Backed Part 5 out as suggested: http://hg.mozilla.org/projects/fig/rev/ce8fb2c1f618 Part 4 was not pushed.

(In reply to Lucas Rocha (:lucasr) from comment #23) > Comment on attachment 781505 [details] [diff] [review] > Part 4: Use CursorAdapter in HistoryPage > > Giving r- just to make it more clear that I don't agree that this patch is > an improvement over the current code. Keeping everything in getView() is > easier to understand for adapters with multiple view types. You can still > inherit from CursorAdapter if you want. The reason for overriding bindView() and newView() was because they are abstract methods and have to be implemented. So, if this class inherits CursorAdapter, we need to override those two methods -- that was the reason for the more complicated approach. Anyways, backed out.

Backout for wrong bug number: http://hg.mozilla.org/projects/fig/rev/58523c4666e8 Original backout: http://hg.mozilla.org/projects/fig/rev/a960b4cb3378

Not yet fixed-fig, part 3 remains :P

SimpleCursorAdapter is clearly not for our needs (or I hate a bunch of -1, new int[] { }). How about a ComplexCursorAdapter? Just like the Simple version, this takes in a bunch of view types and resource ids. newView()s are created based on view-types. The classes extending this will just have to bindViews(). This cleans up a bunch of (convertView != null) cases.

Comment on attachment 782777 [details] [diff] [review] Part 6: WIP: ComplexCursorAdapter Review of attachment 782777 [details] [diff] [review]: ----------------------------------------------------------------- Looks like a handy thing to have. Just needs some API cleanups. Maybe move this to a separate bug so that you can close this bug? ::: mobile/android/base/home/ComplexCursorAdapter.java @@ +17,5 @@ > + * SimpleCursorAdapter's help is unusable for our needs. > + * ComplexCursorAdapter can take a set of view-types and map them to a set of layouts. > + * Any extending class will need to bindView(). > + */ > +abstract class ComplexCursorAdapter extends CursorAdapter { Maybe MultiTypeCursorAdapter is more clear? @@ +29,5 @@ > + mViewTypes = viewTypes; > + mLayouts = layouts; > + > + if (mViewTypes.length != mLayouts.length) { > + throw new IllegalStateException("Y U NO GIMME SAME SIZE ARRAYS??"); A better exception message please :-) @@ +45,5 @@ > + return convertView; > + } > + > + @Override > + public void bindView(View view, Context context, Cursor cursor) { Make it final so that subclasses can't mistakenly override it? @@ +50,5 @@ > + // Do nothing. > + } > + > + @Override > + public View newView(Context context, Cursor cursor, ViewGroup parent) { Same here: make it final so that subclasses can't mistakenly override it? @@ +54,5 @@ > + public View newView(Context context, Cursor cursor, ViewGroup parent) { > + return null; > + } > + > + public View newView(Context context, int position, ViewGroup parent) { And here: make it final so that subclasses can't mistakenly override it? ::: mobile/android/base/home/HistoryPage.java @@ +197,5 @@ > return ROW_STANDARD; > } > > @Override > public int getViewTypeCount() { This should be a final method in ComplexCursorAdapter() that always returns mViewTypes.length. More solid, less error-prone.

Closing this bug. Part 6 will be handled separately.

https://hg.mozilla.org/mozilla-central/rev/0e590de383a8 https://hg.mozilla.org/mozilla-central/rev/1b246e4cd75a https://hg.mozilla.org/mozilla-central/rev/4c3421eb1d43