Cleanup bookmarks based AsyncTask

RESOLVED FIXED in Firefox 13

Status

()

Firefox for Android
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sriram, Assigned: sriram)

Tracking

unspecified
Firefox 14
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox13 fixed, firefox14 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 609415 [details] [diff] [review]
Patch

This patch cleans up the Bookmarks code to have a single AsyncTask.
Attachment #609415 - Flags: review?(mark.finkle)
Attachment #609415 - Flags: review?(margaret.leibovic)
(Assignee)

Updated

6 years ago
Depends on: 715274
(Assignee)

Updated

6 years ago
Assignee: nobody → sriram
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 :)
Attachment #609415 - Flags: review?(mark.finkle) → review+

Comment 3

6 years ago
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 :)
Attachment #609415 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 4

6 years ago
> >+            // 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.
(Assignee)

Comment 5

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/2725a65f0294
(Assignee)

Comment 6

6 years ago
Created attachment 609543 [details] [diff] [review]
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]
Attachment #609543 - Flags: feedback?(mark.finkle)
Attachment #609543 - Flags: feedback?(margaret.leibovic)

Comment 7

6 years ago
(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
Status: NEW → ASSIGNED
Whiteboard: [leave open]
Target Milestone: --- → Firefox 14

Comment 8

6 years ago
(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.
(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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox13: --- → fixed
status-firefox14: --- → fixed
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [leave open]

Comment 10

5 years ago
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 :).
Attachment #609543 - Flags: feedback?(mark.finkle)
Attachment #609543 - Flags: feedback?(margaret.leibovic)
(Assignee)

Comment 11

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.