Since nsITelemetryPing is only ever used from JavaScript, we should migrate this to a jsm. This will make our life easier.

This patch does the following: - move the code of TelemetryPing.js into TelemetryPing.jsm.in; - expose in TelemetryPing.jsm.in the data that was otherwise obtained by clients by loading TelemetryPing.js using the subscript loader; - porting clients/tests that used directly TelemetryPing.js to TelemetryPing.jsm; - porting clients/tests that used the nsITelemetryPing component to TelemetryPing.jsm; - moving the .jsm files from telemetry to telemetry/modules and from to resource://gre/modules/ to resource://gre/modules/telemetry. This should simplify the work towards moving some or all of TelemetryPing off the main thread.

Ooups, TelemetryPing_idle didn't pass. Fixed.

Comment on attachment 804455 [details] [diff] [review] Moving TelemetryPing to a jsm, v2 Review of attachment 804455 [details] [diff] [review]: ----------------------------------------------------------------- I can't tell through splinter, but please ensure that you |hg cp TelemetryPing.js TelemetryPing.jsm.in|, though I'm not sure that you won't be touching every line anyway... AIUI, every time somebody imports TelemetryPing.jsm, they get a fresh copy of |Impl|, is that correct? If so, that's not what we want. I think TelemetryPing.js's observers should still get registered, but you definitely want to double-check. I'm not entirely clear on how this makes life easier for anybody. ::: toolkit/components/telemetry/moz.build @@ +5,5 @@ > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +DIRS += [ > + 'modules', > +] You are going to need build peer review for the moz.build changes, though they look sane enough to me. ::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js @@ +187,5 @@ > + do_check_true(payload.info.revision.startsWith("http")); > + } catch(x) { > + // This check will fail if the repo origin is a local mirror, > + // so ensure that the failure does not stop the test. > + } Please make this a separate bug. This is not the bug to be making changes like this and it's not the right way to fix it. @@ +348,5 @@ > } > > function runInvalidJSONTest() { > let histogramsFile = getSavedHistogramsFile("invalid-histograms.dat"); > + do_print("histogramsFile: " + histogramsFile.path); Debugging leftovers. ::: toolkit/components/telemetry/tests/unit/test_ThirdPartyCookieProbe.js @@ +7,5 @@ > > Cu.import("resource://gre/modules/Services.jsm"); > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/telemetry/TelemetryPing.jsm"); > +Cu.import("resource://gre/modules/telemetry/ThirdPartyCookieProbe.jsm"); Did ThirdPartyCookieProbe.jsm move?

(In reply to Nathan Froyd (:froydnj) (AFK 16-20 September) from comment #3) > Comment on attachment 804455 [details] [diff] [review] > Moving TelemetryPing to a jsm, v2 > > Review of attachment 804455 [details] [diff] [review]: > ----------------------------------------------------------------- > > I can't tell through splinter, but please ensure that you |hg cp > TelemetryPing.js TelemetryPing.jsm.in|, though I'm not sure that you won't > be touching every line anyway... Will do. > AIUI, every time somebody imports TelemetryPing.jsm, they get a fresh copy > of |Impl|, is that correct? If so, that's not what we want. As discussed over IRC, that's not what happens. > I think TelemetryPing.js's observers should still get registered, but you > definitely want to double-check. You are right, they remain registered, and we will need to keep (at least of a subset of) the xpcom component for that purpose. > I'm not entirely clear on how this makes life easier for anybody. Did I convince you over IRC that xpcom components are the wrong way to provide modularity in JavaScript now that we have js modules? > ::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js > @@ +187,5 @@ > > + do_check_true(payload.info.revision.startsWith("http")); > > + } catch(x) { > > + // This check will fail if the repo origin is a local mirror, > > + // so ensure that the failure does not stop the test. > > + } > > Please make this a separate bug. This is not the bug to be making changes > like this and it's not the right way to fix it. Well, it's not meant to be a fix, but sure. > ::: toolkit/components/telemetry/tests/unit/test_ThirdPartyCookieProbe.js > @@ +7,5 @@ > > > > Cu.import("resource://gre/modules/Services.jsm"); > > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > > +Cu.import("resource://gre/modules/telemetry/TelemetryPing.jsm"); > > +Cu.import("resource://gre/modules/telemetry/ThirdPartyCookieProbe.jsm"); > > Did ThirdPartyCookieProbe.jsm move? Yes, I moved the non-public telemetry modules to modules/telemetry.

See above: Nathan, have I managed to convince you that moving to a .jsm is the right thing to do?

(In reply to David Rajchenbach Teller [:Yoric] from comment #5) > See above: Nathan, have I managed to convince you that moving to a .jsm is > the right thing to do? I think so.

Depends on the patch for bug 742500.

Comment on attachment 8355251 [details] [diff] [review] 913070 Review of attachment 8355251 [details] [diff] [review]: ----------------------------------------------------------------- Also, I realize that you'll need to create a new XPCOM component (let's call it TelemetryStartup.js) just to watch for profile-after-change and propagate that notification to |TelemetryPing.jsm|. TelemetryStartup should replace TelemetryPing in the .manifest file at the line dealing with profile-before-change. See https://developer.mozilla.org/en-US/docs/XPCOM/Receiving_startup_notifications Also, please give more meaningful names to your patches when you upload them, e.g. "Moving TelemetryPing to a .jsm, v3". ::: toolkit/components/telemetry/TelemetryPing.js @@ +12,3 @@ > > +function TelemetryPing() { > + Deprecated.warning("nsITelemetryPing is deprecated. Please use TelemetryPing.jsm instead", "FIXME:TODO"); This "FIXME:TODO" should be replaced with the URI of a page documenting TelemetryPing.jsm. @@ +15,5 @@ > } > > +TelemetryPing.prototype = Object.create(JSM.TelemetryPing); > +TelemetryPing.prototype.classID = Components.ID("{55d6a5fa-130e-4ee6-a158-0133af3b86ba}"); > +TelemetryPing.prototype.QueryInterface = XPCOMUtils.generateQI([Components.interfaces.nsITelemetryPing, Components.interfaces.nsIObserver]); Let's take the opportunity to remove nsIObserver. ::: toolkit/components/telemetry/TelemetryPing.jsm @@ +157,5 @@ > + PREF_PREVIOUS_BUILDID: PREF_PREVIOUS_BUILDID, > + }), > + /** > + * Reset TelemetryPing. > + * Maybe "only for testing purposes". ::: toolkit/components/telemetry/moz.build @@ +19,1 @@ > 'ThreadHangStats.h', For this file, once the rest is complete, you'll need a review from glandium. ::: toolkit/components/telemetry/tests/unit/test_TelemetryPing_idle.js @@ +1,4 @@ > /* Any copyright is dedicated to the Public Domain. > http://creativecommons.org/publicdomain/zero/1.0/ */ > > // Check that TelemetryPing notifies correctly on idle. Actually, it's idle-daily, could you take the opportunity to update the comment? ::: toolkit/mozapps/extensions/XPIProvider.jsm @@ +2255,5 @@ > catch (e) { } > > + const tmp = {} > + Cu.import("resource://gre/modules/TelemetryPing.jsm", tmp); > + tmp.TelemetryPing.setAddOns(data); Actually, that |tmp| is not very nice. We can probably do something along the lines of Cu.import("resource://...", {}).TelemetryPing.setAddons.data

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #8) > ::: toolkit/components/telemetry/TelemetryPing.jsm > @@ +157,5 @@ > > + PREF_PREVIOUS_BUILDID: PREF_PREVIOUS_BUILDID, > > + }), > > + /** > > + * Reset TelemetryPing. > > + * > > Maybe "only for testing purposes". It's already there. > ::: toolkit/mozapps/extensions/XPIProvider.jsm > @@ +2255,5 @@ > > catch (e) { } > > > > + const tmp = {} > > + Cu.import("resource://gre/modules/TelemetryPing.jsm", tmp); > > + tmp.TelemetryPing.setAddOns(data); > > Actually, that |tmp| is not very nice. > We can probably do something along the lines of > Cu.import("resource://...", {}).TelemetryPing.setAddons.data The xpcshell testsuite of toolkit/mozapps/extensions hangs if I do that, without showing any particular error.

Please ignore my last comment.

Comment on attachment 8357016 [details] [diff] [review] Moving TelemetryPing to a .jsm, v3 Review of attachment 8357016 [details] [diff] [review]: ----------------------------------------------------------------- r=me with minor stuff below. Also, don't forget to change your commit message to match our conventions (see e.g. http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed for reference). ::: toolkit/components/telemetry/TelemetryPing.js @@ +11,4 @@ > > +function TelemetryPing() { > + Deprecated.warning("nsITelemetryPing is deprecated. Please use TelemetryPing.jsm instead", > + "https://bugzilla.mozilla.org/show_bug.cgi?id=913070"); That's ok for the moment, but in a followup, once TelemetryPing.jsm has been cleaned up (i.e. no synchronous APIs), this will need to point rather to a developer.mozilla.org documentation that you'll need to write. ::: toolkit/components/telemetry/TelemetryPing.jsm @@ +158,5 @@ > + }), > + /** > + * Reset TelemetryPing. > + * > + * Used for testing purposes. I meant that you should probably add the word "only" to this documentation. ::: toolkit/components/telemetry/TelemetryStartup.js @@ +1,5 @@ > +/* -*- js-indent-level: 2; indent-tabs-mode: nil -*- */ > +/* 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/. */ > + This needs a litte documentation. Also, "use strict" is generally a good idea.

Please review toolkit/components/telemetry/moz.build.

Comment on attachment 8357728 [details] [diff] [review] 913070.diff Review of attachment 8357728 [details] [diff] [review]: ----------------------------------------------------------------- Why keep the xpcom component?

(In reply to Mike Hommey [:glandium] from comment #14) > Why keep the xpcom component? Backwards compatibility. But it now displays deprecation messages.

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #15) > (In reply to Mike Hommey [:glandium] from comment #14) > > Why keep the xpcom component? > > Backwards compatibility. But it now displays deprecation messages. Backwards compatibility with what? what would be using that component that isn't in mozilla-central?

Backed out for xpcshell failures. Please run this through Try before requesting checkin again. https://hg.mozilla.org/integration/fx-team/rev/94e15af5dd4b https://tbpl.mozilla.org/php/getParsedLog.php?id=32819464&tree=Fx-Team

The failures seem to be on linux64 only.

try: https://tbpl.mozilla.org/?tree=Try&rev=323ad2bf9dbc

patching file toolkit/components/telemetry/TelemetryPing.js Hunk #1 FAILED at 0 1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/telemetry/TelemetryPing.js.rej Please rebase :(