895360 - [app manager] Device meta data actor

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Paul Rouget [:paul]]

The app manager needs these (static) data from the device:

OS version (Firefox OS, Android, ...)
Gecko version
IMEI
phone number
screen size & dpi

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

I recall Jim mentioning that the "traits" property of the hello packet would be used for something like that, but perhaps this is verbose enough that it needs a separate request.

Comment 2

[Jim Blandy :jimb]

That seems like it would be a separate request to the root actor. A function property of the RootActor constructor's aParameters arg that supplies this info would be great; RootActor should be responsible for handling the requests and formatting the reply.

Comment 3

[Jim Blandy :jimb]

(When creating RootActors on non-phones, we wouldn't supply that parameter, and the RootActor would send an error reply if it is asked for that data.)

Comment 4

[Paul Rouget [:paul]]

More detailed list of info (with example from keon):

Phone number (if available)
Hardware revision (qcom)
OS version (1.2.0.0-prerelease)
OS name (Boot2Gecko)
IMEI
Platform version (25.0a1)
Build Identifier (20130712005025)
Update Chanel (nightly)
screen size & dpi

For non-FxOS devices (Fennec and Firefox Desktop) we might want to skip some of these.

Comment 5

[Paul Rouget [:paul]]

We also need the brand name (Firefox OS, Boot2Gecko, Fennec, Firefox, Aurora, ...), and if it makes sense, the logo (about:logo) as a dataURL.

Comment 6

[Paul Rouget [:paul]]

Not about:logo, but chrome://branding/content/about-logo.png.

Comment 8

[Paul Rouget [:paul]]

Attachment: 895360.diff

Comment 9

[Paul Rouget [:paul]]

Comment on attachment 781755 [details] [diff] [review]
895360.diff

Past, what do you think? I put together device-info and permissionsTable. These data are static.

Comment 10

[Paul Rouget [:paul]]

Attachment: Patch v1

Panos, what do you think of this? A global device actor that is "on the side".

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 783034 [details] [diff] [review]
Patch v1

Review of attachment 783034 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. I'm curious if you tried it on Android though.

::: toolkit/devtools/server/actors/device.js
@@ +38,5 @@
> +
> +    res.dpi = utils.displayDPI;
> +    res.width = window.screen.width;
> +    res.height = window.screen.height;
> +    res.channel = Services.prefs.getCharPref('app.update.channel');

This throws for some reason when run in a scratchpad on desktop nightly and it will probably throw in other embeddings, like Thunderbird, etc. Can you surround it with a try/catch block and leave it undefined in that case?

@@ +46,5 @@
> +
> +    try {
> +      let radioInterfaceLayer = Cc["@mozilla.org/ril;1"].getService(Ci.nsIRadioInterfaceLayer);
> +      res.phoneNumber = radioInterfaceLayer.getMsisdn() || "unknown";
> +    } catch(e) {}

If we can't feature-test and need to use try/catch, would you please add a comment to the catch block explaining why we are ignoring the error?

@@ +60,5 @@
> +
> +  }, {request: {},response: { value: RetVal("json")}}),
> +
> +  screenshot: method(function() {
> +    let window = Services.wm.getMostRecentWindow("navigator:browser");

Hmm, this won't work for Thunderbird, but we have bug 880511 to fix it.

::: toolkit/devtools/server/tests/mochitest/test_device.html
@@ +16,5 @@
> +window.onload = function() {
> +  var Cu = Components.utils;
> +  var Ci = Components.interfaces;
> +
> +  Cu.import("resource://gre/modules/PermissionsTable.jsm") 

Trailing whitespace.

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 783034 [details] [diff] [review]
Patch v1

Review of attachment 783034 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/device.js
@@ +60,5 @@
> +
> +  }, {request: {},response: { value: RetVal("json")}}),
> +
> +  screenshot: method(function() {
> +    let window = Services.wm.getMostRecentWindow("navigator:browser");

With bug 880511, I think it would make sense to use the window from the parent actor. I could imagine that the screenshot command may either be used for a global screenshot of the main window, or just a content tab. In that case using an initialize function and this.parentActor.window would be the solution.

As mentioned bug is landed in fx-team, I'd appreciate if you could fix this in the patch.

Comment 14

[Paul Rouget [:paul]]

A new version of this patch will come soon. The information exposed will change a bit (we will use the same information as the Nightly Developer Tools extension). I will re-ask for a review.

Comment 15

[Paul Rouget [:paul]]

Attachment: patch v2

Addressing previous comments.

This actor is much more generic and should work everywhere (including Thunderbird).

I used some code from Nightly Tester Tools. See https://raw.github.com/mozilla/nightlytt/master/extension/chrome/content/nightly.js

I'm not very happy with the way the test works (retrieving the info the same way the actor does), but I think it's good enough.

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 787450 [details] [diff] [review]
patch v2

Review of attachment 787450 [details] [diff] [review]:
-----------------------------------------------------------------

* We should do the permission stuff in a followup.
* I do not get the point of screenshotToBlob with its current implementation.
* If I follow correctly, Jim and Panos are suggesting to move this getDescription() method 
to the RootActor. I don't have strong opinion on that, both scenarios works for me.
That won't change much codewise to put that code in one place or another,
so I would like to get their final feedback on that.

::: toolkit/devtools/server/actors/device.js
@@ +24,5 @@
> +
> +  _desc: null,
> +
> +  getAppIniString : function(section, key) {
> +    var directoryService = Services.dirsvc;

`directoryService` isn't used.

@@ +31,5 @@
> +
> +    if (!inifile.exists()) {
> +      inifile = Services.dirsvc.get("CurProcD", Ci.nsIFile);
> +      inifile.append("application.ini");
> +    }

We may ensure that inifile exits here before proceeding.

@@ +33,5 @@
> +      inifile = Services.dirsvc.get("CurProcD", Ci.nsIFile);
> +      inifile.append("application.ini");
> +    }
> +
> +    let iniParser = Cm.getClassObjectByContractID("@mozilla.org/xpcom/ini-parser-factory;1", Ci.nsIINIParserFactory).createINIParser(inifile);

Is there any particular reason to not use:
Cc["@mozilla.org/xpcom/ini-parser-factory;1"].getService(Ci.nsIINIParserFactory).createINIParser ?

@@ +65,5 @@
> +        geckoversion: appInfo.platformVersion,
> +        changeset: this.getAppIniString("App", "SourceStamp"),
> +        useragent: win.navigator.userAgent,
> +        locale: Cc["@mozilla.org/chrome/chrome-registry;1"].getService(Ci.nsIXULChromeRegistry).getSelectedLocale("global"),
> +        os: appInfo.OS,

I'm not sure appInfo.OS will be what we want for b2g.
Can you verify its value on b2g device?

What we do want are those two configure variables:
  http://mxr.mozilla.org/mozilla-central/source/b2g/confvars.sh#13
And especially `MOZ_B2G_VERSION`, that is hopefully exposed 
as pref that you should be able to fetch with:
`Services.prefs.getCharPref("b2g.version")`
The other pref isn't exposed, but we can patch gecko to expose it as a pref as well...

@@ +75,5 @@
> +        channel: null,
> +        profile: null,
> +        width: null,
> +        height: null,
> +      }

nit: `;` at EOL

@@ +81,5 @@
> +      // brandname
> +      let bundle = Services.strings.createBundle("chrome://branding/locale/brand.properties");
> +      if (bundle) {
> +        this._desc.brandShortName = bundle.GetStringFromName("brandShortName");
> +        this._desc.brandFullName = bundle.GetStringFromName("brandFullName");

Have you tried this on b2g, I'm expecting to see Mozilla Firefox here.
The b2g branding story is quite messy... the branding is mostly done by gaia build system.
There is still branding files being shipped in b2g's gecko, but I'm far from being sure they are correctly set.

@@ +94,5 @@
> +        if (profile.rootDir.path == profd.path) {
> +          this._desc.profile = profile.name;
> +          break;
> +        }
> +      }

nit: May be helpfull to move "profile name" computing in an helper function. Or at least put a comment saying that we try to guess the profile name.

@@ +107,5 @@
> +      } catch(e) {}
> +
> +    }
> +
> +    // Dynamic data

