Closed
Bug 1101491
Opened 10 years ago
Closed 9 years ago
SEARCH_DEFAULT_ENGINE probe only set during idle-daily!
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: vladan, Assigned: gfritzsche)
References
Details
Attachments
(1 file, 1 obsolete file)
6.24 KB,
patch
|
mconley
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
It looks like SEARCH_DEFAULT_ENGINE is only filled in during the "gather-telemetry" event which I believe is only fired after "idle-daily". This means that only one session during a 24 hour period will report the SEARCH_DEFAULT_ENGINE value.
Reporter | ||
Comment 1•10 years ago
|
||
I believe other Telemetry probes also incorrectly depend on "gather-telemetry".. I will file other bugs once this one is confirmed
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → georg.fritzsche
Assignee | ||
Comment 2•9 years ago
|
||
Hm, ok :-/ We could just see that we submit this on (or close to) startup. Sound reasonable?
Assignee | ||
Comment 3•9 years ago
|
||
Blake, does this seems reasonable? Do you happen to know if we could have perf issues with touching the search service too early?
Attachment #8534994 -
Flags: review?(bwinton)
Comment 4•9 years ago
|
||
Comment on attachment 8534994 [details] [diff] [review] Submit default search engine on startup too It seems okay to me, the one thing I'm worried about is that the search service might not be initialized at this point. Should we wait for browser-search-service's init-complete notification (sent from http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#2989 and http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#3013)? (You could try pinging florian or mconley for the answer to that question…)
Attachment #8534994 -
Flags: review?(bwinton) → review+
Updated•9 years ago
|
Attachment #8534994 -
Flags: review?(mconley)
Assignee | ||
Comment 5•9 years ago
|
||
Hm, or, if search service init is a concern... Maybe it makes more sense to move over to nsBrowserGlue.js and just hang off "browser-search-engine-modified" and "browser-search-service" here: http://hg.mozilla.org/mozilla-central/annotate/0cf461e62ce5/browser/components/nsBrowserGlue.js#l390
Assignee | ||
Comment 6•9 years ago
|
||
Florian or MattN, can you comment on comment 4 and comment 5 so i can do something here this week?
Flags: needinfo?(florian)
Flags: needinfo?(MattN+bmo)
Comment 7•9 years ago
|
||
Comment on attachment 8534994 [details] [diff] [review] Submit default search engine on startup too Review of attachment 8534994 [details] [diff] [review]: ----------------------------------------------------------------- I agree that it makes more sense to collect this information in nsBrowserGlue in those observer notification handlers.
Attachment #8534994 -
Flags: review?(mconley)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(florian)
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 8•9 years ago
|
||
Fixed this to hang off the referenced search service events.
Attachment #8534994 -
Attachment is obsolete: true
Attachment #8537247 -
Flags: review?(mconley)
Comment 9•9 years ago
|
||
Comment on attachment 8537247 [details] [diff] [review] Submit default search engine on search service events Review of attachment 8537247 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the bracing changes reverted. ::: browser/components/nsBrowserGlue.js @@ +419,5 @@ > // initialized already when this notification fires. > let ss = Services.search; > if (ss.currentEngine.name == ss.defaultEngine.name) > return; > + if (data == "engine-current") { I understand the impulse to brace these - my instinct says the same thing. Unfortunately, the rest of this file uses the "no braces for one-liner blocks" convention, and we should probably stick to that convention (or change the entire file).
Attachment #8537247 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #9) > I understand the impulse to brace these - my instinct says the same thing. > Unfortunately, the rest of this file uses the "no braces for one-liner > blocks" convention, and we should probably stick to that convention (or > change the entire file). A little sad, but fair point.
Status: NEW → ASSIGNED
Assignee | ||
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba715f80ac5f
Assignee | ||
Comment 12•9 years ago
|
||
Benjamin, do we still have a need to uplift this to at least aurora?
Flags: needinfo?(benjamin)
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8537247 [details] [diff] [review] Submit default search engine on search service events Approval Request Comment [Feature/regressing bug #]: Recording current search engine measurement in Telemetry. [User impact if declined]: No useful search engine telemetry. [Describe test coverage new/current, TBPL]: Manually verified, automated tests still wait for having time for them. [Risks and why]: Low-risk, small change that just moves search telemetry around to a properly working place. [String/UUID change made/needed]: None.
Attachment #8537247 -
Flags: approval-mozilla-aurora?
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ba715f80ac5f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Updated•9 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → fixed
Updated•9 years ago
|
Attachment #8537247 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•