Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Description • 8 years ago

We have multiple bugs about disappearing favicons and some favicons never showing up in the UI. Some of the bugs are caused by how we store favicons and map page urls to favicon urls.

For details see:
* Bug 1269821
* Bug 1271634

As part of this bug I want to look into reorganizing how we store the icons and map the urls to avoid having missing icons int the UI.

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 1 • 8 years ago

From the observed bugs I think:

* Stored icons should have the icon URL as (additional) key. If we have the URL then we always should be able to load the stored icon for this URL.

* The mapping from page URL to icon URL should be stored independently from history or bookmarks. There are various timing problems when a history entry is required to exist in order store this mapping. Whenever we have downloaded an icon we should always be able to store the mapping. When we have a page URL then we should always be able to lookup the icon URL if we have any.

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 2 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe96b9a64fd6

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 10 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=321ef33c47ed

	Andrzej Hunt :ahunt
	Comment 13 • 8 years ago
mozreview-review

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review70304

::: mobile/android/base/java/org/mozilla/gecko/favicons/FaviconGenerator.java:58
(Diff revision 1)
>  
>      private static final int TEXT_SIZE_DP = 12;
>  
> +    public static class IconWithColor {
> +        public final Bitmap bitmap;
> +        public final int color;

Is it worth adding a comment explaining that this is the icon's main/dominant (?) color?

::: mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java:24
(Diff revision 1)
> + */
> +public class IconRequest {
> +    private Context context;
> +
> +    // Those values are written by the IconRequestBuilder class.
> +    /* package-private */ String pageUrl;

(I was wondering whether we could make these final, and then set them in a Constructor, which is called from Builder.build(), but that would add a lot of unnecessary duplication AFAICS.)

	Andrzej Hunt :ahunt
	Comment 14 • 8 years ago
mozreview-review

Comment on attachment 8782417 [details]
Bug 1290014 - Use new icon framework in UI code.

https://reviewboard.mozilla.org/r/71956/#review70426

This is a nice improvement!

::: mobile/android/base/java/org/mozilla/gecko/reader/ReadingListHelper.java:81
(Diff revision 1)
> +                // the Tab class.
> +                //
> +                // The Tab class could just lookup and load the icon itself. All it needs to do is
> +                // to strip the about:reader URL and perform a normal icon load from cache.
> +                //
> +                // A more global solution (looking at desktop and iOS) would be to copy the <link>

Could we file this as a bug against readability?

(I'm not really familiar with how about:reader works, but I'm worried the suggested approach might not work for offline cached article. I really have no idea.)

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabHistoryItemRow.java:29
(Diff revision 1)
>  public class TabHistoryItemRow extends RelativeLayout {
>      private final FaviconView favicon;
>      private final TextView title;
>      private final ImageView timeLineTop;
>      private final ImageView timeLineBottom;
> -    // Listener for handling Favicon loads.
> +    private Future<IconResponse> ongoinngIconLoad;

typo? s/ongoinng/ongoing/

	Andrzej Hunt :ahunt
	Comment 15 • 8 years ago

For the record, I'm getting the following crash (consistently) on my local build of the patches (rebased on top of today's nightly), only on a tablet (I'll try and fix it locally later, but I'm still focussing on the review and want to make sure this doesn't get lost).

08-18 14:10:03.518 18324-18324/org.mozilla.fennec_andrzejhunt E/GeckoCrashHandler: >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
    java.lang.IllegalStateException: Page URL is required
   at org.mozilla.gecko.icons.IconRequestBuilder.build(IconRequestBuilder.java:123)
   at org.mozilla.gecko.home.TwoLinePageRow.update(TwoLinePageRow.java:281)
   at org.mozilla.gecko.home.TwoLinePageRow.updateFromCursor(TwoLinePageRow.java:318)
   at org.mozilla.gecko.home.CombinedHistoryItem$HistoryItem.bind(CombinedHistoryItem.java:80)
   at org.mozilla.gecko.home.CombinedHistoryAdapter.onBindViewHolder(CombinedHistoryAdapter.java:143)
   at org.mozilla.gecko.home.CombinedHistoryAdapter.onBindViewHolder(CombinedHistoryAdapter.java:19)
   at android.support.v7.widget.RecyclerView$Adapter.onBindViewHolder(RecyclerView.java:5277)
   at android.support.v7.widget.RecyclerView$Adapter.bindViewHolder(RecyclerView.java:5310)
   at android.support.v7.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:4568)
   at android.support.v7.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:4461)
   at android.support.v7.widget.LinearLayoutManager$LayoutState.next(LinearLayoutManager.java:1962)
   at android.support.v7.widget.LinearLayoutManager.layoutChunk(LinearLayoutManager.java:1371)
   at android.support.v7.widget.LinearLayoutManager.fill(LinearLayoutManager.java:1334)
   at android.support.v7.widget.LinearLayoutManager.onLayoutChildren(LinearLayoutManager.java:563)
   at android.support.v7.widget.RecyclerView.dispatchLayout(RecyclerView.java:2847)
   at android.support.v7.widget.RecyclerView.onLayout(RecyclerView.java:3145)
   at android.view.View.layout(View.java:14817)
   at android.view.ViewGroup.layout(ViewGroup.java:4631)
   at android.support.v4.widget.SwipeRefreshLayout.onLayout(SwipeRefreshLayout.java:581)
   at android.view.View.layout(View.java:14817)
  [… snip …]

	Andrzej Hunt :ahunt
	Comment 16 • 8 years ago
mozreview-review

Comment on attachment 8782418 [details]
Bug 1290014 - Remove obsolete code.

https://reviewboard.mozilla.org/r/71958/#review70438

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1189
(Diff revision 1)
>       * @param cr The ContentResolver to use.
>       * @param faviconURL The URL of the favicon to fetch from the database.
>       * @return The decoded Bitmap from the database, if any. null if none is stored.
>       */
>      @Override
> -    public LoadFaviconResult getFaviconForUrl(ContentResolver cr, String faviconURL) {
> +    public LoadFaviconResult getFaviconForUrl(Context context, ContentResolver cr, String faviconURL) {

Do we need to pass in the ContentResolver - I think we can get it from context.getContentResolver(), but I'm not sure if that's bad practice?

::: mobile/android/base/java/org/mozilla/gecko/favicons/decoders/ICODecoder.java:282
(Diff revision 1)
>          if (iconDirEntry.payloadIsPNG) {
>              // PNG payload. Simply extract it and decode it.
>              return BitmapUtils.decodeByteArray(decodand, offset + iconDirEntry.payloadOffset, iconDirEntry.payloadSize);
>          }
>  
> -        // The payload is a BMP, so we need to do some magic to get the decoder to do what we want.
> +        // The payload is a BMP, so we need to do some magic to getIcon the decoder to do what we want.

s/getIcon/get/ ;)

::: mobile/android/base/java/org/mozilla/gecko/favicons/decoders/IconDirectoryEntry.java:37
(Diff revision 1)
>          this.payloadOffset = payloadOffset;
>          this.payloadIsPNG = payloadIsPNG;
>      }
>  
>      /**
> -     * Method to get a dummy Icon Directory Entry with the Erroneous bit set.
> +     * Method to getIcon a dummy Icon Directory Entry with the Erroneous bit set.

I think the same thing happened here: s/getIcon/get/

	Andrzej Hunt :ahunt
	Comment 17 • 8 years ago
mozreview-review

Comment on attachment 8782419 [details]
Bug 1290014 - Move decoders from the favicons to the icons package.

https://reviewboard.mozilla.org/r/72320/#review70446

	Andrzej Hunt :ahunt
	Comment 18 • 8 years ago
mozreview-review

Comment on attachment 8782420 [details]
Bug 1290014 - Move code from the FaviconGenerator class to the IconGenerator in the icons package.

https://reviewboard.mozilla.org/r/72322/#review70448

	Andrzej Hunt :ahunt
	Comment 19 • 8 years ago
mozreview-review

Comment on attachment 8782421 [details]
Bug 1290014 - Add additional unit tests.

https://reviewboard.mozilla.org/r/72598/#review70452

	Andrzej Hunt :ahunt
	Comment 20 • 8 years ago
mozreview-review

Comment on attachment 8782508 [details]
Bug 1290014 - DiskStorage: Do not use StandardCharsets.UTF_8. It's only support on API level 19+.

https://reviewboard.mozilla.org/r/72650/#review70454

	Andrzej Hunt :ahunt
	Comment 21 • 8 years ago

(In reply to Andrzej Hunt :ahunt from comment #15)
> For the record, I'm getting the following crash (consistently) on my local
> build of the patches (rebased on top of today's nightly), only on a tablet
> (I'll try and fix it locally later, but I'm still focussing on the review
> and want to make sure this doesn't get lost).

This is happening because of a malformed "about:reader;URL" url (see Bug 1292903) in my history, which results in pageUrl == null at:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java?q=TwoLinePageRow.java&redirect_type=direct#299

I wonder if we should make getUrlFromAboutReader private, and use stripAboutReaderUrl everywhere?

	Andrzej Hunt :ahunt
	Comment 22 • 8 years ago
mozreview-review

Comment on attachment 8782415 [details]
Bug 1290014 - Add DiskLruCache library.

https://reviewboard.mozilla.org/r/69250/#review70462

I think the links in the commit summary are switched in order ([1] should be the github link, [2] the mozilla wiki)?

	Andrzej Hunt :ahunt
	Comment 23 • 8 years ago
mozreview-review

Comment on attachment 8782507 [details]
Bug 1290014 - ResizingProcessor: Do not resize icons loaded from memory.

https://reviewboard.mozilla.org/r/72648/#review70466

	Andrzej Hunt :ahunt
	Comment 24 • 8 years ago
mozreview-review

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review70772

Looks good to me, and seems to work quite reliably. It would definitely be good to land this soon to give it as much time as possible on nightly. (Including things like the crash I found on about:reader;garbage... URLs.)

::: mobile/android/base/java/org/mozilla/gecko/icons/IconDescriptorComparator.java:47
(Diff revision 1)
> +        if (lhsContainer != rhsContainer) {
> +            return lhsContainer ? -1 : 1;
> +        }
> +
> +        // There's no way to know which icon might be better. Just pick rhs.
> +        return 1;

Do we need to prefer any given icon here? It seems like returning 0 here would match the spec.

This probably doesn't really matter, but I'm worried that the clients (TreeSet, which also uses TreeMap?) could get confused if we're delivering inconsistent results - in which case following the spec would reduce the chance of issues in future?

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 25 • 8 years ago

(In reply to Andrzej Hunt :ahunt from comment #15)
> For the record, I'm getting the following crash (consistently) on my local
> build of the patches (rebased on top of today's nightly), only on a tablet
> (I'll try and fix it locally later, but I'm still focussing on the review
> and want to make sure this doesn't get lost).
> 
> 08-18 14:10:03.518 18324-18324/org.mozilla.fennec_andrzejhunt
> E/GeckoCrashHandler: >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
>     java.lang.IllegalStateException: Page URL is required
>    at
> org.mozilla.gecko.icons.IconRequestBuilder.build(IconRequestBuilder.java:123)
>    at org.mozilla.gecko.home.TwoLinePageRow.update(TwoLinePageRow.java:281)
>    at
> org.mozilla.gecko.home.TwoLinePageRow.updateFromCursor(TwoLinePageRow.java:
> 318)
>    at
> org.mozilla.gecko.home.CombinedHistoryItem$HistoryItem.
> bind(CombinedHistoryItem.java:80)

Oh, that's interesting. This means you have a null or empty URL in your history. This is weird. I think I'm going to add a check to TwoLinePageRow and keep the desired validation in IconRequestBuilder.

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 26 • 8 years ago
mozreview-review-reply

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review70304

> Is it worth adding a comment explaining that this is the icon's main/dominant (?) color?

This class is removed in a later commit (sorry for the confusion). But I'll make sure that the IconResponse class has a comment like that.

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 27 • 8 years ago
mozreview-review-reply

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review70772

> Do we need to prefer any given icon here? It seems like returning 0 here would match the spec.
> 
> This probably doesn't really matter, but I'm worried that the clients (TreeSet, which also uses TreeMap?) could get confused if we're delivering inconsistent results - in which case following the spec would reduce the chance of issues in future?

Yeah, unfortunately we have to prefer an icon. Returning 0 means the icons are basically the same and then one of them will not be added to the list. At least there's a unit test that assets/documents this inconsistency and whenever this changes we will have to adapt the test.

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 28 • 8 years ago
mozreview-review-reply

Comment on attachment 8782417 [details]
Bug 1290014 - Use new icon framework in UI code.

https://reviewboard.mozilla.org/r/71956/#review70426

> Could we file this as a bug against readability?
> 
> (I'm not really familiar with how about:reader works, but I'm worried the suggested approach might not work for offline cached article. I really have no idea.)

The suggested approach should load the same icon but without some of the loops. But yeah, I talked with gijs a bit about this on IRC. I'll file a bug.

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 29 • 8 years ago
mozreview-review-reply

Comment on attachment 8782418 [details]
Bug 1290014 - Remove obsolete code.

https://reviewboard.mozilla.org/r/71958/#review70438

> Do we need to pass in the ContentResolver - I think we can get it from context.getContentResolver(), but I'm not sure if that's bad practice?

That's right. This code is grown into that. I started with just the ContentResolver (like all the other methods) but then I suddenly required a context at the end of the stack to decode the icon (to lookup some resources).

> s/getIcon/get/ ;)

Argh. While working on this I refactored some get() method into getIcon(). Android Studio went crazy and replaced 'get' in all comments too. I thought I found and reverted all of them. Hmz. :)

	:Grisha Kruglov
	Comment 30 • 8 years ago
mozreview-review

Comment on attachment 8782415 [details]
Bug 1290014 - Add DiskLruCache library.

https://reviewboard.mozilla.org/r/69250/#review71240

Out of curiousity, what do we usually do for keeping these 3rd party libs up-to-date?

	:Grisha Kruglov
	Comment 31 • 8 years ago
mozreview-review

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review71266

::: mobile/android/base/java/org/mozilla/gecko/icons/storage/DiskStorage.java:65
(Diff revision 1)
> +
> +        return instance;
> +    }
> +
> +    private Context context;
> +    private DiskLruCache cache;

If you'll make the suggested changes to ensureCacheIsReady, could add a @GuardedBy annotation here as well

::: mobile/android/base/java/org/mozilla/gecko/icons/storage/DiskStorage.java:71
(Diff revision 1)
> +
> +    private DiskStorage(Context context) {
> +        this.context = context.getApplicationContext();
> +    }
> +
> +    private void ensureCacheIsReady() throws IOException {

Perhaps make this return a cache, synchronize the method, and only access cache object through it?

::: mobile/android/base/java/org/mozilla/gecko/icons/storage/FailureCache.java:33
(Diff revision 1)
> +        }
> +
> +        return instance;
> +    }
> +
> +    private LruCache<String, Long> cache;

This could be final, right?

::: mobile/android/base/java/org/mozilla/gecko/icons/storage/FailureCache.java:42
(Diff revision 1)
> +    }
> +
> +    /**
> +     * Remember this icon URL after loading from it (over the network) has failed.
> +     */
> +    public synchronized void rememberFailure(String iconUrl) {

LruCache is thread-safe, I don't think you need to synchronize here?

::: mobile/android/base/java/org/mozilla/gecko/icons/storage/FailureCache.java:50
(Diff revision 1)
> +
> +    /**
> +     * Has loading from this URL failed previously and recently?
> +     */
> +    public synchronized boolean isKnownFailure(String iconUrl) {
> +        final Long failedAt =  cache.get(iconUrl);

Perhaps synchronize on the `cache` itself to ensure this is get/remove combo is atomic?

::: mobile/android/base/java/org/mozilla/gecko/icons/storage/MemoryStorage.java:49
(Diff revision 1)
> +            this.color = color;
> +        }
> +    }
> +
> +    private final LruCache<String, CacheEntry> iconCache;
> +    private final LruCache<String, String> mappingCache;

I have a feeling you might need to synchronize access to these caches on a lock (this?), otherwise consumers might see inconsistent results (even though individual caches are thread-safe).

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 32 • 8 years ago
mozreview-review-reply

Comment on attachment 8782415 [details]
Bug 1290014 - Add DiskLruCache library.

https://reviewboard.mozilla.org/r/69250/#review71240

Manual updates if needed. There's no special process to keep track (to my knowledge).

* httpclient (ch.boye): Hand picked version (for sync). Should be replaced at some point, but this is a bigger task.
* switchboard: Heavily modified and not really the original switchboard anymore. There are bugs for moving it into the tree and/or creating a standalone library.
* picasso: There's a bug where we tried updating the library but it came with a bunch of new dependencies (mainly okhttp).
* org.json.simple: ¯\_(ツ)_/¯ sync 
* dspec: We should remove this and there might be a bug.
* apache.commons.codec ¯\_(ツ)_/¯

Those things are another good reason why we should switch to a gradle based build and not keep our own dependency zoo.

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 33 • 8 years ago
mozreview-review-reply

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review70304

> (I was wondering whether we could make these final, and then set them in a Constructor, which is called from Builder.build(), but that would add a lot of unnecessary duplication AFAICS.)

Yeah, that's what I was wondering too. In the beginning I wasn't sure if there would be a use case where the page URL is not required. So far it seems we always have and need a page URL (That's why I added the check to build() at the end of the refactoring).

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 34 • 8 years ago
mozreview-review-reply

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review70772

Absolutely. There's still a lot of time left in this cycle and I hope to catch a bunch of (new) errors early.

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 35 • 8 years ago
mozreview-review-reply

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review71266

> If you'll make the suggested changes to ensureCacheIsReady, could add a @GuardedBy annotation here as well

@GuardedBy is from the ch.boye http library. I would like to avoid using it more broadly in the code base. In the past I have been using just plain comments:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentCatalog.java#42-44

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 36 • 8 years ago
mozreview-review-reply

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review71266

> I have a feeling you might need to synchronize access to these caches on a lock (this?), otherwise consumers might see inconsistent results (even though individual caches are thread-safe).

Currently the memory cache is only accessed through a single-threaded executor. But yeah, this does not need to be the case forever and this is not part of the contract of MemoryStorage. I'll synchronize.

	:Grisha Kruglov
	Comment 46 • 8 years ago
mozreview-review

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review71844

::: mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java:156
(Diff revision 2)
> +    /* package-private */ IconCallback getCallback() {
> +        return callback;
> +    }
> +
> +    /* package-private */ boolean hasIconDescriptors() {
> +        return icons.size() > 0;

.isEmpty, I would imagine .size might take longer for a TreeSet

::: mobile/android/base/java/org/mozilla/gecko/icons/IconRequestBuilder.java:17
(Diff revision 2)
> +
> +/**
> + * Builder for creating a request to load an icon.
> + */
> +public class IconRequestBuilder {
> +    private IconRequest request;

final?

::: mobile/android/base/java/org/mozilla/gecko/icons/IconRequestExecutor.java:112
(Diff revision 2)
> +
> +            // Store the icon (and mapping) in the disk cache if needed
> +            new DiskProcessor(),
> +
> +            // Resize the icon to match the target size (if possible)
> +            new ResizingProcessor(),

Wouldn't you want to resize first? Do we need to store full-size icons on disk (if downscaling)? and colorprocess would be potentially a bit faster.

But I'm assuming that we'll be mostly making icons smaller here.

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/IconDownloader.java:42
(Diff revision 2)
> +    private static final int MAX_REDIRECTS_TO_FOLLOW = 5;
> +
> +    /**
> +     * The default size of the buffer to use for downloading Favicons in the event no size is given
> +     * by the server. */
> +    private static final int DEFAULT_FAVICON_BUFFER_SIZE = 25000;

DEFAULT_FAVICON_BUFFER_SIZE_BYTES

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/IconDownloader.java:89
(Diff revision 2)
> +    /**
> +     * Download the Favicon from the given URL and pass it to the decoder function.
> +     *
> +     * @param targetFaviconURL URL of the favicon to download.
> +     * @return A LoadFaviconResult containing the bitmap(s) extracted from the downloaded file, or
> +     *         null if no or corrupt data ware received.

spelling

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/IconDownloader.java:161
(Diff revision 2)
> +
> +            return tryDownloadRecurse(newURI, visited);
> +        }
> +
> +        if (status >= 400) {
> +            // Client or Server error. Let's no retry loading from this URL again for some time

"Let's not"

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/IconDownloader.java:168
(Diff revision 2)
> +
> +            connection.disconnect();
> +            return null;
> +        }
> +
> +        return new Response(connection.getInputStream(), connection.getHeaderFieldInt("Content-Length", -1));

Do you need to ensure connection.disconnect() is called? I don't think you'll call it in case of a 200 response

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/IconDownloader.java:204
(Diff revision 2)
> +        // This may not be provided, but if it is, it's useful.
> +        int bufferSize;
> +        if (response.contentLength > 0) {
> +            // The size was reported and sane, so let's use that.
> +            // Integer overflow should not be a problem for Favicon sizes...
> +            bufferSize = response.contentLength + 1;

What's "+ 1"?

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/JarLoader.java:23
(Diff revision 2)
> + * https://developer.mozilla.org/en-US/docs/Mozilla/About_omni.ja_(formerly_omni.jar)
> + */
> +public class JarLoader implements IconLoader {
> +    private static final String LOGTAG = "Gecko/JarLoader";
> +
> +    @Override

Not sure, do you need to carry over annotations declared in the interface when you're overriding? e.g. @Nullable in this case

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/MemoryLoader.java:16
(Diff revision 2)
> +
> +/**
> + * Loader implementation for loading icons from an in-memory cached (Implemented by MemoryStorage).
> + */
> +public class MemoryLoader implements IconLoader {
> +    private MemoryStorage storage;

final

::: mobile/android/base/java/org/mozilla/gecko/icons/preparation/AboutPagesPreparer.java:32
(Diff revision 2)
> +    }
> +
> +    @Override
> +    public void prepare(IconRequest request) {
> +        if (aboutUrls.contains(request.getPageUrl())) {
> +            final String iconUrl = GeckoJarReader.getJarURL(request.getContext(), "chrome/chrome/content/branding/favicon64.png");

could this fail?

::: mobile/android/base/java/org/mozilla/gecko/icons/processing/MemoryProcessor.java:8
(Diff revision 2)
> +import org.mozilla.gecko.icons.IconRequest;
> +import org.mozilla.gecko.icons.IconResponse;
> +import org.mozilla.gecko.icons.storage.MemoryStorage;
> +
> +public class MemoryProcessor implements Processor {
> +    private MemoryStorage storage;

final

::: mobile/android/base/java/org/mozilla/gecko/icons/processing/MemoryProcessor.java:18
(Diff revision 2)
> +
> +    @Override
> +    public void process(IconRequest request, IconResponse response) {
> +        if (request.shouldSkipMemory() || request.getIconCount() == 0 || response.isGenerated()) {
> +            // Do not cache this icon in memory if we should skip the memory cache or if this icon
> +            // has been generated. We can re-generate it if needed.

How often might we re-generate generated icons? caching won't win us anything here?

::: mobile/android/base/java/org/mozilla/gecko/icons/processing/ResizingProcessor.java:36
(Diff revision 2)
> +
> +        if (size > targetSize) {
> +            resizedBitmap = resize(originalBitmap, targetSize);
> +        } else {
> +            // Our largest primary is smaller than the desired size. Upscale by a maximum of 2x.
> +            // largestSize now reflects the maximum size we can upscale to.

s/largestSize/size

::: mobile/android/base/java/org/mozilla/gecko/icons/processing/ResizingProcessor.java:37
(Diff revision 2)
> +        if (size > targetSize) {
> +            resizedBitmap = resize(originalBitmap, targetSize);
> +        } else {
> +            // Our largest primary is smaller than the desired size. Upscale by a maximum of 2x.
> +            // largestSize now reflects the maximum size we can upscale to.
> +            size *= 2;

or could also declare int largestSize = size * 2; for clarity/immutability :)

::: mobile/android/base/java/org/mozilla/gecko/icons/storage/DiskStorage.java:101
(Diff revision 2)
> +            final String key = createKey(KEY_PREFIX_MAPPING, pageUrl);
> +            if (key == null) {
> +                return;
> +            }
> +
> +            editor = cache.edit(key);

.edit might return null if another edit for the same key is in progress.

::: mobile/android/base/java/org/mozilla/gecko/icons/storage/DiskStorage.java:133
(Diff revision 2)
> +            final String key = createKey(KEY_PREFIX_ICON, iconUrl);
> +            if (key == null) {
> +                return;
> +            }
> +
> +            editor = cache.edit(key);

might be null

::: mobile/android/base/java/org/mozilla/gecko/icons/storage/DiskStorage.java:136
(Diff revision 2)
> +            }
> +
> +            editor = cache.edit(key);
> +
> +            outputStream = editor.newOutputStream(0);
> +            bitmap.compress(Bitmap.CompressFormat.PNG, 100, outputStream);

maybe worth pointing out in a comment that quality setting is actually ignored here (png is lossless).

::: mobile/android/base/java/org/mozilla/gecko/icons/storage/DiskStorage.java:140
(Diff revision 2)
> +            outputStream = editor.newOutputStream(0);
> +            bitmap.compress(Bitmap.CompressFormat.PNG, 100, outputStream);
> +
> +            outputStream.close();
> +
> +            editor.commit();

Do you want to commit if .compress returns false?

::: mobile/android/base/java/org/mozilla/gecko/icons/storage/DiskStorage.java:252
(Diff revision 2)
> +            NativeCrypto.sha256update(ctx, data, data.length);
> +
> +            data = url.getBytes(StandardCharsets.UTF_8);
> +            NativeCrypto.sha256update(ctx, data, data.length);
> +            return Utils.byte2Hex(NativeCrypto.sha256finalize(ctx));
> +        } catch (NoClassDefFoundError | ExceptionInInitializerError error) {

This is java 7+ :-(

	:Grisha Kruglov
	Comment 47 • 8 years ago
mozreview-review

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review71868

This is pretty great!

	:Grisha Kruglov
	Comment 48 • 8 years ago
mozreview-review-reply

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review71844

> Not sure, do you need to carry over annotations declared in the interface when you're overriding? e.g. @Nullable in this case

Tried this locally, and you do need to explicitely mark override methods with any annotations that you mean to carry over.

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 49 • 8 years ago
mozreview-review-reply

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review71844

> Wouldn't you want to resize first? Do we need to store full-size icons on disk (if downscaling)? and colorprocess would be potentially a bit faster.
> 
> But I'm assuming that we'll be mostly making icons smaller here.

There are some consumers that require a larger icon size (launcher icons, media notifications). Those load the larger icon from disk.

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 50 • 8 years ago
mozreview-review-reply

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review71844

> What's "+ 1"?

Good question. This is the current code moved from LoadFaviconTask:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java#347

It looks like the code came in here (bug 748100):
https://hg.mozilla.org/mozilla-central/rev/1c402a47da51

Unfortunately there's no explanation why the "+1" was added. It definitely looks wrong.

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 51 • 8 years ago
mozreview-review-reply

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review71844

> could this fail?

What exactly do you mean? This icon is in the omni.ja (inside the APK) and this is the same code we are running now (just restructured). If just loading this icon would fail then we'd generate a fallback.

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 52 • 8 years ago
mozreview-review-reply

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review71844

> How often might we re-generate generated icons? caching won't win us anything here?

Back when I was writing the generator I was thinking about having a small cache inside the generator; would be interesting to measure. In general I would like to avoid to keep to many of them around and use up precious memory.
I would not like to cache them here though: The next time we try to load an icon we still want to try loading it from the web etc.; we do not want to keep returning a generated one from the cache if there's the opportunity to actually load one.

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 53 • 8 years ago
mozreview-review-reply

Comment on attachment 8782416 [details]
Bug 1290014 - Restructure icon code and use disk lru cache.

https://reviewboard.mozilla.org/r/69252/#review71844

> This is java 7+ :-(

We are using this in multiple places; Afaik the build process takes care of creating backwards compatible bytecode.

	:Grisha Kruglov
	Comment 63 • 8 years ago
mozreview-review

Comment on attachment 8782417 [details]
Bug 1290014 - Use new icon framework in UI code.

https://reviewboard.mozilla.org/r/71956/#review73634

::: mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java:32
(Diff revision 3)
>      /* package-private */ boolean skipNetwork;
>      /* package-private */ boolean backgroundThread;
>      /* package-private */ boolean skipDisk;
>      /* package-private */ boolean skipMemory;
> +    /* package-private */ int targetSize;
> +    /* package-private */ boolean prepareOnly;

I think you forgot to set the default value in the constructor? I only see it being set to "true" in `prepareOnly`, and never to "false" (default).

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java:149
(Diff revision 3)
>                      long date = cursor.getLong(dateCol);
>                      int visits = cursor.getInt(visitsCol);
>                      byte[] data = cursor.getBlob(faviconCol);
>                      mDB.updateHistoryInBatch(mCr, mOperations, url, title, date, visits);
>                      if (data != null) {
> -                        mDB.updateFaviconInBatch(mCr, mOperations, url, null, null, data);
> +                        storeBitmap(data, url);

Not quite the same as the original - operation won't be performed in a batch along with processing the rest of the history information (I'm not sure this was even necessary in the first place...). But, since mOperations is a collection of ContentProviderOperation, neither can you do this easily without some restructuring of this code.

So this is likely to be just fine, and AndroidImport is deprecated starting with KitKat anyway.

::: mobile/android/base/java/org/mozilla/gecko/reader/ReadingListHelper.java:84
(Diff revision 3)
> +                // to strip the about:reader URL and perform a normal icon load from cache.
> +                //
> +                // A more global solution (looking at desktop and iOS) would be to copy the <link>
> +                // markup from the original page to the about:reader page and then rely on our normal
> +                // icon loading code. This would work even if we do not have anything in the cache
> +                // for some kind of reason.

It is a strange run-around. Do you want to file a bug to address this at some point? Sounds like it might be a good potential next bug, if provided with enough detail.

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabHistoryItemRow.java:60
(Diff revision 3)
> -            if (v == null) {
> -                return;
> -            }
>  
> -            if (favicon == null) {
> -                v.showDefaultFavicon(url);
> +        if (ongoingIconLoad != null) {
> +            ongoingIconLoad.cancel(true);

Out of curiosity...

When we have these ongoing favicon load tasks, and we cancel them - especially cancel(true), (our top sites grid changed and views got all reshuffled, or whatever else might happen) - we might have gotten quite far in our pipeline. 

The next time IconTask is executed for the same icon url and builder params (likely, very soon after we called cancel, since our view data probably didn't change too much), it's possible for it to "re-use" results of the previous (cancelled) pipeline run. Specifically, the first step in the pipelne is a lookup in memory and disk caches - however, we only put results into the caches (i think) at the last stage ("processors"). Is it possible to move that up a bit in the pipeline, to increase chance of the cache mappings being created? Perhaps at least before Color and Resizing processors?

I'm not sure if I'm entirely off-base here, or if this is a micro-optimization of sorts or if might make some difference.

	:Grisha Kruglov
	Comment 64 • 8 years ago
mozreview-review

Comment on attachment 8782418 [details]
Bug 1290014 - Remove obsolete code.

https://reviewboard.mozilla.org/r/71958/#review73644

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:70
(Diff revision 3)
> -import static org.mozilla.gecko.favicons.LoadFaviconTask.DEFAULT_FAVICON_BUFFER_SIZE;
>  
>  public class LocalBrowserDB implements BrowserDB {
> +    // The default size of the buffer to use for downloading Favicons in the event no size is given
> +    // by the server.
> +    public static final int DEFAULT_FAVICON_BUFFER_SIZE = 25000;

DEFAULT_FAVICON_BUFFER_SIZE_BYTES

::: mobile/android/base/java/org/mozilla/gecko/favicons/decoders/LoadFaviconResult.java:111
(Diff revision 3)
> +    /**
> +     * Select the closest icon size from a list of icon sizes.
> +     * We just find the first icon that is larger than the preferred size if available, or otherwise select the
> +     * largest icon (if all icons are smaller than the preferred size).
> +     *
> +     * @return The closes icon size, or -1 if no sizes are supplied.

s/closes/closest

::: mobile/android/base/java/org/mozilla/gecko/favicons/decoders/LoadFaviconResult.java:116
(Diff revision 3)
> +     * @return The closes icon size, or -1 if no sizes are supplied.
> +     */
> +    public static int selectBestSizeFromList(final List<Integer> sizes, final int preferredSize) {
> +        Collections.sort(sizes);
> +
> +        for (int size : sizes) {

Since you're sorting the list, could also try the last item first, see if it's smaller than your preferredSize, and return that. It'll save you having to run through the for loop if all icons are smaller.

::: mobile/android/base/java/org/mozilla/gecko/favicons/decoders/LoadFaviconResult.java:128
(Diff revision 3)
> +        // selected yet, therefore just take the largest (last) icon.
> +        if (sizes.size() > 0) {
> +            return sizes.get(sizes.size() - 1);
> +        } else {
> +            // This isn't ideal, however current code assumes this as an error value for now.
> +            return -1;

Could return -1 at the very top if sizes.isEmpty()

	:Grisha Kruglov
	Comment 65 • 8 years ago
mozreview-review

Comment on attachment 8782419 [details]
Bug 1290014 - Move decoders from the favicons to the icons package.

https://reviewboard.mozilla.org/r/72320/#review73654

	:Grisha Kruglov
	Comment 66 • 8 years ago
mozreview-review

Comment on attachment 8782420 [details]
Bug 1290014 - Move code from the FaviconGenerator class to the IconGenerator in the icons package.

https://reviewboard.mozilla.org/r/72322/#review73656

Skimmed through, assuming nothing changed during the move.

	:Grisha Kruglov
	Comment 67 • 8 years ago
mozreview-review

Comment on attachment 8782421 [details]
Bug 1290014 - Add additional unit tests.

https://reviewboard.mozilla.org/r/72598/#review73658

mockito is great!

	:Grisha Kruglov
	Comment 68 • 8 years ago
mozreview-review

Comment on attachment 8782507 [details]
Bug 1290014 - ResizingProcessor: Do not resize icons loaded from memory.

https://reviewboard.mozilla.org/r/72648/#review73662

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 79 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af9128a11a59

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 89 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbac38da4e35

	Andrzej Hunt :ahunt
	Comment 90 • 8 years ago
mozreview-review

Comment on attachment 8787212 [details]
Bug 1290014 - Refactor activity stream top sites to use new icon API.

https://reviewboard.mozilla.org/r/76082/#review74030

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 101 • 8 years ago
mozreview-review-reply

Comment on attachment 8782417 [details]
Bug 1290014 - Use new icon framework in UI code.

https://reviewboard.mozilla.org/r/71956/#review73634

> Out of curiosity...
> 
> When we have these ongoing favicon load tasks, and we cancel them - especially cancel(true), (our top sites grid changed and views got all reshuffled, or whatever else might happen) - we might have gotten quite far in our pipeline. 
> 
> The next time IconTask is executed for the same icon url and builder params (likely, very soon after we called cancel, since our view data probably didn't change too much), it's possible for it to "re-use" results of the previous (cancelled) pipeline run. Specifically, the first step in the pipelne is a lookup in memory and disk caches - however, we only put results into the caches (i think) at the last stage ("processors"). Is it possible to move that up a bit in the pipeline, to increase chance of the cache mappings being created? Perhaps at least before Color and Resizing processors?
> 
> I'm not sure if I'm entirely off-base here, or if this is a micro-optimization of sorts or if might make some difference.

This is something I have been thinking about too. I looked at the old code and this one seemed to stop all work immediately too. So this is at least not worst. As long as we are only using a single thread executor this seems to be a sane approach. However there's the possibility to optimize this, more threads and maybe a different order of tasks.

	Pulsebot
	Comment 102 • 8 years ago

Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1e8ebfb60a17
Add DiskLruCache library. r=ahunt,Grisha
https://hg.mozilla.org/integration/autoland/rev/7d65390d95e7
Restructure icon code and use disk lru cache. r=ahunt,Grisha
https://hg.mozilla.org/integration/autoland/rev/a803f062653e
Use new icon framework in UI code. r=ahunt,Grisha
https://hg.mozilla.org/integration/autoland/rev/a73dcd57f417
Remove obsolete code. r=ahunt,Grisha
https://hg.mozilla.org/integration/autoland/rev/c588732a368e
Move decoders from the favicons to the icons package. r=ahunt,Grisha
https://hg.mozilla.org/integration/autoland/rev/9b852e0472ff
Move code from the FaviconGenerator class to the IconGenerator in the icons package. r=ahunt,Grisha
https://hg.mozilla.org/integration/autoland/rev/a87ed908d84b
Add additional unit tests. r=ahunt,Grisha
https://hg.mozilla.org/integration/autoland/rev/f11ee12c87bb
ResizingProcessor: Do not resize icons loaded from memory. r=ahunt,Grisha
https://hg.mozilla.org/integration/autoland/rev/ef83714d1226
DiskStorage: Do not use StandardCharsets.UTF_8. It's only support on API level 19+. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/38d0defa2db7
Refactor activity stream top sites to use new icon API. r=ahunt

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 103 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1e8ebfb60a17
https://hg.mozilla.org/mozilla-central/rev/7d65390d95e7
https://hg.mozilla.org/mozilla-central/rev/a803f062653e
https://hg.mozilla.org/mozilla-central/rev/a73dcd57f417
https://hg.mozilla.org/mozilla-central/rev/c588732a368e
https://hg.mozilla.org/mozilla-central/rev/9b852e0472ff
https://hg.mozilla.org/mozilla-central/rev/a87ed908d84b
https://hg.mozilla.org/mozilla-central/rev/f11ee12c87bb
https://hg.mozilla.org/mozilla-central/rev/ef83714d1226
https://hg.mozilla.org/mozilla-central/rev/38d0defa2db7

	:Grisha Kruglov
	Comment 104 • 8 years ago

Seeing this during startup:

09-06 14:40:28.383 25083-25396/org.mozilla.fennec_grisha D/GeckoTabs: handleMessage: Link:Favicon
09-06 14:40:28.384 25083-25396/org.mozilla.fennec_grisha W/GeckoTabs: handleMessage threw for Link:Favicon
                                                                      java.lang.NullPointerException: Attempt to invoke virtual method 'org.mozilla.gecko.icons.IconRequestBuilder org.mozilla.gecko.icons.IconRequestBuilder.icon(org.mozilla.gecko.icons.IconDescriptor)' on a null object reference
                                                                          at org.mozilla.gecko.Tab.addFavicon(Tab.java:439)
                                                                          at org.mozilla.gecko.Tabs.handleMessage(Tabs.java:525)
                                                                          at org.mozilla.gecko.EventDispatcher.dispatchEvent(EventDispatcher.java:411)
                                                                          at org.mozilla.gecko.EventDispatcher.dispatchEvent(EventDispatcher.java:246)
                                                                          at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:1971)
                                                                          at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
                                                                          at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:525)

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 105 • 8 years ago

I can't see this on crash-stats but I filed bug 1301031.