Dynamic data?

@@ +139,5 @@
> +      ALLOW_ACTION: Ci.nsIPermissionManager.ALLOW_ACTION,
> +      DENY_ACTION: Ci.nsIPermissionManager.DENY_ACTION,
> +      PROMPT_ACTION: Ci.nsIPermissionManager.PROMPT_ACTION
> +    };
> +  }, {request: {},response: { value: RetVal("json")}})

Two things:
 * I'm half convinced permission table should live in the device actor.
 * But what I dislike the most is that we make no abstraction at all on top of PermissionTable.jsm,
so that if anything change in the actual implementation of gecko permissions, we will most likely
have to break our clients. I think we are due to expose a precise API with clear usecases like
list, has, isAllowed and so on (based on client neeeds).
Dumping the internal gecko object doesn't sounds like a safe practice.

@@ +155,5 @@
> +    let deferred = promise.defer();
> +    this.screenshotToDataURL().then(longstr => {
> +      longstr.string().then(dataURL => {
> +        longstr.release().then(null, console.error);
> +        let win = Services.appShell.hiddenDOMWindow;

Please avoid doing anything close or far with the hidden window!
  const {CC} = require("chrome");
  let XMLHttpRequest = CC("@mozilla.org/xmlextras/xmlhttprequest;1");

@@ +161,5 @@
> +        req.open("GET", dataURL, true);
> +        req.responseType = "blob";
> +        req.onload = () => {
> +          let blob = req.response;
> +          deferred.resolve(win.URL.createObjectURL(blob));

I do not get the point of this method if we end up not returning a blob, but an URL??
And we have to be carefull about createObjectURL,
the code using this front *has to* call revokeObjectURL, otherwise we will leak the whole url's content!

@@ +164,5 @@
> +          let blob = req.response;
> +          deferred.resolve(win.URL.createObjectURL(blob));
> +        };
> +        req.onerror = () => {
> +          return deferred.reject();

Can we forward some meaningfull error?
Like deferred.reject(req.status/req.statusText) or anything...

@@ +168,5 @@
> +          return deferred.reject();
> +        }
> +        req.send();
> +      });
> +    });

