913070 - [Telemetry] Move TelemetryPing to a .jsm

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Moving TelemetryPing to a jsm

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.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Moving TelemetryPing to a jsm, v2

Ooups, TelemetryPing_idle didn't pass. Fixed.

Comment 3

[Nathan Froyd [:froydnj]]

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?

Comment 4

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

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

Comment 6

[Nathan Froyd [:froydnj]]

(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.

Comment 7

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 913070

Depends on the patch for bug 742500.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

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

Comment 9

[Roberto Agostino Vitillo (:rvitillo)]

(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.

Comment 10

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Moving TelemetryPing to a .jsm, v2

Comment 11

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Moving TelemetryPing to a .jsm, v3

Please ignore my last comment.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 13

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 913070.diff

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

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8357728 [details] [diff] [review]
913070.diff

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

Why keep the xpcom component?

Comment 15

[David Teller [:Yoric] - still alive but not very active]

(In reply to Mike Hommey [:glandium] from comment #14)
> Why keep the xpcom component?

Backwards compatibility. But it now displays deprecation messages.

Comment 16

[Mike Hommey [:glandium]]

(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?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ceb7d7bffe8c

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

The failures seem to be on linux64 only.

Comment 20

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Moving TelemetryPing to a .jsm, v5

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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 :(

Comment 22

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Moving TelemetryPing to a .jsm, v6

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/683e4728dbe6

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/683e4728dbe6