Closed Bug 1308420 Opened 8 years ago Closed 7 years ago

Move nsBrowserGlue.js _handleURLBarTelemetry() into BrowserUsageTelemetry

Categories

(Firefox :: Address Bar, defect, P3)

Product:
Firefox
Component:
Address Bar
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: gfritzsche, Assigned: standard8)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client:tracking][fxsearch])

Attachments

(1 file)

Bug 1308420 - Move nsBrowserGlue.js _handleURLBarTelemetry() into BrowserUsageTelemetry.
58 bytes, text/x-review-board-request
mak
: review+
Details 
We should migrate this to our new BrowserUsageTelemetry module.
Component: General → Location Bar
Priority: -- → P3
Whiteboard: [measurement:client:tracking] → [measurement:client:tracking][fxsearch]
Assignee: nobody → standard8
Note: there were no tests for the telemetry histograms currently, so I added those before doing the actual move. It isn't testing all the buckets, but it at least tests a couple of them still work.
Comment on attachment 8817605 [details]
Bug 1308420 - Move nsBrowserGlue.js _handleURLBarTelemetry() into BrowserUsageTelemetry.

https://reviewboard.mozilla.org/r/97836/#review99446

::: browser/modules/BrowserUsageTelemetry.jsm:194
(Diff revision 1)
>  };
>  
>  let BrowserUsageTelemetry = {
>    init() {
>      Services.obs.addObserver(this, WINDOWS_RESTORED_TOPIC, false);
> +    Services.obs.addObserver(this, AUTOCOMPLETE_ENTER_TEXT_TOPIC, false);

What about we move the code to a separate listener (similar to URICountListener, something like UrlbarListener) that implements Ci.nsIObserver and Ci.nsISupportsWeakReference.
So here we can add a weakref, and the code is a little bit more modular/separated.

We should probably stop adding all the stuff to BrowserUsageTelemetry and rather make it more like a muxer to separate objects, for readability and cleaner external API.

::: browser/modules/BrowserUsageTelemetry.jsm:479
(Diff revision 1)
> +
> +  /**
> +   * The buckets used for logging telemetry to the FX_URLBAR_SELECTED_RESULT_TYPE
> +   * histogram.
> +   */
> +  URLBAR_SELECTED_RESULT_TYPE_BUCKETS: {

We could maybe shorten this a bit as
URLBAR_SELECTED_RESULT_TYPES
Comment on attachment 8817605 [details]
Bug 1308420 - Move nsBrowserGlue.js _handleURLBarTelemetry() into BrowserUsageTelemetry.

https://reviewboard.mozilla.org/r/97836/#review99446

> What about we move the code to a separate listener (similar to URICountListener, something like UrlbarListener) that implements Ci.nsIObserver and Ci.nsISupportsWeakReference.
> So here we can add a weakref, and the code is a little bit more modular/separated.
> 
> We should probably stop adding all the stuff to BrowserUsageTelemetry and rather make it more like a muxer to separate objects, for readability and cleaner external API.

Good idea, I've seperated that out.
Comment on attachment 8817605 [details]
Bug 1308420 - Move nsBrowserGlue.js _handleURLBarTelemetry() into BrowserUsageTelemetry.

https://reviewboard.mozilla.org/r/97836/#review99888

::: browser/modules/test/browser_UsageTelemetry_content.js:38
(Diff revision 2)
>      Services.search.currentEngine = originalEngine;
>      Services.search.removeEngine(engineDefault);
>      Services.search.removeEngine(engineOneOff);
> +    // Clear history as well.
> +    let sanitizer = new Sanitizer();
> +    yield sanitizer.sanitize(["history"]);

do we need to clear more than history? Cause otherwise this should just be yield PlacesTestUtils.clearHistory(); (sanitize does more that likely we don't need)
You can defineLazyGetterModule PlacesTestUtils in the head file if it's not there already.

::: browser/modules/test/browser_UsageTelemetry_content_aboutHome.js:40
(Diff revision 2)
>      Services.search.currentEngine = originalEngine;
>      Services.search.removeEngine(engineDefault);
>      Services.search.removeEngine(engineOneOff);
> +    // Clear history as well.
> +    let sanitizer = new Sanitizer();
> +    yield sanitizer.sanitize(["history"]);

ditto

::: browser/modules/test/browser_UsageTelemetry_urlbar.js:88
(Diff revision 2)
>      Services.search.removeEngine(engine);
>      Services.prefs.clearUserPref(SUGGEST_URLBAR_PREF, true);
>      Services.prefs.clearUserPref(ONEOFF_URLBAR_PREF);
> +    // Clear history as well.
> +    let sanitizer = new Sanitizer();
> +    yield sanitizer.sanitize(["history"]);

ditto

::: browser/modules/test/head.js:6
(Diff revision 2)
>  Cu.import("resource://gre/modules/Promise.jsm");
>  
> +var tempScope = {};
> +Cc["@mozilla.org/moz/jssubscript-loader;1"].getService(Ci.mozIJSSubScriptLoader)
> +                                           .loadSubScript("chrome://browser/content/sanitize.js", tempScope);
> +var Sanitizer = tempScope.Sanitizer;

yep, let's avoid the sanitizer for now, at least until I find time to complete bug 1167238.
Attachment #8817605 - Flags: review?(mak77) → review+
Comment on attachment 8817605 [details]
Bug 1308420 - Move nsBrowserGlue.js _handleURLBarTelemetry() into BrowserUsageTelemetry.

https://reviewboard.mozilla.org/r/97836/#review99888

> do we need to clear more than history? Cause otherwise this should just be yield PlacesTestUtils.clearHistory(); (sanitize does more that likely we don't need)
> You can defineLazyGetterModule PlacesTestUtils in the head file if it's not there already.

Oh yes, that's much nicer. I hadn't found that option.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7f69f45a0025
Move nsBrowserGlue.js _handleURLBarTelemetry() into BrowserUsageTelemetry. r=mak
Backed out for failing browser_UsageTelemetry_urlbar.js on Linux x64 asan:

https://hg.mozilla.org/integration/autoland/rev/2fecf01d89106376f7791f8eb12f82aba223bdb3

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7f69f45a002534e6089fa9c92ccc8629f3c0de7e
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=8171447&repo=autoland

[task 2016-12-20T11:26:15.140828Z] 11:26:15     INFO - TEST-PASS | browser/modules/test/browser_UsageTelemetry_urlbar.js | browser.engagement.navigation.urlbar must contain the 'search_enter' key. - true == true - 
[task 2016-12-20T11:26:15.143443Z] 11:26:15     INFO - TEST-PASS | browser/modules/test/browser_UsageTelemetry_urlbar.js | 1 - 1 == true - 
[task 2016-12-20T11:26:15.145844Z] 11:26:15     INFO - TEST-PASS | browser/modules/test/browser_UsageTelemetry_urlbar.js | This search must only increment one entry in the scalar. - 1 == 1 - 
[task 2016-12-20T11:26:15.151312Z] 11:26:15     INFO - TEST-PASS | browser/modules/test/browser_UsageTelemetry_urlbar.js | The histogram must contain other-MozSearch.urlbar. - true == true - 
[task 2016-12-20T11:26:15.153920Z] 11:26:15     INFO - TEST-PASS | browser/modules/test/browser_UsageTelemetry_urlbar.js | The key other-MozSearch.urlbar must contain 1. - 1 == 1 - 
[task 2016-12-20T11:26:15.156860Z] 11:26:15     INFO - Buffered messages finished
[task 2016-12-20T11:26:15.159921Z] 11:26:15     INFO - TEST-UNEXPECTED-FAIL | browser/modules/test/browser_UsageTelemetry_urlbar.js | expected counts should match for FX_URLBAR_SELECTED_RESULT_INDEX index 0 - 0 == 1 - JS frame :: chrome://mochitests/content/browser/browser/modules/test/browser_UsageTelemetry_urlbar.js :: checkHistogramResults :: line 17
[task 2016-12-20T11:26:15.163354Z] 11:26:15     INFO - Stack trace:
[task 2016-12-20T11:26:15.165121Z] 11:26:15     INFO -     chrome://mochitests/content/browser/browser/modules/test/browser_UsageTelemetry_urlbar.js:checkHistogramResults:17
[task 2016-12-20T11:26:15.167130Z] 11:26:15     INFO -     chrome://mochitests/content/browser/browser/modules/test/browser_UsageTelemetry_urlbar.js:test_simpleQuery:126
[task 2016-12-20T11:26:15.170006Z] 11:26:15     INFO -     process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:932:23
Flags: needinfo?(standard8)
Not quite sure why just asan failed. I've pushed a try build to see if I can get a little more idea of what's happening:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a6f34a64925eb3395f925ac4e9d4b4beefb2e1c
Flags: needinfo?(standard8)
That failed, so I did some more thinking & investigation. It appears the nightly-asan mozconfigs don't have MOZ_TELEMETRY_REPORTING set, as a result the telemetry part of the test fails.

Additionally, building locally without MOZ_TELEMETRY_REPORTING in the .mozconfig would also fail the test.

I eventually discovered that we need to do `Services.telemetry.canRecordExtended = true;` to get this to work without the option set.
Comment on attachment 8817605 [details]
Bug 1308420 - Move nsBrowserGlue.js _handleURLBarTelemetry() into BrowserUsageTelemetry.

Mak, this is a small additional change, not sure if you want to re-review it or not:

https://reviewboard.mozilla.org/r/97834/diff/3-4/
Attachment #8817605 - Flags: review+ → review?(mak77)
Comment on attachment 8817605 [details]
Bug 1308420 - Move nsBrowserGlue.js _handleURLBarTelemetry() into BrowserUsageTelemetry.

SGTM
Attachment #8817605 - Flags: review?(mak77) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/99365c0b7347
Move nsBrowserGlue.js _handleURLBarTelemetry() into BrowserUsageTelemetry. r=mak
https://hg.mozilla.org/mozilla-central/rev/99365c0b7347
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Blocks: 1780482
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: