Closed
Bug 887492
Opened 11 years ago
Closed 11 years ago
Favicons should have static methods and not be a singleton
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: sriram, Assigned: ckitching)
References
Details
Attachments
(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.
Reporter | ||
Comment 1•11 years ago
|
||
Attachment #767995 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 2•11 years ago
|
||
This is for fig. Based on how this holds up, we could move this central, if we want.
Reporter | ||
Comment 3•11 years ago
|
||
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.
Reporter | ||
Comment 4•11 years ago
|
||
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 5•11 years ago
|
||
Comment on attachment 767995 [details] [diff] [review] Patch 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/Favicons.java @@ -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: https://developer.android.com/reference/android/net/http/AndroidHttpClient.html#close%28%29 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 6•11 years ago
|
||
Comment on attachment 768017 [details] [diff] [review] Part 2: Context Review of attachment 768017 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoApp.java @@ +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?
Updated•11 years ago
|
Attachment #768017 -
Flags: review?(bnicholson) → review-
Assignee | ||
Comment 7•11 years ago
|
||
Addressed by Part of of patch series for Bug 888326.
Assignee: nobody → ckitching
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•11 years ago
|
||
Fixed by: https://hg.mozilla.org/integration/fx-team/rev/26e40024ee85
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 9•11 years ago
|
||
(By which I of course mean: https://hg.mozilla.org/mozilla-central/rev/26e40024ee85 )
Updated•11 years ago
|
Target Milestone: --- → Firefox 26
Assignee | ||
Updated•11 years ago
|
Attachment #767995 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #768017 -
Attachment is obsolete: true
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•