Comment on attachment 9037587 [details] [diff] [review] result-message.patch Review of attachment 9037587 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/glodaFacet.js @@ +6,5 @@ > +/* global logException, Gloda */ > + > +ChromeUtils.import("resource:///modules/MailServices.jsm"); > + > +const createElement = (nodeName, attributes) => { I don't think this is a particulary good idea, especially as document.createElement also has a similar but incompatible signature. In general I don't like helper methods that are not useful enough. They add too much mental overhead to be worth it. Granted, it makes things slightly shorter here. @@ +970,5 @@ > } > } > } > > +class MozResultMessage extends HTMLElement { I've been thinking we should really add some documentation to these as we go. Something like /** * MozResultMessage displays an excerpt of a message. Typically these are used in the gloda * results listing, showing the messages that matched. */ @@ +1279,5 @@ > customElements.define("facet-date", MozFacetDate); > customElements.define("facet-boolean", MozFacetBoolean); > customElements.define("facet-boolean-filtered", MozFacetBooleanFiltered); > customElements.define("facet-discrete", MozFacetDiscrete); > +customElements.define("result-message", MozResultMessage); please rename to facet-result-message and MozFacetResultMessage

Comment on attachment 9037976 [details] [diff] [review] result-message.patch Review of attachment 9037976 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, r=mkmelin ::: mail/base/content/glodaFacet.js @@ +1298,5 @@ > + this.setAttribute("unread", "true"); > + } > +} > + > + nit: double empty rows here @@ +1303,5 @@ > customElements.define("facet-date", MozFacetDate); > customElements.define("facet-boolean", MozFacetBoolean); > customElements.define("facet-boolean-filtered", MozFacetBooleanFiltered); > customElements.define("facet-discrete", MozFacetDiscrete); > +customElements.define("facet-result-message", MozFacetResultMessage); I think we should actually start putting the define straight after the class definition block (where the double empty row is now in this patch)