Closed Bug 1239491 Opened 8 years ago Closed 8 years ago

Follow-up: Use correct CursorLoader implementations and remove SimpleCursorLoader

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(3 files)

bug 760956 found that our SimpleCursorLoader implementation is incorrect. However, most of the crashes are with TopSitesCursorLoader so for simplicity, I'll only focus on fixing that in that bug.

This bug is to use the framework's CursorLoader where possible (to ease maintenance burden) and, where it's not viable, use our custom CursorLoader implementation (probably) added in bug 760956 to fix TopSitesCursorLoader.
It's simpler to lean on the framework than manage our own CursorLoader
implementation.

Review commit: https://reviewboard.mozilla.org/r/30819/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30819/
Attachment #8707654 - Flags: review?(margaret.leibovic)
The first patch should be ready to land – I propose we land it and uplift it. It'd be good to get it on beta (e.g. bug 760956 as the #7 top crasher on beta) though it may not be essential as bug 760956 is likely the only essential part.

In the other patches, I'm stuck on where the cursor is closed when using the CursorLoader directly. Notably, these issues don't come up within the main code line (since we use LoaderManagers): for RemoteTabsCursorLoader, the issue is in an unused method getTabs and for ReadingListLoader, the issue is in a test.
Assignee: nobody → michael.l.comella
When I was initially looking into it, I found the following hard to switch to framework's CursorLoader:
 *BookmarksLoader: loadCursor calls into mDB.getBookmarksInFolder method which is called multiple places so we’d have to duplicate code (or fix it)
 *SearchCursorLoader: Calls into LocalBrowserDB.filterAllSites, which is a shared method (but only by the top sites loader! But that's hard to move over to the framework because it combines three Cursors)
Don't forget to delete the SimpleCursorLoader class!
Summary: Follow-up: Use correct CursorLoader implementations → Follow-up: Use correct CursorLoader implementations and remove SimpleCursorLoader
Important observation:

(In reply to Michael Comella (:mcomella) from bug 760956 comment #35)
> Important observation: the built-in CursorLoader implementation (API 11+)
> [1] uses CancellationSignals while the CursorLoader support library
> implementation [2] does not (and if the support library implementation is
> used, the non-CancellationSignal version will be used *on all devices*).
> 
> Meaning perhaps the CancellationSignal is not an essential part of the
> implementation and my approach in comment 17 is wrong. That or it's a bug in
> the Android framework that was later fixed but forgotten in the support
> library and on GB (perhaps because CancellationSignal is 16+, or earlier
> with the support library).
> 
> Since I'm already so close, I might as well finish this and try it out
> though.
> 
> [1]:
> http://androidxref.com/5.1.0_r1/xref/frameworks/base/core/java/android/
> content/CursorLoader.java
> [2]:
> http://androidxref.com/5.1.0_r1/xref/frameworks/support/v4/java/android/
> support/v4/content/CursorLoader.java

Let's see what we find out when this lands in bug 760956.
(In reply to Michael Comella (:mcomella) from comment #7)
> Important observation:
> 
> (In reply to Michael Comella (:mcomella) from bug 760956 comment #35)
> > Important observation: the built-in CursorLoader implementation (API 11+)
> > [1] uses CancellationSignals while the CursorLoader support library
> > implementation [2] does not (and if the support library implementation is
> > used, the non-CancellationSignal version will be used *on all devices*).
> > 
> > Meaning perhaps the CancellationSignal is not an essential part of the
> > implementation and my approach in comment 17 is wrong. That or it's a bug in
> > the Android framework that was later fixed but forgotten in the support
> > library and on GB (perhaps because CancellationSignal is 16+, or earlier
> > with the support library).
> > 
> > Since I'm already so close, I might as well finish this and try it out
> > though.
> > 
> > [1]:
> > http://androidxref.com/5.1.0_r1/xref/frameworks/base/core/java/android/
> > content/CursorLoader.java
> > [2]:
> > http://androidxref.com/5.1.0_r1/xref/frameworks/support/v4/java/android/
> > support/v4/content/CursorLoader.java
> 
> Let's see what we find out when this lands in bug 760956.

This didn't help resolve the crash. Is there still a reason to go forward with this bug, or is it a WONTFIX?
Flags: needinfo?(michael.l.comella)
It'd be cool to use the framework's CursorLoader to relieve our maintenance burden but considering that code is unlikely to change and the amount of effort it'd take to get all of our Cursors off our custom implementation isn't worth it – there are better things to spend our time on: WONTFIX.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(michael.l.comella)
Resolution: --- → WONTFIX
Attachment #8707654 - Flags: review?(margaret.leibovic)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: