Code review comment for lp:~osomon/webbrowser-app/thumbnails

Revision history for this message
Olivier Tilloy (osomon) wrote :

> Looks like there is a jenkins issue causing a failure, but I tested the
> functionality and it works well and looks good. Before we land this I'd like
> to understand the impact on memory this has to make sure it is acceptable. And
> I have a few other questions..
>
> Specifically:
> - roughly what is the memory usage associated with generating and loading each
> thumbnail
> - how can we limit the thumbnail cache as this seems it will grow limitlessly
> as history grows. We probably need some sort of hard limit on disk size or
> number stored so we don't chew up the users disk space. And a way to recreate
> if one from the cache doesn't exist.
> - Are all thumbnails for the history items loaded in memory when the browser
> is started?
> - Or do we only load thumbnails for images that are visible on the screen? I
> hope so :)
>
> We should ensure that if you have hundreds/thousands of history items (which
> is likely) that we're not creating and loading thumbnails for all of these up
> front but only on demand.

As the timeline view is using nested list views, delegates are created on demand only when needed, and so are images loaded. I haven’t done any specific memory measurements, but I expect the memory consumption related to thumbnails to remain very reasonable (the thumbnails are square PNG images 12×12 grid units), and especially on a small screen such as a phone, where very few delegates are visible at any given time.
We’re not even loading any thumbnail when the application starts, as the timeline view isn’t visible yet, so it doesn’t instantiate any delegate.

Limiting the size of the cache is indeed a problem which this branch doesn’t address at all. I guess we could define a size limit, and when that limit is reached do some cleanup by removing the oldest thumbnails.

There’s also one issue that this branch doesn’t address: if a thumbnail already exists for a page, it’s never going to be re-generated, even if the contents of the page change. An option would be to force re-generating the thumbnail if it’s older than a given interval, e.g. one week.

I believe those two issues can (should) be addressed in other branches.

« Back to merge proposal