Can `screenshotToDataURL()`'s promise be rejected?

If yes, we should also reject the promise we return:
  let deferred = promise.defer();
  this.screenshotToDataURL().then(longstr => {
    ...
  }, deferred.reject);
  return deferred.promise;
-or- directly return the promise:
  return this.screenshotToDataURL().then(longstr => {
    let deferred = promise.defer();
    ...
    return deferred.promise;
  });

Comment 17

[Jim Blandy :jimb]

Comment on attachment 787450 [details] [diff] [review]
patch v2

Review of attachment 787450 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me.

::: toolkit/devtools/server/actors/device.js
@@ +175,5 @@
> +});
> +
> +const _knownDeviceFronts = new WeakMap();
> +
> +exports.getDeviceFront = function(client, form) {

I think it's totally fine to just store the front as a property on the client, with some nice distinctive name, and ditch the WeakMap.

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Paul Rouget [:paul] from comment #15)
> This actor is much more generic and should work everywhere (including
> Thunderbird).

Thanks for keeping Thunderbird in Mind :-)

Comment 19

[Paul Rouget [:paul]]

Attachment: Patch v2.1

(In reply to Alexandre Poirot (:ochameau) from comment #16)
> * We should do the permission stuff in a followup.

See below.

> * I do not get the point of screenshotToBlob with its current implementation.

We return a blob now, not a URL.

> ::: toolkit/devtools/server/actors/device.js
> @@ +24,5 @@
> > +
> > +  _desc: null,
> > +
> > +  getAppIniString : function(section, key) {
> > +    var directoryService = Services.dirsvc;
> 
> `directoryService` isn't used.

Fixed.

> @@ +31,5 @@
> > +
> > +    if (!inifile.exists()) {
> > +      inifile = Services.dirsvc.get("CurProcD", Ci.nsIFile);
> > +      inifile.append("application.ini");
> > +    }
> 
> We may ensure that inifile exits here before proceeding.

Done.

> 
> @@ +33,5 @@
> > +      inifile = Services.dirsvc.get("CurProcD", Ci.nsIFile);
> > +      inifile.append("application.ini");
> > +    }
> > +
> > +    let iniParser = Cm.getClassObjectByContractID("@mozilla.org/xpcom/ini-parser-factory;1", Ci.nsIINIParserFactory).createINIParser(inifile);
> 
> Is there any particular reason to not use:
> Cc["@mozilla.org/xpcom/ini-parser-factory;1"].getService(Ci.
> nsIINIParserFactory).createINIParser ?

Nope. Fixed.

> 
> @@ +65,5 @@
> > +        geckoversion: appInfo.platformVersion,
> > +        changeset: this.getAppIniString("App", "SourceStamp"),
> > +        useragent: win.navigator.userAgent,
> > +        locale: Cc["@mozilla.org/chrome/chrome-registry;1"].getService(Ci.nsIXULChromeRegistry).getSelectedLocale("global"),
> > +        os: appInfo.OS,
> 
> I'm not sure appInfo.OS will be what we want for b2g.
> Can you verify its value on b2g device?
> 
> What we do want are those two configure variables:
>   http://mxr.mozilla.org/mozilla-central/source/b2g/confvars.sh#13
> And especially `MOZ_B2G_VERSION`, that is hopefully exposed 
> as pref that you should be able to fetch with:
> `Services.prefs.getCharPref("b2g.version")`
> The other pref isn't exposed, but we can patch gecko to expose it as a pref
> as well...


Addressed, this way:
+    if (desc.apptype == "b2g") {
+      // B2G specific
+      desc.os = "B2G";
+
+      return this._getSetting('deviceinfo.hardware')
+      .then(value => desc.hardware = value)
+      .then(() => this._getSetting('deviceinfo.os'))
+      .then(value => desc.version = value)
+      .then(() => desc);
+    }

I force "os" to "B2G".
I use "deviceinfo.os" to fetch the version. See:
http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js#172


> @@ +75,5 @@
> > +        channel: null,
> > +        profile: null,
> > +        width: null,
> > +        height: null,
> > +      }
> 
> nit: `;` at EOL
> 
> @@ +81,5 @@
> > +      // brandname
> > +      let bundle = Services.strings.createBundle("chrome://branding/locale/brand.properties");
> > +      if (bundle) {
> > +        this._desc.brandShortName = bundle.GetStringFromName("brandShortName");
> > +        this._desc.brandFullName = bundle.GetStringFromName("brandFullName");
> 
> Have you tried this on b2g, I'm expecting to see Mozilla Firefox here.
> The b2g branding story is quite messy... the branding is mostly done by gaia
> build system.
> There is still branding files being shipped in b2g's gecko, but I'm far from
> being sure they are correctly set.

Addressed. No brandName exposed for B2G.

> @@ +94,5 @@
> > +        if (profile.rootDir.path == profd.path) {
> > +          this._desc.profile = profile.name;
> > +          break;
> > +        }
> > +      }
> 
> nit: May be helpfull to move "profile name" computing in an helper function.
> Or at least put a comment saying that we try to guess the profile name.

