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)
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.
Reporter | ||
Updated•7 years ago
|
Blocks: as-android-distributions
Reporter | ||
Comment 1•7 years ago
|
||
Not actively working on this right now (but might be soon).
Assignee: michael.l.comella → nobody
Reporter | ||
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
I'll investigate. Might be related to bug 1391397.
Flags: needinfo?(s.kaspari)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Updated•7 years ago
|
Iteration: 1.29 → 1.30
Assignee | ||
Comment 4•7 years ago
|
||
For APK distributions I see the icons being loaded but the color not being available on the first start.
Assignee | ||
Comment 5•7 years ago
|
||
I see at least a race between loading the distribution for the first time and the local cache in SuggestedSitePreparer.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 7•7 years ago
|
||
mozreview-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? 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+
Reporter | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 9•7 years ago
|
||
(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.
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review |
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.
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
Pushed by s.kaspari@gmail.com: https://hg.mozilla.org/integration/autoland/rev/163de8db41e8 SuggestedSitePreparer: Update local cache if suggested sites change. r=mcomella
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/163de8db41e8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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
•