Closed Bug 1400397 Opened 2 years ago Closed 2 years ago

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

Categories

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

All
Android
enhancement

Tracking

()

RESOLVED FIXED
Firefox 57
Iteration:
1.30
Tracking Status
firefox57 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mobileAS])

Attachments

(2 files)

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 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.
Attachment #8908868 - Flags: review?(liuche) → review+
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)?
Attachment #8908869 - Flags: review?(liuche) → review+
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.
Pushed by michael.l.comella@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/b4a4b9fa2cfc
https://hg.mozilla.org/mozilla-central/rev/73f00193147b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.