1266839 - replace nsIXULAppInfo

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

We'll need to replace uses of nsIXULAppInfo for the devtools 
de-chrome-ification project.

Comment 1

[Tom Tromey :tromey]

The implementation depends on some bits added in bug 1265802.

Comment 2

[Tom Tromey :tromey]

Attachment: Bug 1266839 - replace uses of nsIXULAppInfo in devtools;

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

Comment 3

[Greg Tatum [:gregtatum]]

https://reviewboard.mozilla.org/r/64662/#review61714

::: devtools/client/framework/toolbox.js:495
(Diff revision 1)
>  
>    _pingTelemetry: function () {
>      this._telemetry.toolOpened("toolbox");
>  
>      this._telemetry.logOncePerBrowserVersion(OS_HISTOGRAM, system.getOSCPU());
> -    this._telemetry.logOncePerBrowserVersion(OS_IS_64_BITS, system.is64Bit ? 1 : 0);
> +    this._telemetry.logOncePerBrowserVersion(OS_IS_64_BITS,

Should we even log this if it's a dummy value? There's no way this will be anything except 1 at this point.

::: devtools/client/shared/shim/Services.js:490
(Diff revision 1)
> -    }
> -  },
> +    },
> +
> +    // It's fine for this to be an approximation.
> +    get name() {
> +      return window.navigator.userAgent || "Unknown";

Will this ever be "Unknown"?

::: devtools/client/shared/shim/Services.js:495
(Diff revision 1)
> +      return window.navigator.userAgent || "Unknown";
> +    },
> +
> +    // It's fine for this to be an approximation.
> +    get version() {
> +      return window.navigator.appVersion || "Unknown";

It seems weird to have these fake values. navigator.appVersion is pretty much just noise. Are we planning on eventually migrating away from having a shim? If that's the case this doesn't really bother me, but if we're planning on keeping the shims, then it seems like we should just get rid of this interface entirely if we can.

Comment 4

[Tom Tromey :tromey]

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #3)

> > -    this._telemetry.logOncePerBrowserVersion(OS_IS_64_BITS, system.is64Bit ? 1 : 0);
> > +    this._telemetry.logOncePerBrowserVersion(OS_IS_64_BITS,
> 
> Should we even log this if it's a dummy value? There's no way this will be
> anything except 1 at this point.

It will still be correct in the running-in-chrome case.

> > +      return window.navigator.userAgent || "Unknown";
> 
> Will this ever be "Unknown"?

I don't know, but I think this is leftover from trying to use some other, less
established property.  I can remove that.

> ::: devtools/client/shared/shim/Services.js:495
> (Diff revision 1)
> > +      return window.navigator.userAgent || "Unknown";
> > +    },
> > +
> > +    // It's fine for this to be an approximation.
> > +    get version() {
> > +      return window.navigator.appVersion || "Unknown";
> 
> It seems weird to have these fake values. navigator.appVersion is pretty
> much just noise. Are we planning on eventually migrating away from having a
> shim? If that's the case this doesn't really bother me, but if we're
> planning on keeping the shims, then it seems like we should just get rid of
> this interface entirely if we can.

On the client it is used in telemetry (which will be a no-op later) and for
the HAR logs.  We have to stick something there.  The reason I did it this way
is to make all the code continue to work identically when running with chrome
privileges.

Comment 5

[Tom Tromey :tromey]

Comment on attachment 8771527 [details]
Bug 1266839 - replace uses of nsIXULAppInfo in devtools;

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

Comment 6

[Greg Tatum [:gregtatum]]

Comment on attachment 8771527 [details]
Bug 1266839 - replace uses of nsIXULAppInfo in devtools;

https://reviewboard.mozilla.org/r/64662/#review62058

LGTM.

Comment 7

[Tom Tromey :tromey]

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

Comment 8

[Tom Tromey :tromey]

Note the dependency, which affects commit order.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/4553ad09496a
replace uses of nsIXULAppInfo in devtools; r=gregtatum

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4553ad09496a