Closed Bug 1347271 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Address Bar, enhancement, P1)

53 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox53 --- unaffected
firefox54 --- fix-optional
firefox55 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

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.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Blocks: 1349557
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)
Priority: -- → P1
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 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+
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]
https://hg.mozilla.org/mozilla-central/rev/b21b040e37b0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
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)
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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.