1265802 - replace nsIXULRuntime.OS and Services.appinfo.OS

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

Replace uses of nsIXULRuntime.OS and Services.appinfo.OS for the devtools
de-chrome-ification project.

Comment 1

[[e-mail]]

Tom, 

I'd plan to add `osString` as a property of Tools.inspector/webconsole in devtools/client/definitions.
Therefore all related API call will be constrained into definitions. 

If that is the right direction, do you have any suggestion to remove the direct call of nsIXULRuntime.OS and Services.appinfo.OS call inside of devtools/client/definitions?

Comment 2

[Tom Tromey :tromey]

(In reply to Fred Lin [:gasolin] from comment #1)
> Tom, 
> 
> I'd plan to add `osString` as a property of Tools.inspector/webconsole in
> devtools/client/definitions.
> Therefore all related API call will be constrained into definitions. 
> 
> If that is the right direction, do you have any suggestion to remove the
> direct call of nsIXULRuntime.OS and Services.appinfo.OS call inside of
> devtools/client/definitions?

I tend to think definitions.js is not the right spot.  I looked into it a little
and I think the major issue with that location is that there are also uses of
`OS` in devtools/shared (particularly devtools/shared/inspector/css-logic.js) --
and so due to import rules the solution has to be in devtool/shared as well.

devtools/shared/system.js comes to mind here; but I am not sure about this one.
It seems like a candidate for splitting up somehow.  But, maybe it would be ok.

The main issue I see is deciding how to differentiate between the chrome and
content cases here.  Maybe the code could check if `navigator.userAgent` exists.

Comment 3

[Tom Tromey :tromey]

Fred - if you are working on this, could you please self-assign the bug?
If not, let me know; it's the last unassigned P1 bug so normally I'd take
it before working on something else.

Comment 4

[[e-mail]]

Tom, now I'd focus on track 2 work, so feel free to take it

Comment 5

[Tom Tromey :tromey]

I got through this and then realized belatedly that it's better to stick the info
on the existing Services shim.

Comment 6

[Tom Tromey :tromey]

Attachment: Bug 1265802 - Add Services.appinfo.OS shim for devtools;

Review commit: https://reviewboard.mozilla.org/r/64610/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64610/

Comment 7

[Tom Tromey :tromey]

Comment on attachment 8771464 [details]
Bug 1265802 - Add Services.appinfo.OS shim for devtools;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64610/diff/1-2/

Comment 8

[Tom Tromey :tromey]

Found an eslint error just after uploading.

Comment 9

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

Comment on attachment 8771464 [details]
Bug 1265802 - Add Services.appinfo.OS shim for devtools;

https://reviewboard.mozilla.org/r/64610/#review61650

Looks reasonable to me, thanks for working on it!

Comment 10

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=316a291093fd

Comment 11

[Tom Tromey :tromey]

Comment on attachment 8771464 [details]
Bug 1265802 - Add Services.appinfo.OS shim for devtools;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64610/diff/2-3/

Comment 12

[Tom Tromey :tromey]

A couple minor test updates to avoid redefining Services.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=624c523f17ec

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/98a277292d0b
Add Services.appinfo.OS shim for devtools; r=jryans

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/98a277292d0b