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
5 months ago
3 months 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 fix-optional, firefox55 fixed)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

5 months ago
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)

Updated

5 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Updated

5 months ago
status-firefox53: --- → unaffected
status-firefox54: --- → affected
status-firefox55: --- → affected
(Assignee)

Updated

5 months ago
Blocks: 1349557

Comment 2

5 months 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)
Priority: -- → P1

Comment 5

5 months 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

5 months 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

5 months 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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b21b040e37b0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 11

4 months ago
(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)

Updated

4 months ago
Duplicate of this bug: 1354329

Updated

4 months ago
Blocks: 1354264
Flags: qe-verify+
QA Contact: brindusa.tot
I'm assuming that it's OK to have this change go to release with 55. If you want it uplifted to 54 this is just about the last minute to request uplift.
status-firefox54: affected → fix-optional
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 14

3 months ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #13)
> I'm assuming that it's OK to have this change go to release with 55. If you
> want it uplifted to 54 this is just about the last minute to request uplift.

Yep, I think that's fine.
Flags: needinfo?(gijskruitbosch+bugs)
You need to log in before you can comment on or make changes to this bug.