Closed Bug 1388396 Opened 2 years ago Closed 2 years ago

Activity Stream: when have no good favicon, display letter from subdomain over stable color

Categories

(Firefox for Android :: General, enhancement, P1)

All
Android
enhancement

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
1.30
Tracking Status
firefox57 --- verified

People

(Reporter: sebastian, Assigned: mcomella)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [MobileAS])

Attachments

(9 files, 1 obsolete file)

488.55 KB, image/png
Details
213.11 KB, image/png
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
205.00 KB, image/png
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Attached image tiny_favicons.png
Some sites only give us tiny favicons. Currently we just center them in the top sites tile and use an extracted background color (see attached screenshot). This doesn't look nice.

What else could we do here? We already generate "nicer" icons if the website doesn't provide one - should we generate one too when the icon is too small?
bbell mentioned that if we can't get a high-res enough icon, we can display screenshots (see also bug 1389259, which is screenshots for a different purpose).
I wonder if we could still run into situations where we do not have screenshots and just a tiny favicon?
(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> I wonder if we could still run into situations where we do not have
> screenshots and just a tiny favicon?

Almost certainly. We drop screenshots when we try to reduce storage and memory pressure, they can fail to be captured, and so on.
Spoke with bbell: we should:

1) Show a screenshot if possible (related: bug 1389259)
2) Show a letter from the subdomain, if available; other domain & surround it with a stable color so that if the domain/subdomain appears in the list multiple times, they'll have the same color (it doesn't have to extract a color from the tiny favicon, especially because it can be ugly; we probably have the code to do all this already)

As Sebastian mentions in comment 5, it's possible we'll have no screenshot so we'll always need #2 so I think we should start there and maybe add screenshots later as an optimization: repurposing this bug as such.
Summary: Activity Stream: How to handle tiny favicons? → Activity Stream: when have no good favicon, display letter from subdomain over stable color
We'll handle screenshots in bug 1389259.
We load icons in IconTask.loadIcons. When all of the loaders fail, we call IconGenerator.generate [1].

