1496743 - Stop logging telemetry when pressing enter on the one-off search settings button in the urlbar

Status: RESOLVED FIXED

(Firefox :: Address Bar, enhancement)

Description

[Mark Banner (:standard8) (afk until 12 Apr)]

In the urlbarBindings in handleCommand, we have this code:

```

// Determine whether to use the selected one-off search button.  In
// one-off search buttons parlance, "selected" means that the button
// has been navigated to via the keyboard.  So we want to use it if
// the triggering event is not a mouse click -- i.e., it's a Return
// key -- or if the one-off was mouse-clicked.
let selectedOneOff = this.popup.oneOffSearchButtons.selectedButton;
if (selectedOneOff &&
    isMouseEvent &&
    event.originalTarget != selectedOneOff) {
  selectedOneOff = null;
}

// Do the command of the selected one-off if it's not an engine.
if (selectedOneOff && !selectedOneOff.engine) {
  selectedOneOff.doCommand();
  return;
}

```

I've done some digging around and I just can't see how that second part is triggered - the preferences button on the one-offs bar doesn't get to the handleCommand method, and the right-click menu is doesn't trigger it either.

Try also shows that mochitests pass without it:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d63853d6ec7819dc559cec59afa0f154f0a98b3b

Therefore, I think it can be removed, which will make the code slightly simpler and make it easier to figure out what to do with the new address bar implementation.

I'll attach a patch, Drew please let me know if you think I'm wrong.

Comment 1

[Mark Banner (:standard8) (afk until 12 Apr)]

Attachment: Bug 1496743 - Remove unnecessary search command fallback from urlbarBindings#handleCommands.

Comment 2

[Drew Willcoxon :adw]

The second part is hit when you press the return key while the preferences button is selected.

Comment 3

[Drew Willcoxon :adw]

(In reply to Mark Banner (:standard8) from comment #0)
> Try also shows that mochitests pass without it:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d63853d6ec7819dc559cec59afa0f154f0a98b3b

It's possible we don't have a test that specifically synthesizes an enter keypress on the settings button.

Comment 4

[Mark Banner (:standard8) (afk until 12 Apr)]

(In reply to Drew Willcoxon :adw from comment #3)
> (In reply to Mark Banner (:standard8) from comment #0)
> > Try also shows that mochitests pass without it:
> > 
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=d63853d6ec7819dc559cec59afa0f154f0a98b3b
> 
> It's possible we don't have a test that specifically synthesizes an enter
> keypress on the settings button.

It certainly looks like it.

Do you know why we add a telemetry result to "FX_URLBAR_SELECTED_RESULT_METHOD" for category "enter" when this happens? We don't appear to do that for a click.

Could there be something else this is handling which would need to be logged as well? The other one-off buttons don't hit this case.

If we just need to re-route the command, and not log telemetry, this would make it easier for the newer urlbar design.

Comment 5

[Drew Willcoxon :adw]

The fact that telemetry is recorded when you hit the return key on the settings button is just an accident I think, due to the fact that the return keypress gets routed through handleCommand.  I don't think we really need to record that.

Comment 6

[Mark Banner (:standard8) (afk until 12 Apr)]

Ok, lets mutate this into not logging that telemetry, and I'll also add test coverage for the settings button.

Comment 7

[Mark Banner (:standard8) (afk until 12 Apr)]

Attachment: Bug 1496743 - Stop logging telemetry when pressing enter on the one-off search settings button in the urlbar.

Also add tests for the search settings button in the urlbar.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/107392a74e8a
Stop logging telemetry when pressing enter on the one-off search settings button in the urlbar. r=adw

Comment 9

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/107392a74e8a