Alessio Placitelli [:Dexter] Assignee
	Description • 9 years ago

This bug is about removing services/datarporting from Firefox codebase.

We should also investigate (and remove) uses of "@mozilla.org/datareporting/service;1".

	Alessio Placitelli [:Dexter] Assignee
	Comment 4 • 9 years ago

Comment on attachment 8701840 [details] [diff] [review]
part 1 - Remove services/datareporting

I did my best to make small, self-contained patches: I hope this helps.

	Alessio Placitelli [:Dexter] Assignee
	Comment 5 • 9 years ago

Comment on attachment 8701842 [details] [diff] [review]
part 2 - Don't use the DRS from other components

The change in "services/healthreport/healthreporter.jsm" is only to make this bug self-contained. Ideally, this will land together with bug 1234526.

	Alessio Placitelli [:Dexter] Assignee
	Comment 6 • 9 years ago

Comment on attachment 8701840 [details] [diff] [review]
part 1 - Remove services/datareporting

Bouncing to :rnewman as :gps is not around.

	Georg Fritzsche [:gfritzsche]
	Comment 7 • 9 years ago

Comment on attachment 8701840 [details] [diff] [review]
part 1 - Remove services/datareporting

Review of attachment 8701840 [details] [diff] [review]:
-----------------------------------------------------------------

This sets up the Services.DataReporting logger:
https://dxr.mozilla.org/mozilla-central/rev/0771c5eab32f0cee4f7d12bc382298a81e0eabb2/services/datareporting/DataReportingService.js#247
... which affects code that will stay around:
https://dxr.mozilla.org/mozilla-central/search?q=Services.DataReporting.&redirect=false&case=true
What is the plan here? Use a Telemetry logger?

Can't we remove the datareporting.policy.dataSubmissionEnabled.v2 pref now?

	Georg Fritzsche [:gfritzsche]
	Comment 8 • 9 years ago

Comment on attachment 8701842 [details] [diff] [review]
part 2 - Don't use the DRS from other components

Review of attachment 8701842 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +3708,5 @@
>     *        item was in the suggestion list and how the user selected it.
>     */
>    recordSearchInHealthReport: function (engine, source, selection) {
>      BrowserUITelemetry.countSearchEvent(source, null, selection);
>      this.recordSearchInTelemetry(engine, source);

We don't need a "recordSearchInHealthReport" function now - lets consolidate in recordSearchInTelemetry().

::: browser/components/preferences/in-content/advanced.js
@@ +295,5 @@
>        checkbox.setAttribute("disabled", "true");
>        return;
>      }
>  
> +    checkbox.checked = Services.prefs.getBoolPref("datareporting.healthreport.uploadEnabled");

Lets not repeat the string and make it a const.

	Alessio Placitelli [:Dexter] Assignee
	Comment 9 • 9 years ago

