879113 - Defect - Start screen should handle error cases when missing favicons, thumbnails gracefully

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Firefox Start, defect, P1)

Description

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Black tiles screenshot

See attachment. Happens after a few seconds for bookmarks and top sites. Where there is a thumbnail, the page title is black, when there isn't, the entire tile is black.

ColorAnalyzer throws a bunch of exceptions like this one:

Error: ColorAnalyzer: image at http://mozorg.cdn.mozilla.net/media/img/favicon.ico didn't load
Source File: file:///c:/mozilla-src/inbound-src/obj-i686-pc-mingw32/dist/bin/components/ColorAnalyzer.js
Line: 60

Comment 1

[Allison Naaktgeboren :ally]

Well, in the event that we cannot retrieve colors or thumbnails, what do we want the colorization to be? Yuan, Asa, Stephen?

Comment 2

[Stephen Horlander [:shorlander]]

How do we end up in this state? Ideally the color would be cached and the favicon should be cached.

If we end up in a situation where we can't get the thumbnail I would like to use the Firefox placeholder instead coloring the entire tile: http://people.mozilla.com/~shorlander/files/metro/metro-infobars-mockup-01.html

Comment 3

[Allison Naaktgeboren :ally]

We don't cache now because of nontrivial pathological interactions between the color analyzer code & the old cache code. It was a serious headache when I was adding the color analyze code for the tiles. I think that can be a v2 feature.

What about the caption bars? Same grey we default to when there is no favicon?

Comment 4

[Jim Mathies [:jimm]]

Seems like a P1 to me, feel free to update if you feel otherwise.

Comment 5

[Allison Naaktgeboren :ally]

It turns out that topsites.js has changed greatly since I last saw it. We do have startup caching in topsites and the favicons service behaves as if it has a cache somewhere of its own (though I have verified that yet). 

Furthermore, ColorAnalyzer does not actually throw. It prints an error to the console on line 60, and returns white/back for color values. White/black might be a valid combination, so treating that set of values as a error might not be the best idea.

Comment 6

[Allison Naaktgeboren :ally]

points=3 because I don't have a handy exception or an error callback in the api, so this will take some more regijjering than I bargained for.

Comment 7

[Matthew N. [:MattN]]