To fix this bug, we'll need to add a branch such that if the loaded icon is too small, we fall back onto IconGenerator.generate). I assume we want to fall back to generating (rather than running the other loaders) to avoid the case where we hit the cache, find the icon is too small, download a new image, it's the same icon (because there are very low chances it'll be larger), and we end up hitting the network each time we want to see this image even though, again, the icon is almost certainly the same.

We may want to add a configuration option for bailing out for small icons, in IconRequestBuilder.

[1]: https://dxr.mozilla.org/mozilla-central/rev/a9d372645a32b8d23d44244f351639af9d73b96a/mobile/android/base/java/org/mozilla/gecko/icons/IconTask.java#140
Please feel free to take this bug - I'm going to work on bug 1391422 because it's a regression from my previous changes.
Assignee: nobody → michael.l.comella
Some notes:
- In IconTask, we process all favicons closer to the desired size (favicon_bg, currently 112dp). This occurs *after* we load the icon (which means the second time we load the icon, the icon we load will be larger - tricky!).
- If increasing in size, we'll only resize a max of 2x
- In bug 1388379 comment 9, I determined the max size of favicon we show is 90dp, based on the top sites tile size

Sebastian, do you know why there's a discrepancy between the top sites tile size (90dp) and the size we generate our favicons to (112dp)?
Flags: needinfo?(s.kaspari)
Attached image Sample small favicons (yahoo, japan) (obsolete) —
More notes:
- The width and height values of the Bitmap in the IconResponse is in pixel values
- We'll need an algorithm to determine when an icon is too small. Since our top sites items are dynamically sized and icon's size will change based on the device size, we may wish to set our cutoff value as a percentage of the actual container size rather than an absolute value.
- We annoying thing: the highlights and top sites containers are different sizes but we'll want to make sure the icons are consistently used between them

---

Aaron, Bryan: do you have an opinion on how we might determine when top sites/highlight icon is too small such that we want to replace it with either a screenshot (bug 1389259) or a generated favicon?

I've attached a screenshot of some small favicons for a reference point. In my opinion, in the screenshot, I think we should replace waitbutwhy (highlights) and keep all the others but I could go either way on Yahoo. For my opinion, that means we would replace icons that are roughly less than 1/3 the width of the top sites/highlight container.
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
Attachment #8900537 - Attachment is obsolete: true
I previously attached the wrong screenshot.
A naive approach might be to ignore an IconResponse object in IconTask.loadIcon() if it's considerably smaller than IconRequest.targetSize (as you mentioned upscaling happens later - so we should take this into account). Whether we want to do this or not could be a configuration detailed set in IconRequest. I guess we do not want to do this everywhere in our app.
Rank: 1
Spoke with bbell on vidyo: we want to have the favicons always fill the top sites tiles and thus no longer want to center favicons with a colored margin.

We'll need to experiment with the parameters but note that bbell prefers a slightly fuzzy icon over a letter. For the first experiment:
- if the icon is 90px square or larger, increase the size so it fills the tile. If there isn't too much artifacting, let's keep the implementation, otherwise follow up with bbell!
- If we don't have a 90px icon, fall back on screenshots (bug 1389259) then the favicon with the letter.

Follow-up note: bbell mentioned there are probably enough 128px square icons for this to work so we should try that value next (rather than using the more round, relative to our image size, 180px square).
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
Side note: if we're removing the centered-favicon with colored margin from top sites, I wonder if we should remove this from all of our UI (since top sites currently has the largest icons of them all).
Not prioritized for the current sprint and I have other things to focus on: unassigning.
Assignee: michael.l.comella → nobody
Assignee: nobody → michael.l.comella
Flags: needinfo?(s.kaspari)
Note: after investigating distributions (bug 1394641), to support our old users, we'll always need to be able to read the bgcolor property and draw a background color behind an icon, even if we don't do that for our code.
The problem is a different bitmap can be cached to disk based on who calls
first:
- It won't have a resize cap if AS calls first and will have artifacting, even
when downscaling for history items
- It'll be capped at 2x scale otherwise, which AS won't show because it's too
small (when we have a minimum size)

This resizing bitmap is cached and returned for every other call load in icons
so we get different results in *all* views based on who called first.

Quick thoughts on solutions:
- Have separate disk caches based on resizes
- Cache the original asset, not the resized asset
- Use the same icon algorithm everywhere wrt resizing & minimum sizes. Ideally,
I think this should be the case but I'm concerned how far reaching this might
be and that e.g. history icons appear more slow and can take lower resolution
images

Review commit: https://reviewboard.mozilla.org/r/177904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177904/
The problem here is that if AS first calls the icon, it'll get the original
icon size which it may reject (e.g. 64px). However, somewhere else will request
the icon, resize it 2x (128px), and cache that result. The next time AS calls,
it'll get the resized icon and display it.

Thoughts on solutions:
- It may be comprehensive with the previous issue
- We figure out how to get the original image size from the cache rather than
the resized size

Review commit: https://reviewboard.mozilla.org/r/177906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177906/
I'm having difficulty with this - for details see comment 16 and comment 17 above. For a high level summary, the icon cache is storing resized images but we're setting different resizing criteria for different views [1], so different bitmaps are cached depending on who calls it first and since all subsequent loads pull from the cache, it affects all icons in the app.

[1]: I didn't want to change how our icons work comprehensively because I'm afraid of the larger effects (e.g. when Gecko requests an icon, do we really want to return one with the same criteria we use for Top Sites?). Basically, as per comment 12, AS has no resize cap and will discard images below a certain size while other views will only resize 2x and do not discard icons.
(In reply to Michael Comella (:mcomella) from comment #16)
> Quick thoughts on solutions:
> ...
> - Use the same icon algorithm everywhere wrt resizing & minimum sizes.
> Ideally,
> I think this should be the case but I'm concerned how far reaching this might
> be and that e.g. history icons appear more slow and can take lower resolution
> images

I just wrote a patch and the few pages I have in my history don't have favicons at 64px+ and appear with generated icons, for example:
- bugzilla.mozilla.org
- addons.mozilla.org (a default bookmark)
- waitbutwhy
- gmx, a distribution
- yahoo redirect links
- blog.bugzilla.com (a vw blog)

This seems really unfortunate.

Another (hacky!) solution thought: we can add an activityStreamProcessor after the ResizingProcessor which will fall back to generated icons if the resized icons are too small. This is hacky because it takes advantage of the specific implementation.

Another thing I noticed: the disk cache doesn't have the resized image but the memory cache does.

---

Sebastian, how would you approach this (what I'm trying to accomplish is in comment 12)? Are you seeing something I don't?
Flags: needinfo?(s.kaspari)
Iteration: --- → 1.30
Rank: 1
Priority: P2 → P1
Just a thought: I'm coming at this looking for the perfect solution but perhaps we can get a good enough solution. e.g. I'm trying to both remove colored backgrounds and fix tiny favicons but we could just remove tiny favicons in top sites and do a separate bug for colored backgrounds.
(In reply to Michael Comella (:mcomella) from comment #20)
> e.g. I'm trying to both remove colored backgrounds

bug 1398863.
I'm concerned the plan in comment 12 will not work because we show too many placeholder icons (bug 19). However, it's hard to know for sure because the random sites I visit are not real profiles.

After speaking with Chenxia, I came up with a "good enough" approach: we discard the smallest images that are not worth scaling up and scale up the medium sized images just so that they're large enough to fill the top site space well and not display too much artifacting. I found some decent parameters by testing a few different favicons on a few devices.

I confirmed with Maria that she'd prefer the slightly artifacted, centered icons over the placeholder icons the original approach would encourage.

Due to time constraints, I'm going with the good-enough approach and I filed bug 1398970 to follow-up with a better implementation.
Here's what top sites would look like: the top row are fairly high res icons while the bottom row are scaled up 3x & generated.

Take any artifacting in the screenshot with a grain of salt: this change is really about how it looks on device, not how the screenshot looks on a computer. The screenshot should be useful for comparing the overall positioning of icons. github.com/accounts.google.com are the smallest icons we'll show and they're originally 32px scaled up to 96px.
(In reply to Michael Comella (:mcomella) from comment #8)
> Sebastian, do you know why there's a discrepancy between the top sites tile
> size (90dp) and the size we generate our favicons to (112dp)?

Looks like this is because highlights/pocket have a width of 112dp and we use smaller size (based on a ratio) for the height.
Flags: needinfo?(s.kaspari)
(In reply to Michael Comella (:mcomella) from comment #22)
> I confirmed with Maria that she'd prefer the slightly artifacted, centered
> icons over the placeholder icons the original approach would encourage.

That is because the placeholder icons are more usable and recognizable by users than the placeholders.
One thought: one way of testing this is to land it and see if anyone files a, "This looks awful" bug in response. :)
To be explicit, I tested on a Nexus 7 (xhdpi), a Nexus 5 (xxhdpi), and a Nexus S (? but I'm guessing hdpi). However, the result is a relationship between the screen size, the DPI, and honestly the display quality so my testing is by no means comprehensive.
Comment on attachment 8906169 [details]
Bug 1388396: Increase max favicon scale factor to 3.

https://reviewboard.mozilla.org/r/177904/#review183802

::: mobile/android/base/java/org/mozilla/gecko/icons/processing/ResizingProcessor.java:23
(Diff revision 2)
> +    // This is the largest factor we'll scale up an image by: the goal is an image
> +    // that both fills the top site space well but does not have extreme resizing
> +    // artifacts. This number was chosen anecdotally by comparing variously-sized
> +    // favicons across devices to see which factor(s) looked the best. bug 1398970
> +    // is filed to take a more comprehensive approach to favicons.
> +    public static final int MAX_SCALE_FACTOR = 3;

I wonder whether this value should depend on the device density. In the UI we display icons based on a physical size (dp) - but here we are scaling pixels. But that's something for bug 1398970 to think about..
Attachment #8906169 - Flags: review?(s.kaspari) → review+
Comment on attachment 8906170 [details]
Bug 1388396: Add IconRequestBuilder.forActivityStream.

https://reviewboard.mozilla.org/r/177906/#review183804

::: mobile/android/base/java/org/mozilla/gecko/icons/IconRequestBuilder.java:16
(Diff revision 2)
>  import org.mozilla.gecko.GeckoAppShell;
>  
>  import java.util.TreeSet;
>  
>  import ch.boye.httpclientandroidlib.util.TextUtils;
> +import org.mozilla.gecko.icons.processing.ResizingProcessor;

nit: This is class does not exist yet. :)

::: mobile/android/base/java/org/mozilla/gecko/icons/IconRequestBuilder.java:112
(Diff revision 2)
> +    public IconRequestBuilder forActivityStream() {
> +        // This value was set anecdotally: 16px icons scaled up both look blurry and
> +        // don't fill the space well. 32px icons look good enough.
> +        internal.minimumSizePxAfterScaling = 32 * ResizingProcessor.MAX_SCALE_FACTOR;
> +        return this;
> +    }

I like wrapping the configuration in a for*() method. I was a bit concerned about adding this directly to the icon code at first. I would like this code to be mostly independent - like a library for loading icons. But then I saw that I added already forLauncherIcon() and some of the loaders are very specific to Fennec already too. So I guess that's not something worth separating right now. :)
Attachment #8906170 - Flags: review?(s.kaspari) → review+
Comment on attachment 8906867 [details]
Bug 1388396: Use IconRequestBuilder.forActivityStream in AS UI.

https://reviewboard.mozilla.org/r/178600/#review183806
Attachment #8906867 - Flags: review?(s.kaspari) → review+
Comment on attachment 8906868 [details]
Bug 1388396: Add MinimumSizeProcessor; .forActivityStream takes effect.

https://reviewboard.mozilla.org/r/178602/#review183808

::: mobile/android/base/java/org/mozilla/gecko/icons/processing/MinimumSizeProcessor.java:23
(Diff revision 1)
> + * {@link ResizingProcessor} the second time, when it's loaded from the cache. It turned out to be much simpler
> + * to enforce the requirement that...
> + *
> + * This processor is expected to be called after {@link ResizingProcessor}.
> + */
> +public class MinimumSizeProcessor implements Processor {

There are a bunch of unit tests for the icon code. Could we add some for the new functionality too?

::: mobile/android/base/java/org/mozilla/gecko/icons/processing/MinimumSizeProcessor.java:33
(Diff revision 1)
> +        if (response.getBitmap().getWidth() >= request.getMinimumSizePxAfterScaling()) {
> +            return;
> +        }
> +
> +        // This is fragile: ideally, we can return the generated response but instead we're mutating the argument.
> +        final IconResponse generatedResponse = IconGenerator.generate(request.getContext(), request.getPageUrl());

nit: This is somewhat side-stepping that the "generator" for this execution is defined in the IconTask. But I guess fixing this would require passing the IconTask instance to all processors. Not sure if this refactor is worth it. I'm finding with landing this as is (although writing a test might be a bit cumbersome with the static method call).
Attachment #8906868 - Flags: review?(s.kaspari) → review+
Comment on attachment 8906170 [details]
Bug 1388396: Add IconRequestBuilder.forActivityStream.

https://reviewboard.mozilla.org/r/177906/#review183804

> nit: This is class does not exist yet. :)

`ResizingProcessor` is already in the code - `MinimumSizeProcessor` was added later.
Comment on attachment 8906868 [details]
Bug 1388396: Add MinimumSizeProcessor; .forActivityStream takes effect.

https://reviewboard.mozilla.org/r/178602/#review183808

> There are a bunch of unit tests for the icon code. Could we add some for the new functionality too?

I wrote a unit test as a follow up commit which I will self review - feel free to look into it too.

> nit: This is somewhat side-stepping that the "generator" for this execution is defined in the IconTask. But I guess fixing this would require passing the IconTask instance to all processors. Not sure if this refactor is worth it. I'm finding with landing this as is (although writing a test might be a bit cumbersome with the static method call).

> nit: This is somewhat side-stepping that the "generator" for this execution is defined in the IconTask.

I want to generate an icon, which I feel should conceptually be a static function with no side effects, independent of an object, which `generate` is. Ideally, this function would not be in the IconGenerator class because the naming/relationship implies the `generate` function should rely on the state of an `IconGenerator`.

In practice, it feels okay that this is side-stepping the generator of the IconTask because the function is indedpendent.

> (although writing a test might be a bit cumbersome with the static method call).

The function has no side effects so I don't think we'd need to mock it, just the arguments passed in (specifically the Context since it returns resources).

---

I made no changes to my code.
Comment on attachment 8907198 [details]
Bug 1388396 - review: Add TestMinimumSizeProcessor.

https://reviewboard.mozilla.org/r/178876/#review183958

r=trivial
Attachment #8907198 - Flags: review?(michael.l.comella) → review+
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0aff84a0a8af
Increase max favicon scale factor to 3. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/8756322ebb4a
Add IconRequestBuilder.forActivityStream. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/77f982a2f17b
Use IconRequestBuilder.forActivityStream in AS UI. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/44fa7b379742
Add MinimumSizeProcessor; .forActivityStream takes effect. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/3300c15011d3
review: Add TestMinimumSizeProcessor. r=mcomella
Comment on attachment 8907709 [details]
Bug 1388396 - bustage: Fix testBitmapNotScaledMoreThanTwoTimesTheSize.

https://reviewboard.mozilla.org/r/179378/#review184540

r=trivial
Attachment #8907709 - Flags: review?(michael.l.comella) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 35b990349de9 -d 34b7638b00c8: rebasing 419816:35b990349de9 "Bug 1388396: Increase max favicon scale factor to 3. r=sebastian"
merging mobile/android/base/java/org/mozilla/gecko/icons/processing/ResizingProcessor.java
rebasing 419817:79d36366ad97 "Bug 1388396: Add IconRequestBuilder.forActivityStream. r=sebastian"
merging mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java
merging mobile/android/base/java/org/mozilla/gecko/icons/IconRequestBuilder.java
rebasing 419818:bf33800ed618 "Bug 1388396: Use IconRequestBuilder.forActivityStream in AS UI. r=sebastian"
merging mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamOverridablePageIconLayout.java
merging mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesCard.java
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesCard.java! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Flags: needinfo?(michael.l.comella)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 1f29845030ca -d 0d2d6244f901: rebasing 419860:1f29845030ca "Bug 1388396: Increase max favicon scale factor to 3. r=sebastian"
rebasing 419861:187058bad7d6 "Bug 1388396: Add IconRequestBuilder.forActivityStream. r=sebastian"
rebasing 419862:488b67ad89f7 "Bug 1388396: Use IconRequestBuilder.forActivityStream in AS UI. r=sebastian"
merging mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesCard.java
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesCard.java! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Oh, I guess something landed on the autoland tree that I can't see because I'm on mozilla-central. Argh.
bug 1389296 conflicted with this on autoland so I pulled that down locally, rebased on top of it, pushed my updated commits to reviewboard, and I expect this to now land on autoland.
Depends on: 1389296
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ada7d6900274
Increase max favicon scale factor to 3. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/a5f78f2e5bfd
Add IconRequestBuilder.forActivityStream. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/b79ec5330563
Use IconRequestBuilder.forActivityStream in AS UI. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/bca0b077f039
Add MinimumSizeProcessor; .forActivityStream takes effect. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/84a794ceb84b
review: Add TestMinimumSizeProcessor. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/77a524d860a0
bustage: Fix testBitmapNotScaledMoreThanTwoTimesTheSize. r=mcomella
Duplicate of this bug: 1254663
This works as implemented, marking as verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.