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)
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
status-firefox53:
--- → unaffected
status-firefox54:
--- → affected
status-firefox55:
--- → affected
Comment 2•7 years 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-
Comment 3•7 years ago
|
||
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•7 years ago
|
Priority: -- → P1
Comment 5•7 years 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)
Comment 6•7 years ago
|
||
(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•7 years 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) |
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
Updated•7 years ago
|
Whiteboard: [fxsearch]
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b21b040e37b0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment 11•7 years 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•7 years ago
|
Flags: qe-verify+
QA Contact: brindusa.tot
Comment 13•7 years ago
|
||
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)
Assignee | ||
Comment 14•7 years 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.
Description
•