(In reply to :Ally Naaktgeboren from comment #5)
> Furthermore, ColorAnalyzer does not actually throw. It prints an error to
> the console on line 60, and returns white/back for color values. White/black
> might be a valid combination, so treating that set of values as a error
> might not be the best idea.

Unless I'm misunderstanding, you can use the first argument to mozIRepresentativeColorCallback [1] which indicates success or failure. Metro's getForegroundAndBackgroundIconColors just ignores that argument which is the main issue.

[1] https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/mozIRepresentativeColorCallback

Comment 8

[Allison Naaktgeboren :ally]

Attachment: debugging patch

Comment 9

[Allison Naaktgeboren :ally]

MattN, does using the bool like in the following manner what you had in mind?

getForegroundAndBackgroundIconColors(aIconURI, aSuccessCallback, aErrorCallback) {
    if (!aIconURI) {
      return;
    }
    let that = this;
    let wrappedIcon = aIconURI;
    ColorAnalyzer.findRepresentativeColor(wrappedIcon, function (success, color) {
      let foregroundColor = that.bestTextColorForContrast(color);
      let backgroundColor = that.convertDecimalToRgbColor(color);
      // let the caller do whatever it is they need through the callback
      if (success) {
        aSuccessCallback(foregroundColor, backgroundColor);
      } else {
        aErrorCallback();
      }
    }, this);
  },

Comment 10

[Matthew N. [:MattN]]

(In reply to :Ally Naaktgeboren from comment #9)
> MattN, does using the bool like in the following manner what you had in mind?
>
>       let foregroundColor = that.bestTextColorForContrast(color);
>       let backgroundColor = that.convertDecimalToRgbColor(color);

Yeah, that makes sense although there is no need to run the above two lines if !success so I would probably do an early return after calling aErrorCallback when findRepresentativeColor fails:

getForegroundAndBackgroundIconColors(aIconURI, aSuccessCallback, aErrorCallback) {
    if (!aIconURI) {
      return;
    }
    let that = this;
    let wrappedIcon = aIconURI;
    ColorAnalyzer.findRepresentativeColor(wrappedIcon, function (success, color) {
      if (!success) {
        aErrorCallback();
        return;
      }

      let foregroundColor = that.bestTextColorForContrast(color);
      let backgroundColor = that.convertDecimalToRgbColor(color);
      // let the caller do whatever it is they need through the callback
      aSuccessCallback(foregroundColor, backgroundColor);
    }, this);
  },

Perhaps the comment should then be tweaked or removed.

Comment 11

[Allison Naaktgeboren :ally]

So this is really two problems: one is the case where the icons are not available and one where the the pagethumbs no longer are (In the original STR neither are available). 

Since it's two problems, I'm going to split this into two patches.

Comment 12

[Allison Naaktgeboren :ally]

Attachment: part 1/2 updating util to use errorcallbacks for missing favicons

in both bookmarks & topsites the desired coloring on the mock when favicon is missing is the defaulted one. 

Note, the coloring function can fail even if the favicon is present in cache (mint.com's one is a good test case for this. it might have transparency in it?).

Comment 13

[Allison Naaktgeboren :ally]

Please leave bug open for part 2

 https://hg.mozilla.org/integration/mozilla-inbound/rev/ed0162953c0a

Comment 14

[Ed Morley [:emorley]]

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

Comment 15

[Allison Naaktgeboren :ally]

Attachment: part 2/2 v0

in particular, am I updating between aSite & aTile correctly? I'd like to make sure the logic is correct before refactoring the css.

Note, the watermark layout is different than what shortlander requested, artifact of bug 873171.

Comment 16

[Allison Naaktgeboren :ally]

Attachment: screenshot, combos of missing favicons, missing thumbnails

Comment 17

[Allison Naaktgeboren :ally]

On the screenshot note that:
sfgate & questionable content: have favicon & thumbnail
xkcd/purple: has no favicon but has thumbnail
facebook: has favicon but no thumbnail
twitter: neither favicon nor thumbnail

Comment 18

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 776017 [details] [diff] [review]
part 2/2 v0

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

::: browser/metro/base/content/TopSites.js
@@ +307,5 @@
>      if (this._useThumbs) {
> +      // TODO would like to move this into the css styling
> +      // after talking with sfoster, this may involve reworking all the css rules relying on customImage
> +      // to do this correctly
> +      aSite.backgroundImage = 'url("chrome://browser/skin/images/firefox-watermark.png")';

Yes, in the tile.css we'll end up with something like: 

richgriditem[tiletype='thumbnail'] .tile-start-container {
  background-image: 'url("chrome://browser/skin/images/firefox-watermark.png")
}

.. so, aSite needs to have aSite.backgroundImage == undefined, which means that when you applyToNode you'll get no customImage attribute and that default will be used. 
Having no customImage attribute currently has the side-effect that the thumbnail style tile rules don't match. So I suggest we avoid overloading customImage here and adjust all the richgriditem[customImage] selectors in tiles.css to be richgriditem[tiletype='thumbnail']. That's more explicit and as we need tiletype anyway when we calculate tile sizes, ties it all together a bit.

then, in richgrid's createItemElement do 

if(this.hasAttribute("tiletype")
  item.setAttribute("tiletype", this.getAttribute("tiletype"))

@@ +319,5 @@
> +          Cu.reportError("file path exists, overwriting the backgroundimage");
> +          let thumbnailURL = PageThumbs.getThumbnailURL(aSite.url);
> +          aSite.backgroundImage = 'url("'+thumbnailURL+'")';
> +          if ("isBound" in aTileNode && aTileNode.isBound) {
> +            aSite.applyToTileNode(aTileNode);

There's an optimization here which we shouldn't implement prematurely, but instead of aSite.applyToTileNode we could do aTileNode.backgroundImage = thumbnailURL; as the setter in the richgriditem binding should take care of the rest.

@@ +322,5 @@
> +          if ("isBound" in aTileNode && aTileNode.isBound) {
> +            aSite.applyToTileNode(aTileNode);
> +          }
> +        }
> +      }, Components.utils.reportError);

Unless you're going to do something with the error case here, I would just let it fall through and get caught by the errback at the end of the chain which we define below on lastPromise

Comment 19

[Allison Naaktgeboren :ally]

So I don't lose track of this feedback too

(17:09:43) mbrubeck: By the way, I personally find the Task version more readable, but if you do use promise.then directly, I'd prefer to see "OS.File.exists().then(...).then(...)" instead of "let promise = OS.File.exists(); let newPromise = promise.then(...); let lastPromise = newPromise.then(...)"

(17:11:36) mbrubeck: Aside from readability, one benefit of Task is that it doesn't silently drop errors; you get automatic error->exception conversion without any boilerplate.

Comment 20

[Allison Naaktgeboren :ally]

Attachment: part 2/2 v1, cleaned up & working

Sam, wrt yielding the Task.spawn() that we talked about in irc, I looked through the metro code(browser-ui/appbar/browser.js) which already uses task.spawn(). They don't yield() (although it looks like some of our tests do), so I opted to follow the convention and not use yield with this one.

Comment 21

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 776868 [details] [diff] [review]
part 2/2 v1, cleaned up & working

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

Looks good, but we're getting a test failure due to a "OS" being a leaked window property. Perhaps we can move the import of osfile.jsm to browser-script.js thusly: 

XPCOMUtils.defineLazyGetter(this, "OS",
                                  "resource://gre/modules/osfile.jsm");

..that gives us the same net result - lazy-loading of the module as needed (and wfm). 

r+ with that and other comments.

::: browser/metro/base/content/TopSites.js
@@ +304,5 @@
>        ColorUtils.getForegroundAndBackgroundIconColors(xpFaviconURI, successAction, failureAction);
>      });
>  
>      if (this._useThumbs) {
> +      Components.utils.import("resource://gre/modules/osfile.jsm");

Let me make sure I've understood the logic here: We have default thumbnail specified via CSS. If a thumbnail exists for the site we assign its url to the backgroundImage prop of the tile. For all other negative/failure cases we just want to leave the default as-is. I think that's the right thing to do, and if so this looks right.

::: browser/metro/base/content/bindings/grid.xml
@@ +556,5 @@
>              typeSizes = this.ownerDocument.defaultView._richgridTileSizes = {};
>  
>              let sheets = this.ownerDocument.styleSheets;
>              // the (first matching) rules that will give us tile type => width/height values
> +            // Nota Bene consuming from the css will use " not ' for string match

Lets explain what's going on here (my bad). Something like: 
The keys in this object are string-matched against the selectorText of rules in our stylesheet. Quoted values in a selector will always use " not '

::: browser/metro/theme/tiles.css
@@ +16,5 @@
>  richgriditem {
>    width: @grid_double_column_width@;
>    height: @grid_row_height@;
>  }
> +richgriditem[tiletype='thumbnail'] {

As the CSS engine will convert these to " anyhow, lets avoid confusion by using " for these quoted values throughout

Comment 22

[Allison Naaktgeboren :ally]

(In reply to Sam Foster [:sfoster] from comment #21)
> Comment on attachment 776868 [details] [diff] [review]
> part 2/2 v1, cleaned up & working
> 
> Review of attachment 776868 [details] [diff] [review]:
> -----------------------------------------------------------------

> ::: browser/metro/base/content/TopSites.js
> @@ +304,5 @@
> >        ColorUtils.getForegroundAndBackgroundIconColors(xpFaviconURI, successAction, failureAction);
> >      });
> >      if (this._useThumbs) {
> > +      Components.utils.import("resource://gre/modules/osfile.jsm");
> 
> Let me make sure I've understood the logic here: We have default thumbnail
> specified via CSS. If a thumbnail exists for the site we assign its url to
> the backgroundImage prop of the tile. For all other negative/failure cases
> we just want to leave the default as-is. I think that's the right thing to
> do, and if so this looks right.

Yup.

Comment 23

[Allison Naaktgeboren :ally]

Attachment: part 2/2 r+ w/nits addressed

inbound's been closed all day. Lots of metro work is waiting to land, in particular Bug 885242.  This means we might bitrot each other, so dear committer (if that is not me) if that is a problem, thank you for trying and I can deal with it next time (allyIsAwake && inboundOpen) is true.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b632352da2e7

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130807030216
Built from http://hg.mozilla.org/mozilla-central/rev/1fb5d14e8348

WFM
Tested on windows 8 using latest nightly for iteration-11. I don't see any top sites' tiles and bookmarks' tiles in black as well as titles.

Comment 27

[Samvedana (:Samvedana)]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130815030203
Built from http://hg.mozilla.org/mozilla-central/rev/a8daa428ccbc

WFM
Tested on windows 8 using latest nightly for iteration-12. I don't see any top sites' tiles and bookmarks' tiles in black as well as titles.