Closed Bug 1477985 Opened Last year Closed Last year

Implement pref switch for new awesome bar implementation

Categories

(Firefox :: Address Bar, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file, 3 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Attachment #9008385 - Flags: review?(standard8)
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #9008385 - Attachment is obsolete: true
Attachment #9008385 - Flags: review?(standard8)
Attachment #9008387 - Flags: review?(standard8)
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."
Attachment #9008387 - Flags: review?(standard8) → review+
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #9008387 - Attachment is obsolete: true
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
Flags: needinfo?(dao+bmo)
Attached patch patch v4Splinter Review
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
Attachment #9008504 - Attachment is obsolete: true
Flags: needinfo?(dao+bmo)
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
https://hg.mozilla.org/mozilla-central/rev/c324bd6eb4bf
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.