I think it's pretty self-explanatory.

> @@ +107,5 @@
> > +      } catch(e) {}
> > +
> > +    }
> > +
> > +    // Dynamic data
> 
> Dynamic data?

meh.

> @@ +139,5 @@
> > +      ALLOW_ACTION: Ci.nsIPermissionManager.ALLOW_ACTION,
> > +      DENY_ACTION: Ci.nsIPermissionManager.DENY_ACTION,
> > +      PROMPT_ACTION: Ci.nsIPermissionManager.PROMPT_ACTION
> > +    };
> > +  }, {request: {},response: { value: RetVal("json")}})
> 
> Two things:
>  * I'm half convinced permission table should live in the device actor.
>  * But what I dislike the most is that we make no abstraction at all on top
> of PermissionTable.jsm,
> so that if anything change in the actual implementation of gecko
> permissions, we will most likely
> have to break our clients. I think we are due to expose a precise API with
> clear usecases like
> list, has, isAllowed and so on (based on client neeeds).
> Dumping the internal gecko object doesn't sounds like a safe practice.

Renaming to rawPermissionTable. AFAIK, there's no clean API on the platform side. If we really want a clean API, we should do it at the platform level, and then expose it to the actor.

> @@ +155,5 @@
> > +    let deferred = promise.defer();
> > +    this.screenshotToDataURL().then(longstr => {
> > +      longstr.string().then(dataURL => {
> > +        longstr.release().then(null, console.error);
> > +        let win = Services.appShell.hiddenDOMWindow;
> 
> Please avoid doing anything close or far with the hidden window!
>   const {CC} = require("chrome");
>   let XMLHttpRequest = CC("@mozilla.org/xmlextras/xmlhttprequest;1");

Fixed.

> @@ +161,5 @@
> > +        req.open("GET", dataURL, true);
> > +        req.responseType = "blob";
> > +        req.onload = () => {
> > +          let blob = req.response;
> > +          deferred.resolve(win.URL.createObjectURL(blob));
> 
> I do not get the point of this method if we end up not returning a blob, but
> an URL??
> And we have to be carefull about createObjectURL,
> the code using this front *has to* call revokeObjectURL, otherwise we will
> leak the whole url's content!

Fixed.

> @@ +164,5 @@
> > +          let blob = req.response;
> > +          deferred.resolve(win.URL.createObjectURL(blob));
> > +        };
> > +        req.onerror = () => {
> > +          return deferred.reject();
> 
> Can we forward some meaningfull error?
> Like deferred.reject(req.status/req.statusText) or anything...

Fixed.

> @@ +168,5 @@
> > +          return deferred.reject();
> > +        }
> > +        req.send();
> > +      });
> > +    });
> 
> Can `screenshotToDataURL()`'s promise be rejected?
> 
> If yes, we should also reject the promise we return:
>   let deferred = promise.defer();
>   this.screenshotToDataURL().then(longstr => {
>     ...
>   }, deferred.reject);
>   return deferred.promise;
> -or- directly return the promise:
>   return this.screenshotToDataURL().then(longstr => {
>     let deferred = promise.defer();
>     ...
>     return deferred.promise;
>   });

