997780 - Implement image loading for suggested sites in TopSitesPanel/TopSitesGridItemView

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Lucas Rocha (:lucasr)]

It uses an imageUrl/bgColor pair to define the image, which is different than the other top sites.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Load suggested sites images in TopSitesGridItemView (r=wesj)

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 8408407 [details] [diff] [review]
Load suggested sites images in TopSitesGridItemView (r=wesj)

The image loading logic in TopSitesPanel is overly complex right now. TopSitesGridItemView needs some love in this area. I decided to not cross this bridge just yet but filed bug 997774 to track this work.

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 8408407 [details] [diff] [review]
Load suggested sites images in TopSitesGridItemView (r=wesj)

Review of attachment 8408407 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/home/TopSitesGridItemView.java
@@ +246,5 @@
> +
> +        Picasso.with(getContext())
> +               .load(imageUrl)
> +               .noFade()
> +               .error(R.drawable.favicon)

Someday it might be nice to patch Picasso to take a Picasso.Listener here instead.

::: mobile/android/base/home/TopSitesPanel.java
@@ +546,5 @@
>                  debug("bindView called twice for same values; short-circuiting.");
>                  return;
>              }
>  
> +            if (type == TopSites.TYPE_SUGGESTED) {

TBH, I think I'd rather we keyed this off the presence of an imageUrl than off the rowType.

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #3)
> Comment on attachment 8408407 [details] [diff] [review]
> Load suggested sites images in TopSitesGridItemView (r=wesj)
> 
> Review of attachment 8408407 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/TopSitesGridItemView.java
> @@ +246,5 @@
> > +
> > +        Picasso.with(getContext())
> > +               .load(imageUrl)
> > +               .noFade()
> > +               .error(R.drawable.favicon)
> 
> Someday it might be nice to patch Picasso to take a Picasso.Listener here
> instead.

What kind of use case do you have in mind?

> ::: mobile/android/base/home/TopSitesPanel.java
> @@ +546,5 @@
> >                  debug("bindView called twice for same values; short-circuiting.");
> >                  return;
> >              }
> >  
> > +            if (type == TopSites.TYPE_SUGGESTED) {
> 
> TBH, I think I'd rather we keyed this off the presence of an imageUrl than
> off the rowType.

Good point, done.

Comment 5

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/af434ea88280

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/af434ea88280