Right now, we only fetch 100 items. We should consider fetching more when the user reaches the end of the list (e.g., adding a loading spinner, and then loading more items, inserting and notifying of more items being added into the RecyclerView before the loading spinner, until there are no more items). This is a little tricky at the moment because we use a Cursor [1] which isn't modifiable (by adding new items), and we'll need to come up with a new query that picks up where the last one left off [2]. This will use an onScrollListener to determine when to load new items. (Not using a Cursor for history will also have the advantage of being able to remove single history items without reloading the entire list, although we'll have to account for that in new query.) [1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java#52 [2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java#233

We could increase in increments of 25, as well. It doesn't have to be 100 items at a time.

Also see bug 1208472 about raising or dropping the limit of 100.

Hi there, I'd like to get started on this. Do you have any advice on how to proceed?

Hello, can anybody help me around?

Could someone assign me to this bug please?

Hello Chenxia, I would like to work on this.

Sebastian is this still a good mentor bug?

Definitely not a good "first" bug. And as liuche is currently busy with other projects I'll remove her from the mentor list.

(In reply to Chenxia Liu [:liuche] from comment #0) > (Not using a Cursor for history What would that mean something like defining a HistoryEntry object and then parsing the cursor we're getting from the DB into a collection of these objects, or am I way off the mark here?

I've hacked something together that more or less seems to work.

What I'm not sure - do we want some replacement telemetry for this (https://dxr.mozilla.org/mozilla-central/rev/d6f2a893cd7a77cacbb1b139fff78d09c2f001e0/mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java#197)?

Comment on attachment 8885115 [details] Bug 1269913 - Part 0 - Clean up imports. https://reviewboard.mozilla.org/r/155516/#review166164

Comment on attachment 8885116 [details] Bug 1269913 - Part 1 - Replace comment that function may return null by annotation. https://reviewboard.mozilla.org/r/155506/#review166166

Comment on attachment 8885117 [details] Bug 1269913 - Part 2 - Handle history and Sync loader resets separately. https://reviewboard.mozilla.org/r/155512/#review166168

Comment on attachment 8885118 [details] Bug 1269913 - Part 3 - Add method for retrieving only history that is older than a certain date. https://reviewboard.mozilla.org/r/155508/#review166170

Is it correct that we append additional history objects to the array? Doesn't have this a negative impact on memory usage? Wouldn't it be more effective to use a (nearly) indefinitely Cursor? ... assuming that traversing the Cursor does not mean keeping everything in memory.

(In reply to Sebastian Kaspari (:sebastian) from comment #32) > Is it correct that we append additional history objects to the array? > Doesn't have this a negative impact on memory usage? Wouldn't it be more > effective to use a (nearly) indefinitely Cursor? ... assuming that > traversing the Cursor does not mean keeping everything in memory. Yes, if you'd scroll and scroll and scroll and scroll and so on, the list would keep growing. I'll try to find out how much memory usage that actually means and how using a cursor for the whole history (but limiting the amount of data that's added to the list until we scroll) would compare with it.

Comment on attachment 8885118 [details] Bug 1269913 - Part 3 - Add method for retrieving only history that is older than a certain date. https://reviewboard.mozilla.org/r/155508/#review168974 ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:785 (Diff revision 2) > null); > } > > @Nullable > @Override > public Cursor getRecentHistory(ContentResolver cr, int limit) { Do you have any users that will call into `getRecentHistory(ContentResolver cr, int limit)`? If not, just delete the old signature. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:804 (Diff revision 2) > Combined.URL, > Combined.TITLE, > Combined.DATE_LAST_VISITED, > Combined.VISITS }, > - History.DATE_LAST_VISITED + " > 0", > + History.DATE_LAST_VISITED + " > 0 AND " > + + History.DATE_LAST_VISITED + " < " + lastVisited, As a matter of general sql hygiene, use bound params: `History.DATE_LAST_VISITED + " > 0 AND " + History.DATE_LAST_VISITED + " < ?", new String[] { String.valueOf(lastVisited) }`

Comment on attachment 8885119 [details] Bug 1269913 - Part 4 - Switch the HistoryAdapter from storing the cursor directly to storing a list of history items instead. https://reviewboard.mozilla.org/r/155510/#review168978 ::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:380 (Diff revision 2) > + } > + > + List<HistoryEntry> historyItems = new ArrayList<>(c.getCount()); > + > + do { > + int _id = c.getInt(c.getColumnIndexOrThrow(BrowserContract.Combined._ID)); Fetch all column indecies you're going to use outside of the while loop. No need to do it over and over again.

I was going to say that simply dropping all limits and getting the whole cursor didn't looked too bad, but I've forgotten that our history limit is either 2000 entries or four weeks, whichever is *more*. I use a copy of my normal profile's (which is synced with Desktop) browser.db, but since I hadn't updated that in a while, all the excess history had already been expired down to 2000 items. In that case getting all history in one go doesn't seem too unreasonable - the HistoryEntry approach uses approximately 750 - 800 kB more in Java allocations (of which 700 kB are the HistoryEntries themselves) after scrolling to the end of the list as compared to using the cursor, while the cursor in turn only needs ~ 500 kB of native allocations to hold 2000 entries. Cursor loading time doesn't increase very much because there is some overhead in getting a cursor in the first place ((125 ms for 2000 items vs. 90 ms for 100 items) and the time required for populateSectionHeaders increases presumably linearly with the amount of data, but still requires only ~9 ms. After using a fresh copy of browser.db things look different, though. Now that I've got ~25,000 history entries, getting the cursor alone will take almost a second (although at least that happens on a background thread) and the performance of populateSectionHeaders degrades *severly* once the cursor becomes so big that it is no longer able to cache everything in memory [1] - now it takes around 4 s to complete, which is approximately 35x slower (although that includes one small change that helps when everything is in memory, but makes things worse when accessing the opposite end of the cursor forces an expensive requery)! That means that - we can probably safely increase the amount of history we fetch in one go to somewhat more than 100 items, but probably not much more than 2000 to 3000 items - just keeping the cursor around is more memory efficient than keeping parsed HistoryEntries around, but slows down data access once we run into the cursor window limit - a MatrixCursor could be used to combine cursors that individually stay below the cursor window limit without presumably too much overhead, but unfortunately that's not really compatible with using a CursorLoader to incrementally load things, because the CursorLoader insists on closing the previously loaded cursor when handing you a new one So - we could either live with the memory overhead of turning the cursor into HistoryEntries, which allows us to parse the cursor incrementally (maybe that should be done on a background thread as well) and also means that populateSectionHeaders will remain relatively fast by operating on data in memory (and for large data sets it could be made more efficient by using a binary search for the position of the next section header instead of iterating over every item) -- although that still leaves the problems that we need to reload everything when the underlying dataset changes (though if the HistoryEntry parsing could be done in the background and populateSectionHeaders sped up accordingly it might not be too bad) - or else just progressively bump up the query size limit and live with that fact that getting the cursor will be getting slower the further you scroll down the list, although something still needs to be done about populateSectionHeaders in that case - ??? (Keeping in mind that while 100 items is indeed not very much, I'm not sure how commonly people will scroll further than what would fit in a single cursor window, i.e. more than a few thousand items, either) [1] I think I've read somewhere that on older Android versions the cursor window is limited to at most 1 MB, while newer versions (no idea whether that means 4.x, 5 or whatever) can load 2 MB at once).

(In reply to Jan Henning [:JanH] from comment #36) > - we could either live with the memory overhead of turning the cursor into > HistoryEntries I've just realised that we don't even get the economy of at least not holding onto the cursor after having parsed its data, because the CursorLoader keeps a reference to it around in order to close it later on. So we would keep both the original cursor (although limited to 1 or 2 MB at most) and the parsed data around, unless we swapped the CursorLoader for something else.

I have some idea for calculating the section headers on the fly, so I'm going to try that approach first.

I've dusted off my WIP patches last week and finally completed them and calculating the section headers. It needs some more refinements, but the basic idea seems to work. It's still not the nicest solution because paging of the data is still left to the cursor, but given that even a single 2 MB cursor can hold around 8000 history items we might be able to live with it for now.

it would be great if you could keep this bug in mind: https://bugzilla.mozilla.org/show_bug.cgi?id=1378026 while implementing this. (so whoever implements the search doesn't has to rewrite/change too much) the same goes for stuff like filters (like on desktop: last week, last month, last year) or jump to date x in history (however that could be implemented. Thx for implementing this :) I managed to exceed 100 urls yesterday on my phone+desktop (Firefox sync) so I noticed that the history doesn't even show every url from one day.