Tim Nguyen :ntim
	Comment 5 • 5 years ago

Comment on attachment 9077186 [details] [diff] [review]
1542720-dexbl-glodasearch-WIP.patch

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

::: mail/base/content/search.js
@@ +230,5 @@
> +    }
> +  }
> +
> +  MozXULElement.implementCustomInterface(MozGlodaSearch, [Ci.nsIObserver]);
> +  customElements.define("gloda-search", MozGlodaSearch, { extends: "autocomplete-input" });

Here, you seem to be defining an element with the markup `<autocomplete-input is="gloda-search" />` which doesn't seem like how you're using it at the moment.

I think what you want to do is: `customElements.define("gloda-search", MozGlodaSearch, { extends: "input" });`
and then use it as: `<html:input is="gloda-search" />`

	Geoff Lankow (:darktrojan)
	Comment 8 • 5 years ago

Comment on attachment 9077233 [details] [diff] [review]
1542720-dexbl-glodasearch.patch

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

Some work needed but it's minor. It's hard to check without the autocomplete working, so let's come back to this when it's all ready.

::: mail/base/content/glodaFacetTab.js
@@ +9,1 @@
>  var { GlodaMsgSearcher } = ChromeUtils.import("resource:///modules/gloda/msg_search.js");

Move this import into search.js with the others.

::: mail/base/content/mainMailToolbox.inc.xul
@@ -323,5 @@
>                   title="&glodaSearch.title;"
>                   align="center"
>                   flex="1"
>                   class="chromeclass-toolbar-additional">
> -        <textbox id="searchInput"

Where'd the ID go?

::: mail/base/content/messenger.xul
@@ +595,1 @@
>                      <box id="glodaSearchFacets"/>

A bit of digging reveals this box did something for about two months in 2009. Let's get rid of it and the incorrect comment about it.

::: mail/base/content/search.js
@@ +111,5 @@
> +              case "mailnews.database.global.indexer.enabled":
> +              this.inputSearch.glodaEnabled =
> +                Services.prefs.getBoolPref("mailnews.database.global.indexer.enabled");
> +              this.inputSearch.collapsed = !this.inputSearch.glodaEnabled;
> +              break;

Indent these four lines.

@@ +157,5 @@
> +        Services.prefs.addObserver("mailnews.database.global.indexer.enabled",
> +          this._prefObserver);
> +
> +        this.glodaCompleter = Cc["@mozilla.org/autocomplete/search;1?name=gloda"]
> +          .getService(Ci.nsIAutoCompleteSearch).wrappedJSObject;

Line up the dots with the [, like how it was.

@@ +175,5 @@
> +    }
> +
> +    // Remove???
> +    get menupopup() {
> +      return document.getAnonymousElementByAttribute(this, "anonid", "quick-search-menupopup");

Seems like we're using popup now, so yeah remove, I think.

::: mail/components/im/content/chat-messenger.inc.xul
@@ -60,5 @@
>                             title="&amp;glodaSearch.title;"
>                             align="center"
>                             flex="1"
>                             class="chromeclass-toolbar-additional">
> -                <textbox id="IMSearchInput"

This ID (and the other one I commented on) is still in use. There's also a mention in mail/components/im/content/chat.css that needs to be removed.

https://searchfox.org/comm-central/rev/bff4ee41a668a974944fe859a9517ece5987ddd3/mail/base/content/mailWindowOverlay.js#3153

::: mail/themes/osx/mail/compose/messengercompose.css
@@ -1036,5 @@
>    -moz-appearance: textfield !important;
>    margin: 3px;
>  }
>  
> -#searchInput > .textbox-input-box #sidebar {

Neither of these match anything in the compose window. I guess they must've, once. Take them away!

::: mail/themes/osx/mail/searchBox.css
@@ +33,5 @@
>    box-shadow: var(--focus-ring-box-shadow);
>  }
>  
> +.gloda-search,
> +.gloda-search,

Hmm…

	Tim Nguyen :ntim
	Comment 11 • 5 years ago • Edited

Comment on attachment 9077813 [details] [diff] [review]
1542720-dexbl-glodasearch.patch

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

::: mail/themes/osx/mail/compose/messengercompose.css
@@ +1034,1 @@
>    -moz-appearance: textfield !important;

