Closed Bug 1101491 Opened 10 years ago Closed 9 years ago

SEARCH_DEFAULT_ENGINE probe only set during idle-daily!

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: vladan, Assigned: gfritzsche)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
I believe other Telemetry probes also incorrectly depend on "gather-telemetry".. I will file other bugs once this one is confirmed
Blocks: 1105864
Assignee: nobody → georg.fritzsche
Hm, ok :-/
We could just see that we submit this on (or close to) startup. Sound reasonable?
Blocks: 1089670
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 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+
Attachment #8534994 - Flags: review?(mconley)
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
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 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)
Flags: needinfo?(florian)
Flags: needinfo?(MattN+bmo)
Fixed this to hang off the referenced search service events.
Attachment #8534994 - Attachment is obsolete: true
Attachment #8537247 - Flags: review?(mconley)
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+
(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
Benjamin, do we still have a need to uplift this to at least aurora?
Flags: needinfo?(benjamin)
Yes please.
Flags: needinfo?(benjamin)
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?
https://hg.mozilla.org/mozilla-central/rev/ba715f80ac5f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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.