Closed Bug 1176331 Opened 5 years ago Closed 5 years ago

Create a "system" module for devtools

Categories

(DevTools :: General, defect)

41 Branch
defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 4 obsolete files)

Combining preprocessed values from AppConstants.jsm, as well as sdk/system
Attached patch 1176331-system.patch (obsolete) — Splinter Review
Attachment #8624909 - Flags: review?(jryans)
Attachment #8624909 - Flags: review?(bgrinstead)
Duplicate of this bug: 1174836
Duplicate of this bug: 1176325
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
Attachment #8624909 - Flags: review?(bgrinstead) → review+
Status: NEW → ASSIGNED
Attached patch 1176331-system.patch (obsolete) — Splinter Review
Removed unused 'system.js' in perf tools now. Changing reviewers to jryans and panos
Attachment #8624909 - Attachment is obsolete: true
Attachment #8624909 - Flags: review?(jryans)
Attachment #8624917 - Flags: review?(past)
Attachment #8624917 - Flags: review?(jryans)
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 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.
Attachment #8624909 - Attachment is obsolete: false
Attachment #8624909 - Flags: review+ → review?(bgrinstead)
Sorry, can't seem to re-add bgrins r+...
Attachment #8624909 - Flags: review- → review+
Comment on attachment 8624917 [details] [diff] [review]
1176331-system.patch

Same comments from last version.
Attachment #8624917 - Flags: review?(jryans) → review-
Attachment #8624909 - Attachment is obsolete: true
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
Attached patch 1176331-system.patch (obsolete) — Splinter Review
Confirmed that the telemetry fields ultimately map to the same source as previously on the device actor.
Attachment #8624917 - Attachment is obsolete: true
Attachment #8624917 - Flags: review?(past)
Attachment #8624949 - Flags: review?(jryans)
Attached patch 1176331-system.patch (obsolete) — Splinter Review
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
Attachment #8624949 - Attachment is obsolete: true
Attachment #8624949 - Flags: review?(jryans)
Attachment #8624958 - Flags: review?(jryans)
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?
Attachment #8624958 - Flags: review?(jryans) → review+
Fixed nits, actually removed EOL, rebased
https://hg.mozilla.org/integration/fx-team/rev/171e623cf40c
Attachment #8624958 - Attachment is obsolete: true
Attachment #8625107 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/171e623cf40c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.