(In reply to Georg Fritzsche [:gfritzsche] [away until Jan 3] from comment #7)
> Comment on attachment 8701840 [details] [diff] [review]
> part 1 - Remove services/datareporting
> 
> Review of attachment 8701840 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This sets up the Services.DataReporting logger:
> https://dxr.mozilla.org/mozilla-central/rev/
> 0771c5eab32f0cee4f7d12bc382298a81e0eabb2/services/datareporting/
> DataReportingService.js#247
> ... which affects code that will stay around:
> https://dxr.mozilla.org/mozilla-central/search?q=Services.DataReporting.
> &redirect=false&case=true
> What is the plan here? Use a Telemetry logger?
> 
> Can't we remove the datareporting.policy.dataSubmissionEnabled.v2 pref now?

Good catch with the loggers, I overlooked the Infobar and the SessionRecorder. Telemetry is the main consumer for both of those, IMHO it makes sense to use the Telemetry logger there.

I was planning to clean-up datareporting-prefs.js in a separate patch, but it's ok to do that here.

	Georg Fritzsche [:gfritzsche]
	Comment 10 • 9 years ago

Comment on attachment 8701843 [details] [diff] [review]
part 3 - Fix or remove tests using the DRS

Review of attachment 8701843 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_aboutHome.js
@@ -80,5 @@
>  
> -// Disabled on Linux for intermittent issues with FHR, see Bug 945667.
> -{
> -  desc: "Check that performing a search fires a search event and records to " +
> -        "Firefox Health Report.",

Do we have test coverage for this with Telemetry?
If it's easy to update to check Telemetry, lets just do that.
If not, we need at least a bug about test coverage for this.

::: browser/base/content/test/general/browser_contextSearchTabPosition.js
@@ -1,5 @@
> -/* This Source Code Form is subject to the terms of the Mozilla Public
> - * License, v. 2.0. If a copy of the MPL was not distributed with this
> - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> -
> -function test() {

Same question here about test coverage.

::: browser/base/content/test/general/browser_urlbar_search_healthreport.js
@@ -2,5 @@
> - * http://creativecommons.org/publicdomain/zero/1.0/ */
> -
> -"use strict";
> -
> -add_task(function* test_healthreport_search_recording() {

Do we have equivalent coverage for search Telemetry?

::: browser/base/content/test/general/healthreport_testRemoteCommands.html
@@ +7,5 @@
>  <script type="application/javascript;version=1.7">
>  
>  function init() {
> +  window.addEventListener("message", doTest, false);
> +  doTest();

doTest() is already used as the callback above, you shouldn't call it explicitly here.

::: browser/components/preferences/in-content/tests/browser.ini
@@ +17,5 @@
>  [browser_connection.js]
>  [browser_connection_bug388287.js]
>  [browser_cookies_exceptions.js]
>  [browser_healthreport.js]
> +skip-if = true || !healthreport # Bug 1185403 for the "true"

What is this change for?

::: browser/components/search/test/browser_healthreport.js
@@ -2,5 @@
> - * http://creativecommons.org/publicdomain/zero/1.0/ */
> -
> -"use strict";
> -
> -function test() {

Does this have equivalent Telemetry test coverage?

::: services/healthreport/modules-testing/utils.jsm
@@ +188,5 @@
>  
>    let reporter;
>  
>    let policyPrefs = new Preferences(branch + "policy.");
> +  /*let listener = new MockPolicyListener();

Why comment out? Why not just remove this part?

	Alessio Placitelli [:Dexter] Assignee
	Comment 14 • 9 years ago

(In reply to Georg Fritzsche [:gfritzsche] [away until Jan 3] from comment #10)
> Comment on attachment 8701843 [details] [diff] [review]
> part 3 - Fix or remove tests using the DRS
> 
> Review of attachment 8701843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/test/general/browser_aboutHome.js
> @@ -80,5 @@
> >  
> > -// Disabled on Linux for intermittent issues with FHR, see Bug 945667.
> > -{
> > -  desc: "Check that performing a search fires a search event and records to " +
> > -        "Firefox Health Report.",
> 
> Do we have test coverage for this with Telemetry?
> If it's easy to update to check Telemetry, lets just do that.
> If not, we need at least a bug about test coverage for this.

I've updated this test to provide coverage for Telemetry, which was missing :-(

> ::: browser/base/content/test/general/browser_contextSearchTabPosition.js
> @@ -1,5 @@
> > -/* This Source Code Form is subject to the terms of the Mozilla Public
> > - * License, v. 2.0. If a copy of the MPL was not distributed with this
> > - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > -
> > -function test() {
> 
> Same question here about test coverage.

Restored and updated to Telemetry.

> ::: browser/base/content/test/general/browser_urlbar_search_healthreport.js
> @@ -2,5 @@
> > - * http://creativecommons.org/publicdomain/zero/1.0/ */
> > -
> > -"use strict";
> > -
> > -add_task(function* test_healthreport_search_recording() {
> 
> Do we have equivalent coverage for search Telemetry?

We do, it's https://dxr.mozilla.org/mozilla-central/rev/0771c5eab32f0cee4f7d12bc382298a81e0eabb2/browser/base/content/test/general/browser_urlbarSearchTelemetry.js

> ::: browser/base/content/test/general/healthreport_testRemoteCommands.html
> @@ +7,5 @@
> >  <script type="application/javascript;version=1.7">
> >  
> >  function init() {
> > +  window.addEventListener("message", doTest, false);
> > +  doTest();
> 
> doTest() is already used as the callback above, you shouldn't call it
> explicitly here.

I've retained the old behaviour here, nothing really changed: if |doTest()| is not called, the first test doesn't get triggered.

The only real difference there, is that FHR is no longer sending the first payload message when about:healthreport is opened. That's why I've dropped the first listener.

> ::: browser/components/preferences/in-content/tests/browser.ini
> @@ +17,5 @@
> >  [browser_connection.js]
> >  [browser_connection_bug388287.js]
> >  [browser_cookies_exceptions.js]
> >  [browser_healthreport.js]
> > +skip-if = true || !healthreport # Bug 1185403 for the "true"
> 
> What is this change for?

This test was disabled on Linux due to FHR making it "orange". Now that FHR is gone, I think it would be a good idea to restore it. I've tested it locally in a tight loop, and doesn't *seem* to fail.

> ::: browser/components/search/test/browser_healthreport.js
> @@ -2,5 @@
> > - * http://creativecommons.org/publicdomain/zero/1.0/ */
> > -
> > -"use strict";
> > -
> > -function test() {
> 
> Does this have equivalent Telemetry test coverage?

Restored and adapted to Telemetry.

> ::: services/healthreport/modules-testing/utils.jsm
> @@ +188,5 @@
> >  
> >    let reporter;
> >  
> >    let policyPrefs = new Preferences(branch + "policy.");
> > +  /*let listener = new MockPolicyListener();
> 
> Why comment out? Why not just remove this part?

Good point, done.

	Georg Fritzsche [:gfritzsche]
	Comment 15 • 9 years ago

Comment on attachment 8704091 [details] [diff] [review]
part 3 - Fix or remove tests using the DRS

Review of attachment 8704091 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_aboutHome.js
@@ +96,5 @@
>      let numSearchesBefore = 0;
>      let searchEventDeferred = Promise.defer();
>      let doc = gBrowser.contentDocument;
> +    is(engine.name, gBrowser.contentWindow.wrappedJSObject.gContentSearchController.defaultEngine.name,
> +       "Engine name in DOM should match engine we just added");

Why this change? I think this was more readable before.

::: browser/base/content/test/general/browser_contextSearchTabPosition.js
@@ +38,5 @@
>    BrowserSearch.loadSearchFromContext("mozilla");
>    BrowserSearch.loadSearchFromContext("firefox");
>  
> +  // Wait for all the tabs to open.
> +  yield tabsLoadedDeferred.promise;

I wonder if there is already a suitable promise*() function in browser/base/content/test/general/head.js instead of this manual tracking.

	Georg Fritzsche [:gfritzsche]
	Comment 16 • 9 years ago

Comment on attachment 8704090 [details] [diff] [review]
part 2 - Don't use the DRS from other components

Review of attachment 8704090 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +3692,5 @@
>      openUILinkIn(searchEnginesURL, where);
>    },
>  
> +  _getSearchEngineId: function (engine) {
> +    if (!engine) {

Can we add a check for !("name" in engine) or the name property being undefined?
This looks like a footgun that could skew search results strangely.

	Alessio Placitelli [:Dexter] Assignee
	Comment 19 • 9 years ago

(In reply to Georg Fritzsche [:gfritzsche] [away until Jan 3] from comment #15)
> ::: browser/base/content/test/general/browser_contextSearchTabPosition.js
> @@ +38,5 @@
> >    BrowserSearch.loadSearchFromContext("mozilla");
> >    BrowserSearch.loadSearchFromContext("firefox");
> >  
> > +  // Wait for all the tabs to open.
> > +  yield tabsLoadedDeferred.promise;
> 
> I wonder if there is already a suitable promise*() function in
> browser/base/content/test/general/head.js instead of this manual tracking.

Due to the use of BrowserSearch, I couldn't find any non-trivial way to do that using the stuff from head.js.

	Alessio Placitelli [:Dexter] Assignee
	Comment 20 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e0b62d85cb8

	Alessio Placitelli [:Dexter] Assignee
	Comment 21 • 9 years ago

Comment on attachment 8704151 [details] [diff] [review]
part 2 - Don't use the DRS from other components

Review of attachment 8704151 [details] [diff] [review]:
-----------------------------------------------------------------

:smaug, we're removing FHR/DRS from Firefox so there should not be any use in keeping the entry in the WebIDL. Would you mind reviewing the changes to MozSelfSupport.webidl?

	Alessio Placitelli [:Dexter] Assignee
	Comment 22 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/3e7dc6d87325ce8f6d817e7f0bad096152cc8aef
Bug 1234522 - Remove services/datareporting. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/c3b6bf176f86b0167f008fbf87e5a2aa39061584
Bug 1234522 - Remove references to the data reporting service. r=gfritzsche,smaug

https://hg.mozilla.org/integration/fx-team/rev/96e0326e798579fda3e4babf0a253b43e19e2635
Bug 1234522 - Fix or remove the tests relying on the data reporting service. r=gfritzsche

	Phil Ringnalda (:philor)
	Comment 23 • 9 years ago

Backed out the whole push in https://hg.mozilla.org/integration/fx-team/rev/19a2342819e4 for the rather odd result of https://treeherder.mozilla.org/#/jobs?repo=fx-team&fromchange=4d4e15151ce3&group_state=expanded&filter-searchStr=7a763e435f27ed8b65c60bee533835959619bd63&tochange=77c75e5b4df1 - something in there made OS X 10.10 debug xpcshell time out in either test_ocsp_stapling.js or test_ocsp_stapling_expired.js nearly all the time.

	Phil Ringnalda (:philor)
	Comment 24 • 9 years ago

Sorry, forgot the bit about how you get 10.10 on try, since it's currently not run by default while we do some thrashing around with a hardware shortage - try: -b d -p macosx64 -u xpcshell[10.10] -t none

	Alessio Placitelli [:Dexter] Assignee
	Comment 25 • 9 years ago

(In reply to Phil Ringnalda (:philor) from comment #24)
> Sorry, forgot the bit about how you get 10.10 on try, since it's currently
> not run by default while we do some thrashing around with a hardware
> shortage - try: -b d -p macosx64 -u xpcshell[10.10] -t none

Thanks!

	Alessio Placitelli [:Dexter] Assignee
	Comment 26 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/d45c6bd25021b298f4bf8231a5f8b50b8011145c
Bug 1234522 - Remove services/datareporting. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/07dec91ebe36e59a4a440916d4cf3bd372f700bd
Bug 1234522 - Remove references to the data reporting service. r=gfritzsche,smaug

https://hg.mozilla.org/integration/fx-team/rev/75926f6418190b471e7b1f1e05e8976f96283ec7
Bug 1234522 - Fix or remove the tests relying on the data reporting service. r=gfritzsche

	Alessio Placitelli [:Dexter] Assignee
	Comment 27 • 9 years ago

(In reply to Phil Ringnalda (:philor) from comment #23)
> Backed out the whole push in
> https://hg.mozilla.org/integration/fx-team/rev/19a2342819e4 for the rather
> odd result of
> https://treeherder.mozilla.org/#/jobs?repo=fx-
> team&fromchange=4d4e15151ce3&group_state=expanded&filter-
> searchStr=7a763e435f27ed8b65c60bee533835959619bd63&tochange=77c75e5b4df1 -
> something in there made OS X 10.10 debug xpcshell time out in either
> test_ocsp_stapling.js or test_ocsp_stapling_expired.js nearly all the time.

Here comes the fun part. Try push right after the backout:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9f1f05a1ae8

Try push today (same stack, just rebased and with the bug numbers removed from the commit message):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63f3d8380132&selectedJob=15278855

To be 100% safe, I incrementally tested each patch in the stack (the initial X bustages are due to an AWS connectivity issue on Sunday):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b55a4cc20e39
https://treeherder.mozilla.org/#/jobs?repo=try&revision=52b0d6d759eb
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d96e0fd5b4b9

Everything seems green, the bustages from the other look unrelated. I'm landing those again, keeping an eye on them.

	Carsten Book [:Tomcat]
	Comment 28 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d45c6bd25021
https://hg.mozilla.org/mozilla-central/rev/07dec91ebe36
https://hg.mozilla.org/mozilla-central/rev/75926f641819