This looks wrong since HTML inputs can't have children.

I don't know what this rule is about, but if `-moz-appearance: textfield` is supposed to style the input itself, I don't think it's still needed given that this is the default style on inputs.

::: mail/themes/osx/mail/searchBox.css
@@ +31,5 @@
>    border-color: -moz-mac-focusring;
>    box-shadow: var(--focus-ring-box-shadow);
>  }
> +.gloda-search,
> +.gloda-search,

Duplicated selector

::: mail/themes/shared/mail/searchBox.css
@@ +34,5 @@
>  .remote-gloda-search:-moz-lwtheme {
>    background-color: var(--lwt-toolbar-field-background-color, hsla(0,0%,100%,.8));
>    color: var(--lwt-toolbar-field-color, black);
>  }
> +.gloda-search:not([focused="true"]):-moz-lwtheme,

You probably want to switch `[focused="true"]` to `:focus` here and below since `[focused]` is specific to `textbox`

@@ +46,5 @@
>  .themeableSearchBox:-moz-lwtheme:not([disabled]):hover,
>  .remote-gloda-search:-moz-lwtheme:hover {
>    background-color: var(--lwt-toolbar-field-background-color, white);
>  }
> +.gloda-search:-moz-lwtheme[focused="true"],

same here.

@@ +54,5 @@
>    background-color: var(--lwt-toolbar-field-focus, var(--lwt-toolbar-field-background-color, white));
>    color: var(--lwt-toolbar-field-focus-color, var(--lwt-toolbar-field-color, black));
>    border-color: var(--toolbar-field-focus-border-color);
>  }
> +:root[lwt-selection] .gloda-search .textbox-input:-moz-lwtheme::selection,

This selector is wrong. <textbox> used to have anonymous XBL children (including .textbox-input), but that's no longer the case with the HTML input.

`:root[lwt-selection] .gloda-search::selection` should just be fine.

::: mail/themes/windows/mail/searchBox.css
@@ +62,5 @@
>      border-color: ThreeDShadow;
>    }
>  }
>  /* Add margins to show the hover box-shadow */
> +.gloda-search

I think you forgot `,` at the end of line.

@@ +72,5 @@
>  .themeableSearchBox[focused="true"] {
>    border-color: Highlight;
>  }
>  /* special treatment because these boxes are on themable toolbars */
> +.gloda-search

same here.

	Magnus Melin [:mkmelin] Reporter
	Comment 12 • 5 years ago

Comment on attachment 9077813 [details] [diff] [review]
1542720-dexbl-glodasearch.patch

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

::: mail/base/content/mainMailToolbox.inc.xul
@@ +325,5 @@
>                   flex="1"
>                   class="chromeclass-toolbar-additional">
> +        <html:input type="text"
> +                    is="gloda-search"
> +                    id="searchInput"

would put is and id on the first line

