Include prefill sites as a separate type in BrowserUsageTelemetry for url bar selection types

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Location Bar
P1
normal
RESOLVED FIXED
2 months ago
10 days ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks: 3 bugs)

53 Branch
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox53 unaffected, firefox54 affected, firefox55 fixed)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

https://dxr.mozilla.org/mozilla-central/rev/6d38ad302429c98115c354d643e81987ecec5d3c/browser/modules/BrowserUsageTelemetry.jsm#56-68

doesn't have a separate entry for prefill sites. It probably should so we know how often people interact with this type of entry.
Comment hidden (mozreview-request)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
status-firefox53: --- → unaffected
status-firefox54: --- → affected
status-firefox55: --- → affected
Blocks: 1349557

Comment 2

a month ago
mozreview-review
Comment on attachment 8850010 [details]
Bug 1347271 - log preloaded top sites in telemetry and update their naming,

https://reviewboard.mozilla.org/r/122762/#review125466

::: browser/modules/BrowserUsageTelemetry.jsm:254
(Diff revision 1)
>            "searchsuggestion" :
>          action.type;
>      }
>      if (!actionType) {
>        let styles = new Set(controller.getStyleAt(idx).split(/\s+/));
> -      let style = ["autofill", "tag", "bookmark"].find(s => styles.has(s));
> +      let style = ["autofill", "tag", "bookmark", "prefill-site"].find(s => styles.has(s));

this won't account for autofilled entries, that I expect being the most common ones.
I'm not sure whether it's worth to distinguish between prefill-autofill or prefill-history, but in any case we should add the prefill also to the autofill entry, and here put it first so it's catched before autofill.
The question whether it's worth to distinguish the  cases may be a good one for our analyst, Dave Zeber.

FWIW, I'd probably name the style and these entries "preseeded" to avoid any possible confusion with autofill.
Attachment #8850010 - Flags: review?(mak77) → review-
Dave, do you think it's worth to distinguish if the preseeded entry was inserted as "autofill" (directly in the locationbar field) or as a normal history entry, or is it fine to just keep a single count?
Flags: needinfo?(dzeber)
Comment hidden (mozreview-request)

Updated

a month ago
Priority: -- → P1

Comment 5

25 days ago
I don't think it's necessary to distinguish between autofill and history at this point, so long as we count all uses of the preseeded results.

However, I didn't really understand comment 2 about how autofill will behave in this case. If the user presses <Enter> on an autofill from a preseeded result, does than increment both |"preloaded-top-site"| and |"autofill"|? Or are the |URLBAR_SELECTED_RESULT_TYPES| mutually exclusive?
Flags: needinfo?(dzeber) → needinfo?(mak77)
(In reply to Dave Zeber [:dzeber] from comment #5)
> I don't think it's necessary to distinguish between autofill and history at
> this point, so long as we count all uses of the preseeded results.

ok

> However, I didn't really understand comment 2 about how autofill will behave
> in this case. If the user presses <Enter> on an autofill from a preseeded
> result, does than increment both |"preloaded-top-site"| and |"autofill"|? Or
> are the |URLBAR_SELECTED_RESULT_TYPES| mutually exclusive?

They are mutually exclusive, so we can keep a global count summing all of them and percentages make sense.
Is this ok?
Flags: needinfo?(mak77) → needinfo?(dzeber)

Comment 7

24 days ago
mozreview-review
Comment on attachment 8850010 [details]
Bug 1347271 - log preloaded top sites in telemetry and update their naming,

https://reviewboard.mozilla.org/r/122762/#review129338

r=me with the test fix.

I think the mutually exclusiveness of probes makes sense to be able to calculate sums and percentages, we can always change it later if it won't fix, no reason to keep this from landing as-is.

::: commit-message-8d967:1
(Diff revision 2)
> +Bug 1347271 - log prefill sites in telemetry, r?mak

maybe you want to also update the commit message to preloaded vs prefill

::: toolkit/components/places/mozIPlacesAutoComplete.idl:145
(Diff revision 2)
> -   * Populate list of Prefill Sites from JSON.
> +   * Populate list of Preloaded Sites from JSON.
>     *
>     * @param sites
>     *        Array of [url,title] to populate from.
>     */
> -  void populatePrefillSiteStorage(in jsval sites);
> +  void populatePreloadedSiteStorage(in jsval sites);

Then you should fix test_prefill_sites.js, since it uses this API (that test is the only reason this API exists...)
Attachment #8850010 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)

Comment 9

24 days ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b21b040e37b0
log preloaded top sites in telemetry and update their naming, r=mak
Whiteboard: [fxsearch]

Comment 10

23 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b21b040e37b0
Status: ASSIGNED → RESOLVED
Last Resolved: 23 days ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(In reply to Marco Bonardo [::mak] from comment #6)
> They are mutually exclusive, so we can keep a global count summing all of
> them and percentages make sense.
> Is this ok?

Yes, I agree that makes sense, and that's the behaviour I'd expect.
Flags: needinfo?(dzeber)
Duplicate of this bug: 1354329
Blocks: 1354264
Flags: qe-verify+
QA Contact: brindusa.tot
You need to log in before you can comment on or make changes to this bug.