1206085 - Replace Telemetry JSM preprocessor usage with AppConstants.jsm usage

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

We have a lot of platform- and build build-flag-specific #ifdefs etc. in the JS modules in toolkit/components/telemetry/.

I want us to switch over to using AppConstants.jsm instead (mainly to have error/log line numbers line up with the code).

Comment 1

[Georg Fritzsche [:gfritzsche]]

Vladan, do you have any concerns here?
Performance doesn't seem a concern as we shouldn't have hot code implemented in the JSMs anyway.

Comment 2

[Vladan Djeric (:vladan)]

Sounds good to me

Comment 3

[Georg Fritzsche [:gfritzsche]]

Hey Iaroslav, would you be interested in taking this on?

It's mostly about removing the preprocessor usage toolkit/components/telemetry/ with what AppConstants.jsm usage (and add properties if something is still missing).

That prepares us well for the other backlogged cleanup work for Telemetry in bug 1201022.

Comment 4

[Iaroslav Sheptykin]

Hi Georg!

Sound doable. I am on it!

Comment 5

[Iaroslav Sheptykin]

Hi Georg!

If I understand correctly there are the files to be updated:

/toolkit/components/telemetry/TelemetryController.jsm (View Hg log or Hg annotations)

    line 677 -- #ifdef MOZILLA_OFFICIAL

/toolkit/components/telemetry/TelemetrySession.jsm (View Hg log or Hg annotations)

    line 1420 -- #ifdef MOZ_WIDGET_ANDROID
    line 1632 -- #ifdef MOZ_WIDGET_ANDROID
    line 1735 -- #ifdef MOZ_WIDGET_ANDROID

/toolkit/components/telemetry/TelemetryEnvironment.jsm (View Hg log or Hg annotations)

    line 277 -- #ifdef XP_WIN
    line 671 -- #ifdef MOZ_WIDGET_GONK
    line 975 -- #ifdef MOZ_WIDGET_GONK
    line 1129 -- #ifdef XP_WIN
    line 1242 -- #ifdef XP_WIN

