1400397 - Highlights may fail to load image (via https://getpocket.com/explore/trending)

Status: RESOLVED FIXED

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

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Chenxia discovered https://getpocket.com/explore/trending shows a blank image in highlights. It turns out our override image url is https://getpocket.com/explore/trending, which explains why there is no image there. :P

There are two issues here:
1) If we fail to load the override image url, we should fall back on the pageURL.
2) Why are we extracting https://getpocket.com/explore/trending as an image?

Let's deal with #1 here.

---

Adding to the current sprint because it's important and prominent if our pocket link does this.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

#2 is bug 1400398.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1400397: Use Icons if overridePageURL fails to load.

Icons apparently doesn't fade images, however, so it looks bad. Also, we try to
request the image each time we bind, so scrolling up and down will create
additional pop-in, which also sucks.

Review commit: https://reviewboard.mozilla.org/r/180490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180490/

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1400397: Do not try to reload failed highlight images.

Review commit: https://reviewboard.mozilla.org/r/180492/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180492/

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8908869 [details]
Bug 1400397: Do not try to reload failed highlight images.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180492/diff/1-2/

Comment 5

[Chenxia Liu [:liuche]]

Comment on attachment 8908868 [details]
Bug 1400397: Use Icons if overridePageURL fails to load.

https://reviewboard.mozilla.org/r/180490/#review186198

Okay, add error handling, good.

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8908869 [details]
Bug 1400397: Do not try to reload failed highlight images.

https://reviewboard.mozilla.org/r/180492/#review186200

I also wish Picasso had some way of limiting the number of refetches, especially since we don't know if these images are always going to be missing, or are missing because of chance and would succeed if we retried.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamOverridablePageIconLayout.java:107
(Diff revision 2)
> +        // time this method is called, which is each time the View is shown (or hidden and reshown): we prevent this by
> +        // checking against a cache of failed request urls. If we let Picasso make the request each time, this is bad
> +        // for the user's network and the replacement icon will pop-in after the timeout each time, which looks bad.
>          if (NetworkUtils.isWifi(getContext()) &&
> -                !TextUtils.isEmpty(overrideImageURL)) {
> +                !TextUtils.isEmpty(overrideImageURL) &&
> +                !nonFaviconFailedRequestURLs.contains(overrideImageURL)) {

So we don't refetch ever? Maybe it's better to try a certain number of times, rather than always failing once we've failed before. But I guess this is stored in memory so it gets cleared out. 

I'd be curious to know if our failures are due to "one-off" failures, or if these sites will just *always* fail.

Any telemetry that we can add (in a follow-up)?

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8908869 [details]
Bug 1400397: Do not try to reload failed highlight images.

https://reviewboard.mozilla.org/r/180492/#review186200

> So we don't refetch ever? Maybe it's better to try a certain number of times, rather than always failing once we've failed before. But I guess this is stored in memory so it gets cleared out. 
> 
> I'd be curious to know if our failures are due to "one-off" failures, or if these sites will just *always* fail.
> 
> Any telemetry that we can add (in a follow-up)?

> So we don't refetch ever?

Not in the current implementation.

> Maybe it's better to try a certain number of times, rather than always failing once we've failed before.

The reason I only tried once was that Picasso will make the request, so the top sites tile is blank, and then when it fails, our other icon (a favicon) will pop in. If you give it the opportunity to retry, either 1) scrolling up and scrolling down or 2) opening a context menu will repeat the behavior when Picasso retries, which looks really bad that the same replacement image is popping in multiple times (especially since it doesn't fade).

> I'd be curious to know if our failures are due to "one-off" failures, or if these sites will just always fail.

In Pocket's case, it'd always fail because it wasn't a url to an image (I filed bug 1400398).

I'm concerned this could be complicated to determine. It'd be hard to distinguish events like local network timeout and server timeout, and I don't know if our icon downloading follows redirects but if so, we can't just parse the URL to check for always failure.

> Any telemetry that we can add (in a follow-up)?

A good idea! Do you propose recording a value each time we add an item to the cache so that we can determine if this is a widespread problem? If so, I filed bug 1401045.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4a4b9fa2cfc
Use Icons if overridePageURL fails to load. r=liuche
https://hg.mozilla.org/integration/autoland/rev/73f00193147b
Do not try to reload failed highlight images. r=liuche

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b4a4b9fa2cfc
https://hg.mozilla.org/mozilla-central/rev/73f00193147b