1276050 - Remove duplicate of loadUncachedFavicon

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Favicon Handling, defect)

Description

[Andrzej Hunt :ahunt]

Favicons.getSizedFaviconForPageFromLocal contains a near copy of loadUncachedFavicon, we can remove that duplicated code and just call loadUncachedFavicon.

Comment 1

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1276050 - Remove duplicate code from Favicons r?sebastian

loadUncachedFavicon does exactly the same thing.

Review commit: https://reviewboard.mozilla.org/r/55552/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55552/

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8757039 [details]
MozReview Request: Bug 1276050 - Remove duplicate code from Favicons r?sebastian

https://reviewboard.mozilla.org/r/55552/#review52876

::: mobile/android/base/java/org/mozilla/gecko/favicons/Favicons.java:296
(Diff revision 1)
>                  return dispatchResult(pageURL, targetURL, result, callback);
>              }
>          }
>  
>          // No joy using in-memory resources. Go to background thread and ask the database.
> -        final LoadFaviconTask task =
> +        return loadUncachedFavicon(context, pageURL, targetURL, 0, targetSize, callback);

loadUncachedFavicon() additionally checks whether the pageURL is not null. Is it okay for all callers of getSizedFaviconForPageFromLocal() if we do this additional check now?

Comment 3

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/55552/#review52876

> loadUncachedFavicon() additionally checks whether the pageURL is not null. Is it okay for all callers of getSizedFaviconForPageFromLocal() if we do this additional check now?

Hmm, it looks like we might run into issues. It would seem illogical to call getSizedFaviconForPageFromLocal() with a null pageURL, but my understanding of current behaviour is:
- LoadFaviconTask is called with pageURL=null, faviconURL=null
- Fail to determing URL in LoadFaviconTask at Favicons.getFaviconURLForPageURLFromCache()
- Fail to determine URL in LoadFaviconsTask at Favicons.getFaviconURLForPageURL()
- Get null from Favicons.guessDefaultFaviconURL() too
- Return null from LoadFaviconTask.doInBackground()
- callback receives null Bitmap

New behaviour:
- loadUncachedFavicon returns NOT_LOADING

The various listeners involved are in:
- BrowserApp: does nothing if null Bitmap is received, ignores return value -> behaviour is unchanged
- TwoLinePageRow: shows default favicon if null Bitmap is received, uses return value only to cancel loading -> won't update to the default generated favicon if null pageURL is passed in
- TopSitesPanel: tries to show the bitmap either way, even if null. Ignores return value. -> we won't update the icons if we pass in a null pageURL
- Seems to be the same as TwoLinePageRow

I'm not sure in what cases we'd actually pass in a null pageURL: most of these cases should have a URL, but it's possible we could receive empty URLs from the DB in various special cases. (The UI blocks us from setting empty URLs for bookmarks, all other items should have a URL, but bugs or sync could push in bad data I guess.)

I'll hold off with landing for now: these are all edge cases. Most likely we'd hit them when updating items in existing lists (e.g. TwoLinePageRow: if bookmarks get reloaded, and a given displayed TwoLinePageRow receives an empty URL, it will probably retain its old favicon instead of going blank).

Comment 4

[Andrzej Hunt :ahunt]

Ignore the above comment. Everything is exactly the same as before: in the case of an empty URL, loadUncachedFavicon calls dispatchResult(null...), which results in onFaviconLoaded being called with a null bitmap. The behaviour is therefore essentially unchanged.

In fact this behaviour is better: previously we'd start a LoadFaviconTask, resulting in messaging from UI->background->UI, just to return a null Favicon for an empty URL. Now we return early with a null Favicon for an empty URL.

Comment 5

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/9575e608cabe6f7ed7cf73475558ddbe9938bf9b
Bug 1276050 - Remove duplicate code from Favicons r=sebastian

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9575e608cabe