Comment on attachment 9032810 [details] [diff] [review] facet-base-bindings.patch Review of attachment 9032810 [details] [diff] [review]: ----------------------------------------------------------------- Seems to work I do see some bugs (rendering issues such as overlapping texts and clicking around fairly easily something is null. But this happens on trunk too. ::: mail/base/content/glodaFacet.js @@ +122,5 @@ > + > + set disabled(val) { > + if (val) { > + this.setAttribute("disabled", "true"); > + this.checkbox.setAttribute("disabled", true); "true" @@ +141,5 @@ > + this.checkbox.checked = val; > + if (val) { > + // the XBL inherits magic appears to fail if we explicitly check the > + // box itself rather than via our click handler, presumably because > + // we unshadow something. So manually apply changes ourselves. presumably this could go? please investigate and remove, or change the comment to reflect current state without xbl

Comment on attachment 9033388 [details] [diff] [review] facet-base-bindings.patch Review of attachment 9033388 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, just a couple of style nits. r=mkmelin ::: mail/base/content/glodaFacetBindings.xml @@ +122,2 @@ > > + if(aType === "boolean") { if ( @@ +123,5 @@ > + if(aType === "boolean") { > + facet = document.createElement("facet-boolean"); > + facet.setAttribute("name", aAttrDef.attributeName); > + facets.appendChild(facet); > + } else if(aType === "boolean-filtered") { } else if (

This patch now depends upon facets binding patch(Bug 1516640). Magnus I have made some changes, I dont think it needs a separate feedback request. You can take a look at the changes tho.

> This patch now depends upon facets binding patch(Bug 1516640). Magnus I have > made some changes, I dont think it needs a separate feedback request. You > can take a look at the changes tho. Why? Seems like a bad idea to throw in that as a dependency just before landing. Please always focus on landing as small parts as possible. This will help reviews, and also regression hunting. I tried it now with bug 1516640 also attached. Nothing works anymore. I just get facet is undefined glodaFacetView.js:137 and then it's of course downhill from there.

Comment on attachment 9035598 [details] [diff] [review] facet-base-bindings.patch Review of attachment 9035598 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, r=mkmelin