We'd like to experiment with a new awesome bar implementation. To that end we'll need a pref for switching views along with the autocomplete controller.

Comment on attachment 9008387 [details] [diff] [review] patch v2 Review of attachment 9008387 [details] [diff] [review]: ----------------------------------------------------------------- Overall I think the structure looks good. There's a few comments below, but they're mostly minor, so r+, unless you want me to take another look afterwards. It'd be nice to start thinking about the test structure soon before we get too much functionality, so that we can ensure we have the right testability hooks etc. I'm not suggesting it for this patch though - this is a good base to help start the integration and it'll obviously be changing a lot as we move on. ::: browser/base/content/browser.js @@ +260,5 @@ > + > + let element = document.getElementById("urlbar"); > + > + // For now, always use the legacy implementation in the first window to > + // have a usable address bar e.g. for accessing about:config. Nice idea. @@ +269,5 @@ > + > + // Disable the legacy XBL binding. > + element.setAttribute("quantumbar", "true"); > + if (element.hasAttribute("focused")) { > + element.inputField.focus(); Is the focus here a temporary work around whilst we're still flipping back and forth? If so, it might just be worth a comment that we shouldn't need it in future. ::: browser/components/urlbar/UrlbarInput.jsm @@ +7,5 @@ > +var EXPORTED_SYMBOLS = ["UrlbarInput"]; > + > +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm"); > + > +/* eslint-disable require-jsdoc */ As per discussion on irc, lets go with changing the eslint configuration, it was an experiment and obviously isn't going to work well enough for us. If you could change the .eslintrc.js file in the directory to just have "ClassDeclaration" as true, and the other ones as false, then we can drop the disable comment here. @@ +14,5 @@ > + UrlbarView: "resource:///modules/UrlbarView.jsm", > +}); > + > +/** > + * Represents the urlbar textbox. It might be useful to expand this to mention that this behaves as a wrapper for the textbox, and hence exposes the required methods. @@ +36,5 @@ > + let methods = ["addEventListener", "removeEventListener", > + "setAttribute", "hasAttribute", "removeAttribute", "getAttribute", > + "focus", "blur", "select"]; > + let readonlyProperties = ["focused", "inputField", "editor"]; > + let readWriteProperties = ["value", "placeholder", "readOnly", "selectionStart", "selectionEnd"]; nit: maybe make these all constants as that's what they are? @@ +40,5 @@ > + let readWriteProperties = ["value", "placeholder", "readOnly", "selectionStart", "selectionEnd"]; > + > + for (let method of methods) { > + this[method] = (a, b, c, d) => { > + this.textbox[method](a, b, c, d); I think it would be slightly nicer to use spread/rest operators here, then we don't have to worry about the number of arguments. @@ +75,5 @@ > + formatValue() { > + } > + > + /** > + * Handles DOM events from the textbox. Maybe change the comment for the method to something like "Passed DOM events for the textbox to the _on<event type> functions." - then you don't have to read the code to find out. ESLint fails here because `event` isn't documented. Maybe just add a `@param {Event} event` for that without a comment? ::: browser/components/urlbar/UrlbarView.jsm @@ +5,5 @@ > +"use strict"; > + > +var EXPORTED_SYMBOLS = ["UrlbarView"]; > + > +/* eslint-disable require-jsdoc */ This can also be removed per the earlier comment. @@ +13,5 @@ > + */ > +class UrlbarView { > + /** > + * @param {object} urlbar > + * The UrlbarInput instance for this UrlbarView. Rather than `{object}`, you can say `{UrlbarInput}`. @@ +24,5 @@ > + > + this._mainContainer = this.panel.querySelector(".urlbarView-body-inner"); > + this._rows = this.panel.querySelector(".urlbarView-results"); > + > + this._rows.addEventListener("overflow", event => { Maybe add a comment here, something like "Detect overflow/underflows so we can style the results differently."

Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f105be1abab0 Implement basic UrlbarInput and UrlbarView classes and a hidden pref for using them. r=standard8

Backout by dluca@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c288c7cc48 Backed out changeset f105be1abab0 for browser-chrome failures on browser/base/content/test/performance/browser_urlbar_search.js. CLOSED TREE

This seems to do the trick, although I'm somewhat concerned that the browser_urlbar_search.js and browser_urlbar_keyed_search.js failures are essentially intermittent. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6aa636024d84304391f30974b7a02457cfb21b0

Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c324bd6eb4bf Implement basic UrlbarInput and UrlbarView classes and a hidden pref for using them. r=standard8