Closed
Bug 1057247
Opened 9 years ago
Closed 9 years ago
Increase favicon retry time
Categories
(Firefox for Android Graveyard :: Favicon Handling, defect)
Tracking
(firefox33 verified, firefox34 verified, firefox35 verified)
VERIFIED
FIXED
Firefox 35
People
(Reporter: m_kato, Assigned: rnewman)
References
Details
Attachments
(1 file)
3.18 KB,
patch
|
mfinkle
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Now, retry failed favicon's time (FAILURE_RETRY_MILLISECONDS) is hard-coded as 20 min. A carrier requests us this timer becomes customizable by preference (about:config).
Reporter | ||
Comment 1•9 years ago
|
||
s/carrier/mobile operator/
Assignee | ||
Comment 2•9 years ago
|
||
Pref customizability here would be non-trivial, but I'd be very open to bumping this value to one much higher. What value does the carrier favor?
Hardware: ARM → All
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(m_kato)
Comment 3•9 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #2) > Pref customizability here would be non-trivial, but I'd be very open to > bumping this value to one much higher. What value does the carrier favor? How hard would it be to add a sue of PrefsHelper into the code? http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/PrefsHelper.java
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #3) > How hard would it be to add a use of PrefsHelper into the code? Not hard, but it would couple the favicon cache to Gecko via an event listener, which I'm not keen to do without a strong motivation. If we were going that route, I'd try to sculpt a simple sibling of PrefsHelper that didn't incur a round-trip, taking an approach similar to the one we're taking for proxy handling: cache the value on the Java side, and update it when it changes in Gecko. We would benefit from that work in several places. In this case I don't think it's particularly necessary: the 20-minute default is arbitrary, we really have no idea if it offers value in the real world, and even if it does, I'm not super concerned if we don't retry a failed fetch for an hour (or six) instead of 20 minutes.
Reporter | ||
Comment 5•9 years ago
|
||
I am asking this mobile operator what number is better...
Flags: needinfo?(m_kato)
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(m_kato)
Reporter | ||
Comment 6•9 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #4) > (In reply to Mark Finkle (:mfinkle) from comment #3) > > > How hard would it be to add a use of PrefsHelper into the code? > > Not hard, but it would couple the favicon cache to Gecko via an event > listener, which I'm not keen to do without a strong motivation. > > If we were going that route, I'd try to sculpt a simple sibling of > PrefsHelper that didn't incur a round-trip, taking an approach similar to > the one we're taking for proxy handling: cache the value on the Java side, > and update it when it changes in Gecko. We would benefit from that work in > several places. > > In this case I don't think it's particularly necessary: the 20-minute > default is arbitrary, we really have no idea if it offers value in the real > world, and even if it does, I'm not super concerned if we don't retry a > failed fetch for an hour (or six) instead of 20 minutes. I talk with vendor today. They are happy to change to 1 hour. Could you change to 1 hour?
Flags: needinfo?(m_kato)
Assignee | ||
Comment 7•9 years ago
|
||
Code change is trivial, but signoff always worthwhile.
Attachment #8490805 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Comment 8•9 years ago
|
||
Comment on attachment 8490805 [details] [diff] [review] Increase favicon refetch time to one hour. >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java >+ Log.i("GeckoXXX", "storeAndNotify: " + osLocale); >- >+ Log.i("GeckoXXX", "storeAndNotify: last = " + lastOSLocale); >+ Log.i("GeckoXXX", "storeAndNotify: tag = " + osLanguageTag); Remove these (Probably part of another patch) >diff --git a/mobile/android/base/favicons/cache/FaviconCache.java b/mobile/android/base/favicons/cache/FaviconCache.java >+ // Retry failed favicons after one hour. >+ public static final long FAILURE_RETRY_MILLISECONDS = 1000 * 60 * 60; Did we ever apply science to the retry? If it was arbitrary, why not go higher? 4 hours?
Attachment #8490805 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 9•9 years ago
|
||
Heh, I knew I should have made sure I had qreffed first. No, we didn't apply science. Yes, I'd be happy to go higher.
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/7c528e5b0f36
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8490805 [details] [diff] [review] Increase favicon refetch time to one hour. Approval Request Comment [Feature/regressing bug #]: Old design decision. [User impact if declined]: Fennec will re-fetch failed favicons every 20 minutes. Partner request. [Describe test coverage new/current, TBPL]: None. [Risks and why]: Zero risk. Changes a constant. [String/UUID change made/needed]: None.
Attachment #8490805 -
Flags: approval-mozilla-beta?
Attachment #8490805 -
Flags: approval-mozilla-aurora?
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7c528e5b0f36
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Updated•9 years ago
|
Updated•9 years ago
|
Attachment #8490805 -
Flags: approval-mozilla-beta?
Attachment #8490805 -
Flags: approval-mozilla-beta+
Attachment #8490805 -
Flags: approval-mozilla-aurora?
Attachment #8490805 -
Flags: approval-mozilla-aurora+
Comment 13•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/a9518affef0e https://hg.mozilla.org/releases/mozilla-beta/rev/515fa121e700
Comment 14•9 years ago
|
||
I see the change in the code. As I understand we did not implement a new pref ( as seen in the patch too) so I will set this to VERIFIED FIXED.
Status: RESOLVED → VERIFIED
Assignee | ||
Updated•9 years ago
|
Summary: Customizable favicon retry time → Increase favicon retry time
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
•