Fixed.

Comment 20

[Alexandre Poirot [:ochameau]]

Comment on attachment 788905 [details] [diff] [review]
Patch v2.1

Review of attachment 788905 [details] [diff] [review]:
-----------------------------------------------------------------

We agreed to expose raw permission table for the device inspector,
as it seems to be exactly what is needed for the device inspector.
But we may come back to permissions and expose an API,
when we are going to implement helpers and validators for local apps.
For example, we will want to ensure that an app manifest permissions
are correct regarding its type, or list permissions by app type, ...

The mochitest-chrome aren't executed on b2g, any chance you can do a mochitest with SpecialPowers?

::: toolkit/devtools/server/actors/device.js
@@ +33,5 @@
> +  typeName: "device",
> +
> +  _desc: null,
> +
> +  getAppIniString : function(section, key) {

nit: getAppIniString -> _getAppInitString

@@ +62,5 @@
> +        handle: (name, value) => deferred.resolve(value),
> +        handleError: (error) => deferred.reject(error),
> +      });
> +    } else {
> +      deferred.reject(new Error("Not settings service"));

nit: Not -> No?

Comment 21

[Paul Rouget [:paul]]

(In reply to Alexandre Poirot (:ochameau) from comment #20)
> The mochitest-chrome aren't executed on b2g, any chance you can do a
> mochitest with SpecialPowers?

I tried and failed.

Comment 22

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=63f429734ff3

Comment 23

[Paul Rouget [:paul]]

Attachment: patch v2.2

forgot to destroy server in test

Comment 24

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=fdf537f42bb5

Comment 25

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/bb057c2ca845

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bb057c2ca845