Comment on attachment 9047754 [details] [diff] [review] de-xbl_glodacomplete-richlistitem.patch Review of attachment 9047754 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/db/gloda/content/gloda-contact-chunk.js @@ +6,5 @@ > + > +/* global MozXULElement, MozGlodacompleteBaseRichlistitem */ > + > +/** > + * The MozGlodaContactChunk widget displays autocomplete item with an autocomplete @@ +41,5 @@ > + <label class="ac-ellipsis-after ac-url-text" hidden="true"></label> > + <image class="ac-type-icon"></image> > + </hbox> > + </vbox> > + `)); two space indent please @@ +83,5 @@ > + > + if (contact == null) > + return; > + > + var identity = contact.identities[0]; please make all the var let instead while touching this ::: mailnews/db/gloda/content/gloda-fulltext-all.js @@ +32,5 @@ > +} > + > +MozXULElement.implementCustomInterface(MozGlodaFulltextAll, [Ci.nsIDOMXULSelectControlItemElement]); > + > +customElements.define("gloda-fulltext-all", MozGlodaFulltextAll, { extends: "richlistitem" }); I think it would be preferable to merge all these very related item into the same js file ::: mailnews/db/gloda/content/gloda-multi.js @@ +31,5 @@ > + > + renderItem(aObj) { > + var node = document.createElementNS( > + "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", > + "richlistitem"); just use document.createXULElement @@ +35,5 @@ > + "richlistitem"); > + > + node.obj = aObj; > + node.setAttribute("type", > + "gloda-" + this.row.nounDef.name + "-chunk"); should fit on one row, no? ::: mailnews/db/gloda/content/gloda-single-identity.js @@ +43,5 @@ > + </hbox> > + </vbox> > + </hbox> > + `)); > + indention 2 space please ::: mailnews/db/gloda/content/glodacomplete-base-richlistitem.js @@ +106,5 @@ > + > + _setUpDescription(aDescriptionElement, aText) { > + // Get rid of all previous text > + while (aDescriptionElement.hasChildNodes()) > + aDescriptionElement.lastChild.remove(); add braces for the loop ::: mailnews/db/gloda/content/glodacomplete.xml @@ +39,5 @@ > > + // Unlike our superclass, we create nodes every time because we have > + // heterogeneous results and we cannot rely on the XBL bindings to > + // to change fast enough. > + item = document.createElement("richlistitem", { is: result.getStyleAt(this._currentIndex) }); this is now the wrong comment

Comment on attachment 9048478 [details] [diff] [review] de-xbl_glodacomplete-richlistitem.patch Review of attachment 9048478 [details] [diff] [review]: ----------------------------------------------------------------- Seems to be working, but some stuff to do still. I wonder if there's not a bunch of unused functionality here... Also, can you perhaps put a note in glautocomp.js about the full types. It's hard to find where these get set up as the type names are build dynamically :/ ::: mailnews/db/gloda/content/autocomplete-richlistitem.js @@ +186,5 @@ > + } > + } > + > + MozXULElement.implementCustomInterface(MozGlodacompleteBaseRichlistitem, > + [Ci.nsIDOMXULSelectControlItemElement]); Looks nicer as MozXULElement.implementCustomInterface( MozGlodacompleteBaseRichlistitem, [Ci.nsIDOMXULSelectControlItemElement] ); @@ +204,5 @@ > + "label.ac-url-text": "selected", > + }; > + } > + > + connectedCallback() { same comments as in bug 1531305 re children and delayConnectedCallback @@ +246,5 @@ > + this._identityBox = this.querySelector(".ac-url"); > + this._identity = this.querySelector("description.ac-url-text"); > + > + this._nameBox = this.querySelector(".ac-title"); > + this._name = this.querySelector(this, "anonid", "name"); I think we want to get away from the anonid querying. just acc a class and query on that instead, perhaps name it something more destinctive than "name". what is this actually matching? @@ +256,5 @@ > + this.initializeAttributeInheritance(); > + } > + > + get label() { > + let identity = this.obj; what's this.obj? I don't see it getting set anywhere? @@ +301,5 @@ > + get label() { > + return "full text search: " + this.row.item; // what is this for? l10n? > + } > + > + _adjustAcItem() { this is just used once, so just inline the code. same for the other places @@ +524,5 @@ > + > + MozXULElement.implementCustomInterface(MozGlodaSingleTag, [Ci.nsIDOMXULSelectControlItemElement]); > + > + customElements.define("glodacomplete-base-richlistitem", > + MozGlodacompleteBaseRichlistitem, { extends: "richlistitem" }); please just define them after the class body as you go

Comment on attachment 9050276 [details] [diff] [review] de-xbl_glodacomplete-richlistitem.patch Review of attachment 9050276 [details] [diff] [review]: ----------------------------------------------------------------- When fixing comment, check if you have the same problem more than once. I'm not necessarily commenting on each. Not specific to this bug, but in general ::: mailnews/db/gloda/content/autocomplete-richlistitem.js @@ +13,5 @@ > + * @extends {MozElements.MozRichlistitem} > + */ > + class MozGlodacompleteBaseRichlistitem extends MozElements.MozRichlistitem { > + connectedCallback() { > + if (this.delayConnectedCallback() || this.hasChildNodes()) { since you don't add any childhodes, you don't need the this.hasChildNodes() protection @@ +60,5 @@ > + let start = 0; > + let end = 0; > + let boundaries = []; > + let len = regions.length; > + for (let i = 0; i < len; i++) { please just use regions.length directly. having len as a temp is un-needed micro-optimization @@ +214,5 @@ > + > + connectedCallback() { > + super.connectedCallback(); > + > + this.appendChild(MozXULElement.parseXULToFragment(` here you do need protection against double connectedCallbacks. should likely also check delayConnectedCallback @@ +305,5 @@ > + > + this._explanation = document.createElement("description"); > + this._explanation.classList.add("explanation"); > + let label = gGlodaCompleteStrings.get("glodaComplete.messagesMentioningMany.label"); > + this._explanation.setAttribute("value", label.replace("#1", this.row.words.join(", "))); this._explanation seems to not end up in the DOM at any point?? @@ +329,5 @@ > + class MozGlodaFulltextSingle extends MozGlodacompleteBaseRichlistitem { > + connectedCallback() { > + super.connectedCallback(); > + > + this._explanation = document.createElement("description"); same here @@ +359,5 @@ > + * > + * @extends MozGlodacompleteBaseRichlistitem > + */ > + class MozGlodaMulti extends MozGlodacompleteBaseRichlistitem { > + connectedCallback() { same checks needed here @@ +431,5 @@ > + } > + > + connectedCallback() { > + super.connectedCallback(); > + this.appendChild(MozXULElement.parseXULToFragment(` and around here

Comment on attachment 9051819 [details] [diff] [review] de-xbl_glodacomplete-richlistitem.patch Review of attachment 9051819 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just a few minor changes. r=mkmelin with the below fixed ::: mailnews/db/gloda/content/autocomplete-richlistitem.js @@ +6,5 @@ > + > +/* global MozXULElement, MozElements */ > +{ > + /** > + * The MozGlodacompleteBaseRichlistitem widget shares the base class is the abstract base class for all the gloda autocomplete items. Please also add @abstract @@ +98,5 @@ > + _needsAlternateEmphasis(aText) { > + for (let i = aText.length; --i >= 0;) { > + let charCode = aText.charCodeAt(i); > + // Arabic, Syriac, Indic languages are likely to have ligatures > + // that are broken when using the main emphasis styling Please fix all the sentences to have a final period. @@ +214,5 @@ > + connectedCallback() { > + if (this.delayConnectedCallback() || this.hasChildNodes()) { > + return; > + } > + super.connectedCallback(); probably worth doing super before the other check. not sure if it would cause problems to do it like this either, but it's more fragile than needed (same for the other widgets here, call super first)