854011 - Bugs in grid.xml break thumbnail tile styling

Status: RESOLVED FIXED

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

Description

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Bug 852197 caused a regression in the Top Sites tile group; the tiles in this group are no longer styled as thumbnail tiles.

This actually exposed some pre-existing bugs that had conspired to cover each up.  The richgriditem binding defined a "backgroundimage" property (lowercase 'i'), but a few places in the code used "backgroundImage" instead.

In addition to fixing those bugs, this patch tries to simplify the code a little so we have fewer names to confuse ourselves with:

* Instead of caching color and backgroundImage in private fields, store them in attributes, and use those same attributes for styling the elements.
* Get rid of the separate setColor and setBackgroundImage methods; just put this code in the property setters.

The real problem is that we can't control when the XBL binding is applied after we call createElement.  This patch doesn't fully address that issue.  I think I know how to fix it but I want to coordinate with sfoster's changes in bug 808770.

Note: Sorry if this conflicts with any patches that people have in flight.

Comment 1

[Brian R. Bondy [:bbondy]]

Comment on attachment 728446 [details] [diff] [review]
patch

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

::: browser/metro/base/content/TopSites.js
@@ +158,5 @@
>  
>        if (this._useThumbs) {
>          let thumbnail = PageThumbs.getThumbnailURL(uri);
>          let cssthumbnail = 'url("'+thumbnail+'")';
> +        item.setAttribute("customImage", cssthumbnail);

maybe put a comment here that the binding isn't applied yet to explain why backgroundImage is not used.  The property also does this is it needed here?  this._box.style.backgroundImage = val;

::: browser/metro/base/content/bindings/grid.xml
@@ +526,5 @@
>            return null;
>          ]]></getter>
>        </property>
>  
> +      <property name="color" onget="return this.getAttribute('customColor');">

would it be less confusing to call this backgroundColor while you're here?

::: browser/metro/base/content/bookmarks.js
@@ +233,5 @@
>                         .getService(Components.interfaces.mozIColorAnalyzer);
>      ca.findRepresentativeColor(url, function (success, color) {
>        let colorStr = Bookmarks.fixupColorFormat(color);
>        Bookmarks.setFaveIconPrimaryColor(aBookmarkId, colorStr);
> +      aItem.setAttribute("customColor", colorStr);

Ditto comment. The setter for color also does this, is it needed here? this._box.style.backgroundColor = val;

Comment 2

[Matt Brubeck (:mbrubeck)]

(In reply to Brian R. Bondy [:bbondy] from comment #1)
> > +        item.setAttribute("customImage", cssthumbnail);
> 
> maybe put a comment here that the binding isn't applied yet to explain why
> backgroundImage is not used.

Added a comment.

> The property also does this is it needed here?
> this._box.style.backgroundImage = val;

We can't do this here because _box is a binding property and isn't available yet.  The constructor will do it when the binding is attached.

> > +      <property name="color" onget="return this.getAttribute('customColor');">
> 
> would it be less confusing to call this backgroundColor while you're here?

Possibly, but I decided to hold off on renaming for now because it would add to the conflicts with other pending patches.

> > +      aItem.setAttribute("customColor", colorStr);

This change was not necessary so I dropped it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8de57bc45b46

Comment 3

[Brian R. Bondy [:bbondy]]

Gotcha, makes sense. Thanks.

Comment 4

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/8de57bc45b46