1057247 - Increase favicon retry time

Status: VERIFIED FIXED

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

Description

[Makoto Kato [:m_kato]]

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).

Comment 1

[Makoto Kato [:m_kato]]

s/carrier/mobile operator/

Comment 2

[Richard Newman [:rnewman]]

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?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 4

[Richard Newman [:rnewman]]

(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.

Comment 5

[Makoto Kato [:m_kato]]

I am asking this mobile operator what number is better...

Comment 6

[Makoto Kato [:m_kato]]

(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?

Comment 7

[Richard Newman [:rnewman]]

Attachment: Increase favicon refetch time to one hour.

Code change is trivial, but signoff always worthwhile.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 9

[Richard Newman [:rnewman]]

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.

Comment 10

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/7c528e5b0f36

Comment 11

[Richard Newman [:rnewman]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c528e5b0f36

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a9518affef0e
https://hg.mozilla.org/releases/mozilla-beta/rev/515fa121e700

Comment 14

[Ioana Chiorean]

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.