[see http://mxr.mozilla.org/mozilla-central/search?find=%2Ftoolkit%2Fcomponents%2Ftelemetry%2F&string=%23ifdef]

#ifdef constant mapping:
1. #ifdef MOZILLA_OFFICIAL -> AppConstants.MOZILLA_OFFICIAL [1]
2. #ifdef MOZ_WIDGET_ANDROID -> AppConstants.platform = "android" [2]
3. #ifdef XP_WIN -> AppConstants.platform = "win" [3]
4. #ifdef MOZ_WIDGET_GONK -> AppConstants.platform = "gonk" [4]

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/AppConstants.jsm#41
[2] http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/AppConstants.jsm#138
[3] http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/AppConstants.jsm#134
[4] http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/AppConstants.jsm#140

Comment 6

[Georg Fritzsche [:gfritzsche]]

I see a few more with #if (including MOZ_WIDGET_GTK usage), but otherwise this looks right.
https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolkit%2Fcomponents%2Ftelemetry+ext%3Ajsm+ext%3Ajs+%23if&redirect=false&case=false&limit=69&offset=0

Comment 7

[Iaroslav Sheptykin]

Attachment: MozReview Request: Bug 1206085: Finishing the replacement of the telemetry preprocessor usage. r?gfritzsche

Bug 1206085: Replacing preprocessor directives with AppConstants in Telemetry modules. r?gfritzsche

Comment 8

[Iaroslav Sheptykin]

Hi Georg!

Above is the first draft of the patch. I wanted to learn using ReviewBoard and its review workflow this time. I hope it is ok with you. Otherwise I can just drop a patch here as always.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8664212 [details]
MozReview Request: Bug 1206085: Finishing the replacement of the telemetry preprocessor usage. r?gfritzsche

https://reviewboard.mozilla.org/r/19939/#review18029

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:285
(Diff revision 1)
> -function getServicePack() {
> +this.getServicePack = function() {

I don't think we need to wrap this.
On non-windows platforms this just shouldn't get called, wrapping it with the `if` doesn't seem to give any advantage.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:681
(Diff revision 1)
> -#endif
> +  if (AppConstants.platform !== "android") {

Nit: One empty line before this.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1025
(Diff revision 1)
> +    if (AppConstants.platform !== "gonk") {

Nit: empty line before this.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1029
(Diff revision 1)
> +    if (AppConstants.platform !== "android") {

Nit: empty line before this.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
(Diff revision 1)
> -#if defined(MOZ_WIDGET_GONK) || defined(MOZ_WIDGET_ANDROID)
> -  /**
> -   * Get the device information, if we are on a portable device.
> -   * @return Object containing the device information data.
> -   */
> -  _getDeviceData: function () {
> -    return {
> -      model: getSysinfoProperty("device", null),
> -      manufacturer: getSysinfoProperty("manufacturer", null),
> -      hardware: getSysinfoProperty("hardware", null),
> -      isTablet: getSysinfoProperty("tablet", null),
> -    };
> -  },
> -#endif

I don't think we need to change this.
Lets just drop the `#if` around it.
If we want to be careful, lets return null on non "gonk" and "android" platforms.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1117
(Diff revision 1)
> -    let servicePack = getServicePack();
> +      let servicePack = getServicePack();

That actually works?
I think we need to move the `servicePack` declaration out of the `if` block.
Although, given that we now add the windows specific details in their own section, lets just move this there too (line 1129).

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1236
(Diff revision 1)
> +    if (AppConstants.platform === "win") {

Nit: empty line before this.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1239
(Diff revision 1)
> +    if (["gonk", "android"].indexOf(AppConstants.platform) !== -1) {

Nit: empty line before this.

::: toolkit/components/telemetry/TelemetrySession.jsm:1311
(Diff revision 1)
> -#if defined(MOZ_WIDGET_GONK) || defined(MOZ_WIDGET_ANDROID)
> -    clearSubsession = false;
> +    const isSubsession = (["gonk", "android"].indexOf(AppConstants.platform) !== -1) ?
> +      (clearSubsession = false) : !this._isClassicReason(reason);

I think this is a bit hard to read.

How about:

```javascript
const isMobile = ["gonk", "android"].indexOf(...) ...;
const isSubsession = isMobile ? ...;
if (isMobile) {
  clearSubsession = false;
}
```

Comment 10

[Georg Fritzsche [:gfritzsche]]

(In reply to Iaroslav Sheptykin from comment #8)
> Hi Georg!
> 
> Above is the first draft of the patch. I wanted to learn using ReviewBoard
> and its review workflow this time. I hope it is ok with you. Otherwise I can
> just drop a patch here as always.

Sure, RB is nice for reviewers anyway :)

Comment 11

[Iaroslav Sheptykin]

Comment on attachment 8664212 [details]
MozReview Request: Bug 1206085: Finishing the replacement of the telemetry preprocessor usage. r?gfritzsche

Bug 1206085: Replacing preprocessor directives with AppConstants in Telemetry modules. r?gfritzsche

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8664212 [details]
MozReview Request: Bug 1206085: Finishing the replacement of the telemetry preprocessor usage. r?gfritzsche

https://reviewboard.mozilla.org/r/19939/#review18033

This looks good, thanks!
Lets fix the mentioned issues, give this a try run (all platforms over mochitests and xpcshell), then get it landed.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1254
(Diff revision 2)
> +    if (AppConstants.platform === "win") {

Nit: empty line before this one.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1257
(Diff revision 2)
> +    if (["gonk", "android"].indexOf(AppConstants.platform) !== -1) {

Nit: Make it an `else if` or add an empty line before this one.

Comment 13

[Iaroslav Sheptykin]

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

Comment 14

[Iaroslav Sheptykin]

Comment on attachment 8664212 [details]
MozReview Request: Bug 1206085: Finishing the replacement of the telemetry preprocessor usage. r?gfritzsche

Bug 1206085: Replacing preprocessor directives with AppConstants in Telemetry modules. r=gfritzsche

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8664212 [details]
MozReview Request: Bug 1206085: Finishing the replacement of the telemetry preprocessor usage. r?gfritzsche

https://reviewboard.mozilla.org/r/19939/#review18045

Comment 16

[Georg Fritzsche [:gfritzsche]]

The try run looks good to me, i starred the unrelated failures.

Comment 17

[Iaroslav Sheptykin]

great!

Comment 18

[Carsten Book [:Tomcat]]

Hi, this failed to apply:

patching file toolkit/components/telemetry/TelemetryEnvironment.jsm
Hunk #1 FAILED at 14
Hunk #7 FAILED at 993
2 out of 10 hunks FAILED -- saving rejects to file toolkit/components/telemetry/TelemetryEnvironment.jsm.rej

could you take a look? Thanks!

Comment 19

[Iaroslav Sheptykin]

Hi Carsten! Sorry for causing you trouble. I cannot reproduce this failure when doing:

$ hg import https://reviewboard-hg.mozilla.org/gecko/rev/fa9f62f0377d65388e218a78f8b60d2c35586d72

against the newest mozilla-central. To me hg reports: "Hunk #1 succeeded at 21 with fuzz 1 (offset 0 lines)."

What are you importing into? Or how can I reproduce this failure?

Comment 20

[Pulsebot]

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

Comment 21

[Iaroslav Sheptykin]

Hi Georg! I am worried that the pulsebot has checked something in that does not properly work. See comment 18 by Carsten. Apparently the patch didn't apply for Carsten but it is now checked in by Pulsebot. Is that ok?

Comment 22

[Georg Fritzsche [:gfritzsche]]

I did a quick diff here between what was pushed to fxteam and the final state on ReviewBoard:
https://pastebin.mozilla.org/8847682

There is nothing bad going on, i guess the conflict was just the UpdateChannel/UpdateUtils that was on fx-team but not mozilla-central yet.
Looks like who landed this just resolved that conflict :)

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b169aa6de69

Comment 24

[Philip Chee]

You forgot to update the moz.build file
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/moz.build?rev=c646a88160d7#44
Move from EXTRA_PP_JS_MODULES to EXTRA_JS_MODULES

In TelemetrySession.jsm you can remove the final bit of preprocessing using AppConstants.jsm

In toolkit/modules/moz.build add HISTOGRAMS_FILE_VERSION to the section that begins (This may not be needed):

for var in ('ANDROID_PACKAGE_NAME',
            .......

Then in AppConstants.jsm near the end of the file add a line like:

  HISTOGRAMS_FILE_VERSION: @HISTOGRAMS_FILE_VERSION@

Then in TelemetrySession.jsm:

-      revision: HISTOGRAMS_FILE_VERSION,
+      revision: AppConstants.HISTOGRAMS_FILE_VERSION,

Comment 25

[Iaroslav Sheptykin]

Comment on attachment 8664212 [details]
MozReview Request: Bug 1206085: Finishing the replacement of the telemetry preprocessor usage. r?gfritzsche

Bug 1206085: Finishing the replacement of the telemetry preprocessor usage. r?gfritzsche

Comment 26

[Georg Fritzsche [:gfritzsche]]

Philip, can we open a follow-up bug next time for this kind of things?
The patch here was already landed and merged.

Comment 27

[Iaroslav Sheptykin]

Georg, I am discarding the review request because of a mistake on my behalf. I moved the PP_MODULES into a wrong place.

Comment 28

[Georg Fritzsche [:gfritzsche]]

https://reviewboard.mozilla.org/r/19939/#review18551

While we are doing this, we should also address/move the `HISTOGRAM_FILE_VERSION` definition:
https://dxr.mozilla.org/mozilla-central/rev/031db40e2b558c7e4dd0b4c565db4a992c1627c8/toolkit/components/telemetry/Makefile.in#9

All this really has is the current revision of the repo (e.g. "https://hg.mozilla.org/mozilla-central/rev/6256ec9113c1"), so lets make this just a more general constant on AppConstants.jsm like `REVISION` or `SOURCE_REVISION`.

I also wonder if we don't have a cleaner solution in or build system now.

::: toolkit/components/telemetry/moz.build:46
(Diff revision 4)
> -    'TelemetryController.jsm',
> +  'TelemetryController.jsm',
> -    'TelemetryEnvironment.jsm',
> +  'TelemetryEnvironment.jsm',
> -    'TelemetrySession.jsm',
> +  'TelemetrySession.jsm',

These are not testing-specific modules, they belong into `EXTRA_JS_MODULES`.

Comment 29

[Georg Fritzsche [:gfritzsche]]

Iaroslav, lets move further discussion & review over to bug 1209458. Thanks.

Comment 30

[Georg Fritzsche [:gfritzsche]]

Also, thanks Phillip for the catch here.