Closed Bug 1131257 Opened 7 years ago Closed 7 years ago

Split ReadingListAccessor out of BrowserDB

Categories

(Firefox for Android Graveyard :: Data Providers, defect)

All
Android
defect
Not set
normal

Tracking

(firefox38 fixed, firefox39 fixed)

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(1 file)

Centralizing reading list access logic will make Bug 1130461 much easier. This bug is the first part of that.
We follow the same pattern as for URLMetadata, TabsAccessor, and Searches; BrowserDB hands over a single class that's specialized to handle the Reading List.
Attachment #8561694 - Flags: review?(margaret.leibovic)
Landed a cleanup patch.
Whiteboard: [leave open]
Comment on attachment 8561694 [details] [diff] [review]
Split LocalReadingListDB out of LocalBrowserDB. v1

Review of attachment 8561694 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Is there any reason why ReadingListAccessor even needs to live as part of BrowserDB? Does that just make it easier to make this swap right now?

::: mobile/android/base/BrowserApp.java
@@ +1697,5 @@
>              Telemetry.addToHistogram("PLACES_PAGES_COUNT", db.getCount(cr, "history"));
>              Telemetry.addToHistogram("PLACES_BOOKMARKS_COUNT", db.getCount(cr, "bookmarks"));
>              Telemetry.addToHistogram("FENNEC_FAVICONS_COUNT", db.getCount(cr, "favicons"));
>              Telemetry.addToHistogram("FENNEC_THUMBNAILS_COUNT", db.getCount(cr, "thumbnails"));
> +            Telemetry.addToHistogram("FENNEC_READING_LIST_COUNT", db.getReadingListAccessor().getCount(getContentResolver()));

Why aren't we just using the cr local variable here?
Attachment #8561694 - Flags: review?(margaret.leibovic) → review+
Thanks for the review!

> LGTM. Is there any reason why ReadingListAccessor even needs to live as part
> of BrowserDB? Does that just make it easier to make this swap right now?

Making life easier is my main reason.

I should note that the RL tables are actually in the same DB, so I can see a legitimate independent argument, too. If Search/URLMetadata/etc. are owned by BrowserDB, then RL should be too.


> Why aren't we just using the cr local variable here?

*shrug*!

Will fix.
Whiteboard: [leave open]
Someone didn't run mcMerge.

http://hg.mozilla.org/mozilla-central/rev/b323b586b5e6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.