Closed Bug 1396941 Opened 7 years ago Closed 7 years ago

Distribution suggested sites icons are placeholders until second run

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

All
Android
defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: mcomella, Assigned: sebastian)

References

Details

(Whiteboard: [mobileAS])

Attachments

(1 file)

As discovered in bug 1394641, when you first open the app, placeholder icons are shown for the distribution suggested sites:

https://bug1394641.bmoattachments.org/attachment.cgi?id=8904654

When you restart the app, the expected icons are shown:

https://bug1394641.bmoattachments.org/attachment.cgi?id=8904658

I confirmed with mkaply via IRC that this is a new issue with AS.

---

Adding to the current sprint as a breakout from bug 1394641.
Not actively working on this right now (but might be soon).
Assignee: michael.l.comella → nobody
Note: I've only tested this with OTA distributions.

Another observation: some distributions show other favicons, which are replaced by the expected suggested sites on second run.

---

Sebastian, I think you rewrote the favicon loading code and its integration with AS – do you know what might be going on here? Note that distribution suggested sites configuration files and the icons are being downloaded in the background so perhaps the icons aren't ready yet. Here's more background on suggested sites for distributions: https://wiki.mozilla.org/Mobile/Distribution_Files#Suggested_sites
Flags: needinfo?(s.kaspari)
I'll investigate. Might be related to bug 1391397.
Flags: needinfo?(s.kaspari)
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Iteration: 1.29 → 1.30
For APK distributions I see the icons being loaded but the color not being available on the first start.
I see at least a race between loading the distribution for the first time and the local cache in SuggestedSitePreparer.
Comment on attachment 8907725 [details]
Bug 1396941 - SuggestedSitePreparer: Update local cache if suggested sites change.

https://reviewboard.mozilla.org/r/179396/#review184688

This looks like it'll fix the local cache such that subsequent loads of the top sites will fix the icons, but if Top Sites gets the generated icons first, will it refresh itself for the newly updated icons? r+ if so since I think we want this to be the case.

This patch was a little confusing because of the method names and that I wasn't sure what was being synchronized and why: I added issues.

::: mobile/android/base/java/org/mozilla/gecko/icons/preparation/SuggestedSitePreparer.java:46
(Diff revision 1)
> +                        performSynchronizationOnBackgroundThread(context);
> +                    }
> +                });
> +    }
> +
> +    private void performSynchronizationOnBackgroundThread(final Context context) {

nit: What's a "synchronization"? I think this be `performRefreshSiteFaviconMapOnBackgroundThread`, as per my comment below.

::: mobile/android/base/java/org/mozilla/gecko/icons/preparation/SuggestedSitePreparer.java:60
(Diff revision 1)
>      // Loading suggested sites (and iterating over them) is potentially slow. The number of suggested
>      // sites is low, and a HashSet containing them is therefore likely to be exceedingly small.
>      // Hence we opt to iterate over the list once, and do an immediate lookup every time a favicon
>      // is requested:
>      // Loading can fail if suggested sites haven't been initialised yet, only proceed if we return true.
> -    private boolean initialise(final Context context) {
> +    private synchronized boolean performInitialisation(Context context) {

nit: I find it confusing there's an `initialise` and a `performInitialisation` - what's the difference? I think `initialise` and `refreshSiteFaviconMap` might be clearer.

::: mobile/android/base/java/org/mozilla/gecko/icons/preparation/SuggestedSitePreparer.java:60
(Diff revision 1)
>      // Loading suggested sites (and iterating over them) is potentially slow. The number of suggested
>      // sites is low, and a HashSet containing them is therefore likely to be exceedingly small.
>      // Hence we opt to iterate over the list once, and do an immediate lookup every time a favicon
>      // is requested:
>      // Loading can fail if suggested sites haven't been initialised yet, only proceed if we return true.
> -    private boolean initialise(final Context context) {
> +    private synchronized boolean performInitialisation(Context context) {

nit: I assume you're synchronizing over accesses to siteFaviconMap - let's add a comment to that effect. (on `siteFaviconMap`?).
Attachment #8907725 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8907725 [details]
Bug 1396941 - SuggestedSitePreparer: Update local cache if suggested sites change.

https://reviewboard.mozilla.org/r/179396/#review184688

> This looks like it'll fix the local cache such that subsequent loads of the top sites will fix the icons, but if Top Sites gets the generated icons first, will it refresh itself for the newly updated icons?

To be explicit, I'm concerned that there could be a race such that this patch will work sometimes (we get the generated favicons, we get the distribution favicons, and then top sites loads the icons) but not others (we get the generated favicons, top sites loads the icons, we get the distribution favicons).

However, in my testing, this appears to always work so perhaps the case I'm concerned about above won't happen.
(In reply to Michael Comella (:mcomella) from comment #7)
> This looks like it'll fix the local cache such that subsequent loads of the
> top sites will fix the icons, but if Top Sites gets the generated icons
> first, will it refresh itself for the newly updated icons? r+ if so since I
> think we want this to be the case.

There are two things happening here why I think this should always work:
* Top sites already reload when suggested sites change / distributions are loaded
* SuggestedSitePreparer injects a new URL (with higher priority) for the suggested site's icon - because of that a potentially previously generated and cached icon won't be re-used. We try to load the new one first.
Comment on attachment 8907725 [details]
Bug 1396941 - SuggestedSitePreparer: Update local cache if suggested sites change.

https://reviewboard.mozilla.org/r/179396/#review184902

::: mobile/android/base/java/org/mozilla/gecko/icons/preparation/SuggestedSitePreparer.java:46
(Diff revision 1)
> +                        performSynchronizationOnBackgroundThread(context);
> +                    }
> +                });
> +    }
> +
> +    private void performSynchronizationOnBackgroundThread(final Context context) {

Ouch, this was me being confused while refactoring the names - This should have been performInitialisationOnBackgroundThread(). :)

::: mobile/android/base/java/org/mozilla/gecko/icons/preparation/SuggestedSitePreparer.java:60
(Diff revision 1)
>      // Loading suggested sites (and iterating over them) is potentially slow. The number of suggested
>      // sites is low, and a HashSet containing them is therefore likely to be exceedingly small.
>      // Hence we opt to iterate over the list once, and do an immediate lookup every time a favicon
>      // is requested:
>      // Loading can fail if suggested sites haven't been initialised yet, only proceed if we return true.
> -    private boolean initialise(final Context context) {
> +    private synchronized boolean performInitialisation(Context context) {

The code to update the boolean and hashset now can run on different threads (icon loader thread vs. background thread). I'm synchronizing here so that it is guaranteed that the data/state is visible to both threads.
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/163de8db41e8
SuggestedSitePreparer: Update local cache if suggested sites change. r=mcomella
https://hg.mozilla.org/mozilla-central/rev/163de8db41e8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.