Closed Bug 887492 Opened 9 years ago Closed 9 years ago

Favicons should have static methods and not be a singleton


(Firefox for Android Graveyard :: General, defect)

Not set


(Not tracked)

Firefox 26


(Reporter: sriram, Assigned: ckitching)




(2 obsolete files)

Most methods inside Favicons can be made static. And, Favicons is a singleton too. It would be better to make the members static and make the class static methods.
Attached patch Patch (obsolete) — Splinter Review
Attachment #767995 - Flags: review?(margaret.leibovic)
This is for fig. Based on how this holds up, we could move this central, if we want.
The context used by Favicons is from BrowserApp. This would be attached everytime BrowserApp is re-created. Should we move that piece to GeckoApplication? So that the context is ApplicationContext.
Attached patch Part 2: Context (obsolete) — Splinter Review
This patch makes the caller pass in a context to work with. This way Favicons doesn't have to work about contexts.
Attachment #768017 - Flags: review?(bnicholson)
Comment on attachment 767995 [details] [diff] [review]

Review of attachment 767995 [details] [diff] [review]:

This looks good, but let's get a follow-up bug (or another patch here) to fix this HttpClient memory issue.

::: mobile/android/base/
@@ -163,5 @@
>      }
> -    public void clearFailedCache() {
> -        mFailedCache.evictAll();
> -    }

Nice catch, I see this isn't used anywhere.

@@ -194,5 @@
> -                cancelFaviconLoad(taskId);
> -            }
> -        }
> -        if (mHttpClient != null)
> -            mHttpClient.close();

I'm worried about us never closing the HttpClient anymore:

Instead of holding onto a static reference, I think we should make a helper method that gets an HttpClient instance, executes the request, then closes the instance. Or we could just do exactly this inline in downloadFavicon, since that's the only place we use the sHttpClient anyway.

I actually don't even see us calling this close method anywhere currently, though, so we can do this in a follow-up patch. It might be a memory win.
Attachment #767995 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 768017 [details] [diff] [review]
Part 2: Context

Review of attachment 768017 [details] [diff] [review]:

::: mobile/android/base/
@@ +1125,5 @@
>          ThreadUtils.setUiThread(Thread.currentThread(), new Handler());
>          Tabs.getInstance().attachToActivity(this);
> +        Favicons.setFaviconSizes(getResources().getDimensionPixelSize(R.dimen.favicon_size_small),
> +                                 getResources().getDimensionPixelSize(R.dimen.favicon_size_large));

This really isn't an improvement over attachToContext() in that we still require external code to initialize favicons. Could you instead create two methods in Favicons, maybe getSmallFaviconSize() and getLargeFaviconSize(), that initialize these values lazily?
Attachment #768017 - Flags: review?(bnicholson) → review-
Addressed by Part of of patch series for Bug 888326.
Assignee: nobody → ckitching
Depends on: 888326
Fixed by:
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Attachment #767995 - Attachment is obsolete: true
Attachment #768017 - Attachment is obsolete: true
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.