Closed Bug 1266839 Opened 5 years ago Closed 5 years ago

replace nsIXULAppInfo

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.3 - Jul 18
Tracking Status
firefox50 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file)

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: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 50.3 - Jul 18
Priority: P2 → P1
The implementation depends on some bits added in bug 1265802.
Depends on: 1265802
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.
(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 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+
Note the dependency, which affects commit order.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/4553ad09496a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.