1176331 - Create a "system" module for devtools

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

Combining preprocessed values from AppConstants.jsm, as well as sdk/system

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1176331-system.patch

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8624909 [details] [diff] [review]
1176331-system.patch

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

Assuming the contents of the new system.js are the same as what was ripped out of toolbox.js / device.js I'm on board!

::: browser/devtools/performance/performance.xul
@@ -13,5 @@
>  ]>
>  
>  <window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>    <script src="chrome://browser/content/devtools/theme-switching.js"/>
> -  <script type="application/javascript" src="performance/system.js"/>

Was this file deleted as part of the patch?  I'm not seeing the deletion in splinter

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1176331-system.patch

Removed unused 'system.js' in perf tools now. Changing reviewers to jryans and panos

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Oops, didn't see you reviewed it Brian! And yes just reupped the patch that removes the performance/system.js -- it should be the same as toolbox.js/device.js, with additional available information

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8624909 [details] [diff] [review]
1176331-system.patch

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

Please re-verify that the values from the device actor have not changed.

We recently started logging some of them to telemetry[1], so it's important that the keys and values remain as they were.

I'll take another look once you've checked them.

[1]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/modules/app-manager.js#518-529

::: toolkit/devtools/shared/system.js
@@ +31,5 @@
> +};
> +
> +let CACHED_INFO = null;
> +
> +function *getSystemInfo () {

Nit: I believe our ESLint rules warn on a space before ( here

@@ +52,5 @@
> +    version = yield exports.getSetting("deviceinfo.os");
> +  }
> +  // Not B2G
> +  else {
> +    os = OS.type();

Is this the same as "appInfo.OS"?

@@ +53,5 @@
> +  }
> +  // Not B2G
> +  else {
> +    os = OS.type();
> +    version = geckoVersion;

This used to be "appInfo.version", any reason to change?

@@ +93,5 @@
> +    name: appInfo.name,
> +
> +    // The XUL application's version, for example "0.8.0+" or "3.7a1pre".
> +    // It is different than the version of Gecko or the XULRunner platform.
> +    XULVersion: appInfo.version,

Should the key name be lower case to match other keys?  I know it's an acronym, up to you I guess.

@@ +119,5 @@
> +     * Information regarding the operating system.
> +     */
> +
> +    // Returns a path to the temp directory
> +    tmpdir: OS.tmpdir(),

Is this safe for Fennec / B2G?  I know other file / directory references have occasionally failed on those runtimes.

@@ +151,5 @@
> +    // `'msvc', 'n32', 'gcc2', 'gcc3', 'sunc', 'ibmc'...`
> +    compiler,
> +
> +    // Returns EOL character for the OS
> +    EOL: OS.EOL,

Should the key name be lower case to match other keys?  I know it's an acronym, up to you I guess.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Sorry, can't seem to re-add bgrins r+...

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8624917 [details] [diff] [review]
1176331-system.patch

Same comments from last version.

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8624909 [details] [diff] [review]
1176331-system.patch

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

::: browser/devtools/performance/performance.xul
@@ -13,5 @@
>  ]>
>  
>  <window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>    <script src="chrome://browser/content/devtools/theme-switching.js"/>
> -  <script type="application/javascript" src="performance/system.js"/>

Fixed in latest patch

::: toolkit/devtools/shared/system.js
@@ +31,5 @@
> +};
> +
> +let CACHED_INFO = null;
> +
> +function *getSystemInfo () {

yuck

@@ +52,5 @@
> +    version = yield exports.getSetting("deviceinfo.os");
> +  }
> +  // Not B2G
> +  else {
> +    os = OS.type();

yup, but changing it to that anyway for clarity

@@ +53,5 @@
> +  }
> +  // Not B2G
> +  else {
> +    os = OS.type();
> +    version = geckoVersion;

No; you're right, this should be version http://mxr.mozilla.org/mozilla-central/source/xpcom/system/nsIXULAppInfo.idl#39

@@ +93,5 @@
> +    name: appInfo.name,
> +
> +    // The XUL application's version, for example "0.8.0+" or "3.7a1pre".
> +    // It is different than the version of Gecko or the XULRunner platform.
> +    XULVersion: appInfo.version,

Removing this -- this is redundant with `version` now, and it's the application version, not XUL itself, which is represented by platformversion/geckoversion.

@@ +119,5 @@
> +     * Information regarding the operating system.
> +     */
> +
> +    // Returns a path to the temp directory
> +    tmpdir: OS.tmpdir(),

Removing -- this was just from the SDK module

@@ +151,5 @@
> +    // `'msvc', 'n32', 'gcc2', 'gcc3', 'sunc', 'ibmc'...`
> +    compiler,
> +
> +    // Returns EOL character for the OS
> +    EOL: OS.EOL,

Removing -- this was just from the SDK module

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1176331-system.patch

Confirmed that the telemetry fields ultimately map to the same source as previously on the device actor.

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1176331-system.patch

Oops did not update patch. This has all the changes from the previous review. And rebased.

Big try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=302a18eb0085

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8624958 [details] [diff] [review]
1176331-system.patch

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

::: toolkit/devtools/shared/system.js
@@ +120,5 @@
> +    /**
> +     * Information regarding the operating system.
> +     */
> +
> +    // Returns the endianness of teh architecture: either "LE" or "BE"

Nit: teh -> the

@@ +147,5 @@
> +    // `'msvc', 'n32', 'gcc2', 'gcc3', 'sunc', 'ibmc'...`
> +    compiler,
> +
> +    // Returns EOL character for the OS
> +    EOL: OS.EOL,

It seems you did not remove this?

Comment 14

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1176331-system.patch

Fixed nits, actually removed EOL, rebased
https://hg.mozilla.org/integration/fx-team/rev/171e623cf40c

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/171e623cf40c