replace nsIXULAppInfo

RESOLVED FIXED in Firefox 50

Status

P1
enhancement
RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

unspecified
Firefox 50
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [devtools-html])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
We'll need to replace uses of nsIXULAppInfo for the devtools 
de-chrome-ification project.
Flags: qe-verify-
Priority: -- → P2
Priority: P2 → P1
Priority: P1 → P2
(Assignee)

Updated

2 years ago
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 50.3 - Jul 18
Priority: P2 → P1
(Assignee)

Comment 1

2 years ago
The implementation depends on some bits added in bug 1265802.
Depends on: 1265802
(Assignee)

Comment 2

2 years ago
Created attachment 8771527 [details]
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/
Attachment #8771527 - Flags: review?(gtatum)
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.
(Assignee)

Comment 4

2 years ago
(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.
(Assignee)

Comment 5

2 years ago
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 on attachment 8771527 [details]
Bug 1266839 - replace uses of nsIXULAppInfo in devtools;

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

LGTM.
Attachment #8771527 - Flags: review?(gtatum) → review+
(Assignee)

Comment 8

2 years ago
Note the dependency, which affects commit order.
Keywords: checkin-needed

Comment 9

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/4553ad09496a
replace uses of nsIXULAppInfo in devtools; r=gregtatum
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4553ad09496a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.