739334 - Cleanup bookmarks based AsyncTask

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sriram Ramasubramanian [:sriram]]

There are two AsyncTask's used in searching and displaying Bookmarks. This is redundant as just the argument in the second one differs from the first one. This can be cleaned up to reuse on single AsynTask.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch cleans up the Bookmarks code to have a single AsyncTask.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

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

The goal here is good. Removing redundant code is a good thing.

Looks OK to me. Leaning on Margaret for the details :)

Comment 3

[:Margaret Leibovic]

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

I think we should also let Lucas know about this change, since he's been doing work in here as well, but this looks fine to me.

>diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java

>-            bookmarksList.setAdapter(mBookmarksAdapter);
>+            LinearLayout headerView = mBookmarksAdapter.getHeaderView();
>+            if (headerView == null) {
>+                headerView = (LinearLayout) LayoutInflater.from(mContext).inflate(R.layout.awesomebar_header_row, null);
>+                mBookmarksAdapter.setHeaderView(headerView);
>+            }

I don't think it makes much sense to be storing this header view with the adapter, since the adapter doesn't do anything with it, but I guess there isn't really a better place unless we store it on AweseomeBarTabs, so I guess that's fine.

>+            // Add/Remove header based on the root folder
>+            if (mFolderId == Bookmarks.FIXED_ROOT_ID) {
>+                if (list.getHeaderViewsCount() == 1)
>+                    list.removeHeaderView(headerView);
>+            } else {
>+                if (list.getHeaderViewsCount() == 0)
>+                    list.addHeaderView(headerView, null, true);

This is from your patch for bug 715274, but I think you could factor out these list.getHeaderViewsCount() calls into a temp variable.

>+                ((TextView) headerView.findViewById(R.id.title)).setText(mFolderTitle);

Also from the other patch - I don't really care too much, but I know Lucas likes moving these casts into temp variable as well for better readability :)

Comment 4

[Sriram Ramasubramanian [:sriram]]

> >+            // Add/Remove header based on the root folder
> >+            if (mFolderId == Bookmarks.FIXED_ROOT_ID) {
> >+                if (list.getHeaderViewsCount() == 1)
> >+                    list.removeHeaderView(headerView);
> >+            } else {
> >+                if (list.getHeaderViewsCount() == 0)
> >+                    list.addHeaderView(headerView, null, true);
> 
> This is from your patch for bug 715274, but I think you could factor out
> these list.getHeaderViewsCount() calls into a temp variable.

I thought of moving it to a temporary variable and using it. However, in either of the if..else blocks, the call is made only once -- which made me feel not to go for temporary variables.

> 
> >+                ((TextView) headerView.findViewById(R.id.title)).setText(mFolderTitle);
> 
> Also from the other patch - I don't really care too much, but I know Lucas
> likes moving these casts into temp variable as well for better readability :)

I'm of the other kind :D Unless something is used more than once, I like casting and using it directly.

Comment 5

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2725a65f0294

Comment 6

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: More cleanup

This patch does more cleanup of bookmarks.
1. The BookmarksListView is now a separate entity. Each instance of the ListView will have its own adapter, to manage its data and supply the views for it. This can be changed for phones/tablets into two different versions, if needed -- easily.
2. The Actions required to be done on a bookmark are supplied as an interface "Events". Anything can listen to it -- currently AwesomeBarTabs.java does. The list of listeners can be extended to a List, but we won't need that, as we will have one single listener at any time.
3. AwesomeBarTabs caches the listview, and holds the AsyncTask to perform the DB operations. Later, this can be made into a call to some "DBAccessor", which can give back the results as delegation (like RemoteTabs). In essence, the BookmarksQueryTask can be changed independent of ListView, BookmarksListAdapter.
4. The header view needed for the list, is held with the list.
5. The click event handling for the list, is also with the list.

Basically, the listview and the adapter are coupled together (like TabsTray).
The QueryTask stays with AwesomeBarTabs, which can be changed to anything. The resulting cursor is given to the ListView.

If this approach is fine, we can probably decouple the other tabs too.

[I need to cleanup this patch a bit]

Comment 7

[Ed Morley [:emorley]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/2725a65f0294

https://hg.mozilla.org/mozilla-central/rev/2725a65f0294

Comment 8

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> Created attachment 609543 [details] [diff] [review]
> WIP: More cleanup

This should go in a different bug so that we can close this one. It's confusing to leave a bug open that's already had a patch land.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Margaret Leibovic [:margaret] from comment #8)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> > Created attachment 609543 [details] [diff] [review]
> > WIP: More cleanup
> 
> This should go in a different bug so that we can close this one. It's
> confusing to leave a bug open that's already had a patch land.

Agreed

https://hg.mozilla.org/releases/mozilla-aurora/rev/585261ed3fc3

Comment 10

[:Margaret Leibovic]

Comment on attachment 609543 [details] [diff] [review]
WIP: More cleanup

Clearing feedback, since we mentioned in the comments that we think this should go in a separate bug. Also, at this point I think this is too risky a change to take unless there's some real benefit we're going to get. I definitely agree that cleaning up the code is a worthy goal, so we should revisit this after we ship a 1.0 and have more time to address potential regressions (also it would be nice if we had more test coverage before landing this :).

Comment 11

[Sriram Ramasubramanian [:sriram]]

I happy to take this after 1.0, but I'm not ready for tests. Honestly, if there would be any regressions, they will be caught from old tests. So writing newer tests doesn't sound convincing to me. Those old tests without this change should work properly with these newer changes.