1130809 - Batch-load thumbnails from DB

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

Follow-on from Bug 1130694.

We can now batch up the URLs for tabs that need their thumbnails to be loaded from the DB, and make a single request to fetch them. This'll save ContentResolver round-trip times and some other noise.

Comment 1

[Michael Holloway [:mdholloway]]

This looks like a good next bug for me to work on.  

After looking through the code to better understand what's going on, I put together a little prototype of potential changes to refreshThumbnails() to loop through the tabs for missing thumbnails, grab missing thumbnails from the db as a Cursor, and loop through the tabs again, adding thumbnails in where needed:

public void refreshThumbnails() {
  final BrowserDB db = GeckoProfile.get(mAppContext).getDB();
  ThreadUtils.postToBackgroundThread(new Runnable() {
    @Override
    public void run() {
      List<String> urls;
      Cursor thumbnails;

      for (final Tab tab : mOrder) {
        if (tab.getThumbnail() == null) {
          urls.add(tab.getURL());
        }
      }

      if (urls != null) {
        try {
          thumbnails = db.getThumbnailsForUrls(getContentResolver(), urls);
        } catch (Exception e) {
          // ignore
        }
      }

      if (thumbnails != null) {
        thumbnails.moveToFirst();
        for (final Tab tab : mOrder) {
          if (tab.getThumbnail() == null) {
            if (thumbnails.getBlob() != null) {
              Bitmap bitmap = BitmapUtils.decodeByteArray(thumbnails.getBlob());
              BitmapDrawable thumbnail = new BitmapDrawable(mAppContext.getResources(), bitmap);
              Tabs.getInstance().notifyListeners(tab.getContext(), Tabs.TabEvents.THUMBNAIL);
            }

            if thumbnails.hasNext() {
              thumbnails.moveToNext();
            }
          }
        }
      }
    }
  }
}

Am I on the right track?

Comment 2

[Richard Newman [:rnewman]]

> Am I on the right track?

Yes. You should take a look at TopSitesPanel.ThumbnailsLoader, which is an AsyncLoader for an array of URLs. It already does most of this stuff for you -- you can refactor it out, implement a ThumbnailsLoaderCallbacks to handle stuffing the results into Tabs, and get the advantages of shared code.


Comments on this approach, though:

>       List<String> urls;
>       Cursor thumbnails;

Don't forget to close your cursor in a `finally` block.

>         thumbnails.moveToFirst();
>         for (final Tab tab : mOrder) {

There's no guarantee that this is the same sequence of tabs that you walked before. You need an intermediate step, mapping the IDs of the tabs you used to create `urls` to the images returned by this cursor. You can use a `SparseArray` to do this.

`getThumbnailsForUrls` will return both URLs and thumbnails. I'm not sure whether we should be reliant on the URL or the tab ID for a match, but we definitely don't want to be using the index of the tab!


>             if thumbnails.hasNext() {
>               thumbnails.moveToNext();

This is wrong, but the suggestions I made above will make this disappear.

Comment 3

[Michael Holloway [:mdholloway]]

It looks like TopSitesPanel.ThumbnailsLoader is the right direction to take, but I'm stuck on how to implement it -- I'm starting to get my head wrapped around how Firefox for Android fits together in general, but I've only implemented an AsyncTaskLoader in a much simpler app.  Here, since Tabs isn't an Activity or Fragment subclass, I'm not sure how to start or get access to an existing LoaderManager to call initLoader()...

So, sorry but it looks like I'll need a little more hand-holding than I expected at first.  If you think it'd be easier to talk through on IRC, let me know and I'll hop on.

Comment 4

[Richard Newman [:rnewman]]

Sorry for the late reply to this; I've been a little underwater.

There's an implicit relationship here between Tabs and BrowserApp. Tabs happens to be a singleton, so there isn't a direct reference, but when we're fetching thumbnails there should be an 'enclosing' BrowserApp.

(BrowserApp is an Activity, which gives you a LoaderManager.)

Note that Tabs.refreshThumbnails is only called from Tabs{List,Grid}Layout. In effect, Tabs is the 'model' for those view/controllers.

Tabs{List,Grid}Layout is created from TabsPanel.createTabsLayout, which is called from BrowserApp.onCreateView. That's the containment relationship.

So, if we want to make this a bit more Androidy, the question is: how do we relate Tabs's thumbnail management to this chain of classes?

Taking a look at Tabs{List,Grid}Layout.show() should give you some good ideas. We request thumbnails, take a snapshot of the list of tabs, and register for change notifications. Hell, we already have a TabsGridLayoutAdapter! This doesn't seem too far from a proper loader…

Comment 5

[Michael Holloway [:mdholloway]]

Attachment: 1130809_batch_load_thumbnails_in_progress.patch

Richard, thanks for the explanation of how the pieces fit together, it was very helpful.  I'm attaching an in-progress patch just so you can see where I'm headed with this at the moment, since I probably won't be able to look at it again until Thursday or Friday at the earliest.  Following your earlier advice, I've broken out ThumbnailInfo and ThumbnailsLoader into their own classes under /mobile/android/base, and I have some loader callback code in progress in Tabs.java.  

I'll have to think more about your last points about Tabs{List,Grid}Layout.show() next time I have a chance to sit down with this.  I've also been looking at how the whole process works over in TopSitesPanel world as well, though it looks like there are some separate concerns in play over there, so I won't want to follow it too closely.

("Feedback" seems like a more appropriate flag than "needinfo" for this, unless there's an org-specific usage here I couldn't find via Google.)

Comment 6

[Michael Holloway [:mdholloway]]

All right, I'm back at it.  If I'm following you, a loader performs all the functions in Tabs{List,Grid}Layout.show() except for setting the visibility to View.VISIBLE, so we could clear the rest out, and instead fire up a ThumbnailsLoader to do the job in the TabsPanel case of the if-else block of BrowserApp.onCreateView(...), immediately before calling TabsPanel.createTabsLayout(context, attrs)?

Comment 7

[Richard Newman [:rnewman]]

Sorry, Michael; I'm a little swamped right now. I'll leave this flag open and get back to you next week, I hope.

Comment 8

[Michael Holloway [:mdholloway]]

Attachment: 2015-05-10 working patch

The good news: I came back to this and have some code I'd like to begin testing as a starting point; new patch is attached. 

The bad news: ./mach build doesn't want to build this.  It can't find the new ThumbnailInfo and ThumbnailsLoader classes I added in /mobile/android/base even though Android Studio seemed not to have any trouble.  

See https://pastebin.mozilla.org/8833029.

Comment 9

[Richard Newman [:rnewman]]

Just a flying response on the weekend: files you add need to be added in the right spot (should be obvious; alphabetical and fairly sane sections) to moz.build in mobile/android/base.

Comment 10

[Michael Holloway [:mdholloway]]

Attachment: 2015-05-10 working patch v2

Aha, thanks.  Fennec is up and running again with the new patch, though I haven't gotten to test it yet at all and I'm sure it'll need some more tinkering before it's review-ready.

Comment 11

[Michael Holloway [:mdholloway]]

Attachment: 2015-05-10 working patch v3

Comment 12

[Michael Holloway [:mdholloway]]

The very limited time I have to devote to this nowadays doesn't seem to be sufficient to keep ahead of code changes & merge conflicts.  If someone else wants to take it over, have at it; I think I'll bow out here.

Comment 13

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.