Closed Bug 1206085 Opened 4 years ago Closed 4 years ago

Replace Telemetry JSM preprocessor usage with AppConstants.jsm usage

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: gfritzsche, Assigned: yarik.sheptykin, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [unifiedTelemetry] [lang=js] [measurement:client])

Attachments

(1 obsolete file)

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).
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.
Flags: needinfo?(vladan.bugzilla)
Sounds good to me
Flags: needinfo?(vladan.bugzilla)
Assignee: nobody → gfritzsche
Whiteboard: [unifiedTelemetry] → [unifiedTelemetry] [lang=js]
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.
Flags: needinfo?(yarik.sheptykin)
Hi Georg!

Sound doable. I am on it!
Flags: needinfo?(yarik.sheptykin)
Assignee: gfritzsche → yarik.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
Bug 1206085: Replacing preprocessor directives with AppConstants in Telemetry modules. r?gfritzsche
Attachment #8664212 - Flags: review?(gfritzsche)
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.
Mentor: 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;
}
```
Attachment #8664212 - Flags: review?(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 :)
Status: NEW → ASSIGNED
Summary: Remove Telemetry JSM preprocessor usage with AppConstants.jsm usage → Replace Telemetry JSM preprocessor usage with AppConstants.jsm usage
Whiteboard: [unifiedTelemetry] [lang=js] → [unifiedTelemetry] [lang=js] [measurement:client]
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
Attachment #8664212 - Flags: review?(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.
Attachment #8664212 - Flags: review?(gfritzsche) → review+
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
Attachment #8664212 - Attachment description: MozReview Request: Bug 1206085: Replacing preprocessor directives with AppConstants in Telemetry modules. r?gfritzsche → MozReview Request: Bug 1206085: Replacing preprocessor directives with AppConstants in Telemetry modules. r=gfritzsche
Attachment #8664212 - Flags: review+ → review?(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
Attachment #8664212 - Flags: review?(gfritzsche) → review+
The try run looks good to me, i starred the unrelated failures.
great!
Keywords: checkin-needed
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!
Flags: needinfo?(yarik.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?
Flags: needinfo?(yarik.sheptykin) → needinfo?(cbook)
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?
Flags: needinfo?(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 :)
Flags: needinfo?(gfritzsche)
Flags: needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/8b169aa6de69
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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,
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attachment #8664212 - Attachment description: MozReview Request: Bug 1206085: Replacing preprocessor directives with AppConstants in Telemetry modules. r=gfritzsche → MozReview Request: Bug 1206085: Finishing the replacement of the telemetry preprocessor usage. r?gfritzsche
Attachment #8664212 - Flags: review+ → review?(gfritzsche)
Philip, can we open a follow-up bug next time for this kind of things?
The patch here was already landed and merged.
Attachment #8664212 - Attachment is obsolete: true
Attachment #8664212 - Flags: review?(gfritzsche)
Georg, I am discarding the review request because of a mistake on my behalf. I moved the PP_MODULES into a wrong place.
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`.
Blocks: 1209458
Iaroslav, lets move further discussion & review over to bug 1209458. Thanks.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Also, thanks Phillip for the catch here.
You need to log in before you can comment on or make changes to this bug.