::: mail/base/content/search.js
@@ +1,4 @@
> +/**
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

please add a blank line after this

@@ +38,5 @@
>          event.preventDefault();
>          event.stopPropagation();
> +      });
> +
> +      this.addEventListener("keypress", (event) => {

please combine the keypress handlers

@@ +51,5 @@
> +    }
> +    connectedCallback() {
> +      super.connectedCallback();
> +      // @implements {nsIObserver}
> +      this._prefObserver = {

this is named with underscore, and the other observer isn't. For consistency, maybe both without?

@@ +74,1 @@
>          inputSearch: this,

use the => way to declare the methods utilizing inputSearch and you can drop the inputSearch and simply access this

::: mail/base/jar.mn
@@ +71,5 @@
>      content/messenger/aboutDialog.css               (content/aboutDialog.css)
>      content/messenger/converterDialog.css           (content/converterDialog.css)
>      content/messenger/notification.css              (content/notification.css)
>  *   content/messenger/messenger.css                 (content/messenger.css)
> +    content/messenger/search.js                     (content/search.js)

can we name it gloda-search.js

	Magnus Melin [:mkmelin] Reporter
	Comment 14 • 5 years ago

Comment on attachment 9078230 [details] [diff] [review]
1542720-dexbl-glodasearch.patch

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

Didn't try it but looks alright to me

::: mail/base/content/gloda-search.js
@@ +170,5 @@
> +      if (this.value) {
> +        let tabmail = document.getElementById("tabmail");
> +        // If the current tab is a gloda search tab, reset the value
> +        //  to the initial search value.  Otherwise, clear it.  This
> +        //  is the value that 3is going to be saved with the current

remove the 3

there is also some strange spacing in here

@@ +171,5 @@
> +        let tabmail = document.getElementById("tabmail");
> +        // If the current tab is a gloda search tab, reset the value
> +        //  to the initial search value.  Otherwise, clear it.  This
> +        //  is the value that 3is going to be saved with the current
> +        //  tab when we switch back to it next.

T

@@ +178,5 @@
> +        if (tabmail.currentTabInfo.mode.name == "glodaFacet") {
> +          // we'd rather reuse the existing tab (and somehow do something
> +          // smart with any preexisting facet choices, but that's a
> +          // bit hard right now, so doing the cheap thing and closing
> +          // this tab and starting over

Please make the sentences capitalized, and dot at the end.

@@ +204,5 @@
> +    }
> +  }
> +
> +  MozXULElement.implementCustomInterface(MozGlodaSearch, [Ci.nsIObserver]);
> +  customElements.define("gloda-search", MozGlodaSearch, { extends: "input" });

Thinking more about it, it should be gloda-autocomplete-input.
It's generally nice when the child element name ends with -parent. At least it makes things more clear.

Would also affect the file name and class naming.

	Magnus Melin [:mkmelin] Reporter
	Comment 17 • 5 years ago

Comment on attachment 9093461 [details] [diff] [review]
1542720-dexbl-glodasearch-Part1.patch

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

::: mail/base/content/gloda-autocomplete-input.js
@@ +18,2 @@
>  
> +customElements.whenDefined("autocomplete-input").then(() => {

you shouldn't need the whenDefined, since above one is created

@@ +66,3 @@
>  
> +    connectedCallback() {
> +      super.connectedCallback();

please set the "is" attribute to make sure it's there.

this element should have some protection from running conectedCallback and setting up observers many times

@@ +69,3 @@
>  
> +      // @implements {nsIObserver}
> +      this.prefObserver = {

Please switch this over to using  defineLazyPreferenceGetter

	Magnus Melin [:mkmelin] Reporter
	Comment 21 • 5 years ago

Comment on attachment 9093670 [details] [diff] [review]
1542720-dexbl-glodasearch.patch

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

::: mail/base/content/gloda-autocomplete-input.js
@@ +76,2 @@
>  
> +      XPCOMUtils.defineLazyPreferenceGetter(

you should import XPCOMUtils (or lint isn't happy either?)

@@ +82,5 @@
> +        (pref, oldVal, newVal) => {
> +          if (newVal) {
> +            this.removeAttribute("hidden");
> +          } else {
> +            this.setAttribute("hidden", "hidden");

please use this.toggleAttribute("hidden", !newVal);

@@ +111,5 @@
>  
> +      // @implements {nsIObserver}
> +      this.textObserver = {
> +        observe: (aSubject, aTopic, aData) => {
> +          if (aTopic == "autocomplete-did-enter-text") {

The subject == this check is supposed to make sure this widget doesn't do anything for when text is entered in other autocomplete widgets on the same page. So you should add some check that does that. How, depends on what aSubject is now.

	Alessandro Castellani [:aleca] Assignee
	Comment 23 • 5 years ago

Comment on attachment 9093723 [details] [diff] [review]
1542720-dexbl-glodasearch.patch

Not ready for review as the gloda search is triggered from other autocomplete inputs.

	Pulsebot
	Comment 25 • 5 years ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/95e390183284
temporarily disable Gloda subtest of test-keyboard-interface.js. rs=bustage-fix

	Magnus Melin [:mkmelin] Reporter
	Comment 26 • 5 years ago

Comment on attachment 9093734 [details] [diff] [review]
1542720-dexbl-glodasearch.patch

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

Looks good to me! r=mkmelin

	Pulsebot
	Comment 30 • 5 years ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/395ad4f5654a
Follow-up: Fix linting errors. r=mkmelin