1308419 - Clean up "keyword-search" Telemetry

Status: NEW

(Firefox :: Address Bar, defect, P3)

Description

[Georg Fritzsche [:gfritzsche]]

This is only handling Telemetry:
https://dxr.mozilla.org/mozilla-central/rev/da986c9f1f723af1e0c44f4ccd4cddd5fb6084e8/browser/components/nsBrowserGlue.js#326

According to mak, this might actually be not needed anymore:
> the current urlbar is supposed to always know what will happen when pressing enter
> that's why I suspect "keyword-search" may be double counting

We should at least move it to BrowserUsageTelemetry.jsm, to consolidate this kind of code.
We also need to check whether this is double-counting now.

Comment 1

[Mark Banner (:standard8)]

Notes to self:

- Probably need test in browser_UsageTelemetry_urlbar.js for keyword-search, or a new file based on that one.
- browser/base/content/test/urlbar/browser_action_keyword.js may be useful for constructing the keyword part.
- Might also be worth testing manually.

Comment 2

[Mark Banner (:standard8)]

Marco, I'm trying to write a basic test for this, however the keyword-search event never seems to get called:

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/browser/components/nsBrowserGlue.js#326

I've tried this manually in the UI as well. I set some logging and a breakpoint but that code never gets hit. This is what I'm doing:

- Visit twitter or bbc.co.uk
- Right-click the search field and select to add a keyword search. Add a keyword (e.g. bbcs) in the bookmarks box.
- Open a new tab, enter "bbcs f1"

=> Breakpoint doesn't get hit, and logging is not there.

e10s vs non-e10s doesn't affect the result.

Comment 3

[Alessio Placitelli [:Dexter]]

(In reply to Mark Banner (:standard8, limited time in Dec) from comment #2)
> Marco, I'm trying to write a basic test for this, however the keyword-search
> event never seems to get called:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/browser/components/nsBrowserGlue.
> js#326

Are you building from mozilla-central? If so make sure to add MOZ_TELEMETRY_REPORTING to your mozconfig, otherwise this part won't be enabled. See bug 1312182 for details.

Comment 4

[Mark Banner (:standard8)]

(In reply to Alessio Placitelli [:Dexter] from comment #3)
> Are you building from mozilla-central? If so make sure to add
> MOZ_TELEMETRY_REPORTING to your mozconfig, otherwise this part won't be
> enabled. See bug 1312182 for details.

Thank you, that definitely gets it registered.

It appears we don't log telemetry for bookmark based keyword searches, but we do for the search engines defined in the browser - I've got the latter working now.

At this time, I'm not quite certain the code that triggers the keyword-search event from via nsDocShell is working 100%, it may be unused. However, I need to dig into that more confirm if my suspicions are correct or not.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Mark Banner (:standard8, limited time in Dec) from comment #4)
> At this time, I'm not quite certain the code that triggers the
> keyword-search event from via nsDocShell is working 100%, it may be unused.
> However, I need to dig into that more confirm if my suspicions are correct
> or not.

The fact is that before we were relying on the docshell to tell whether something ended up being a search or a visit, and thus it made sense for it to report that telemetry.
Unified complete instead asks to the URIFixup first, and returns an appropriate moz-action uri that already tells whether we search or visit. So I'd expect us to not go through the keyword-search thing anymore.
My assumption is that we should be able to remove that code, but we must check that it's really never touched anymore.

Comment 6

[Mark Banner (:standard8)]

I have found that we go through the keyword-search event for the case where a search is copied & pasted into the url bar with "Paste & Go".

I'm looking into some tests to cover that case.

Comment 7

[Marco Bonardo [:mak]]

(In reply to Mark Banner (:standard8, limited time in Dec) from comment #6)
> I have found that we go through the keyword-search event for the case where
> a search is copied & pasted into the url bar with "Paste & Go".
> 
> I'm looking into some tests to cover that case.

Ah yes, we do that cause we don't actually run an autocomplete search at all in such a case. We just set the value and handle Enter, that passes the value through getShortcutOrURIAndPostData and that will do some resolution of it (only keyword or alias resolution).
The automated tests basically do the same, many just set textValue and simulate Enter.
We could remove that code path by issuing a search and handling enter once it returns the first entry, it may add some complication to the code though and thus it still goes through the legacy path.

That said, I'm not sure it would be worth keeping that telemetry if itàs just for paste & go, the usage of keywords and aliases is quite low, if we multipy that by the probability the user may do that through paste & go... it likely covers a micro-tiny-ignorable fraction of the searches.

Comment 8

[Mark Banner (:standard8)]

Realistically, I don't think I'm going to work on this in the short term.