[app manager] Device meta data actor

RESOLVED FIXED in Firefox 26

Status

()

Firefox
Developer Tools: WebIDE
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

Trunk
Firefox 26
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

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

OS version (Firefox OS, Android, ...)
Gecko version
IMEI
phone number
screen size & dpi
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

5 years ago
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

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

Comment 4

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

Comment 5

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

Comment 6

5 years ago
Not about:logo, but chrome://branding/content/about-logo.png.
(Assignee)

Updated

5 years ago
Assignee: nobody → paul
Component: Developer Tools → Developer Tools: App Manager
(Assignee)

Updated

5 years ago
Duplicate of this bug: 895361
(Assignee)

Comment 8

5 years ago
Created attachment 781755 [details] [diff] [review]
895360.diff
(Assignee)

Updated

5 years ago
Depends on: 898394
(Assignee)

Updated

5 years ago
Blocks: 898485
(Assignee)

Comment 9

5 years ago
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.
Attachment #781755 - Flags: review?(past)
(Assignee)

Comment 10

4 years ago
Created attachment 783034 [details] [diff] [review]
Patch v1

Panos, what do you think of this? A global device actor that is "on the side".
Attachment #781755 - Attachment is obsolete: true
Attachment #781755 - Flags: review?(past)
Attachment #783034 - Flags: review?(past)
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.
Attachment #783034 - Flags: review?(past) → review+
(Assignee)

Updated

4 years ago
Depends on: 880511
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.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 895364
(Assignee)

Updated

4 years ago
No longer blocks: 898485
(Assignee)

Comment 14

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

Comment 15

4 years ago
Created attachment 787450 [details] [diff] [review]
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.
Attachment #783034 - Attachment is obsolete: true
Attachment #787450 - Flags: review?(poirot.alex)
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;
  });
Attachment #787450 - Flags: review?(poirot.alex)
Attachment #787450 - Flags: feedback?(past)
Attachment #787450 - Flags: feedback?(jimb)

Comment 17

4 years ago
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.
Attachment #787450 - Flags: feedback?(jimb) → feedback+
(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 :-)
(Assignee)

Comment 19

4 years ago
Created attachment 788905 [details] [diff] [review]
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.
Attachment #788905 - Flags: review?(poirot.alex)
(Assignee)

Updated

4 years ago
Attachment #787450 - Attachment is obsolete: true
Attachment #787450 - Flags: feedback?(past)
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?
Attachment #788905 - Flags: review?(poirot.alex) → review+
(Assignee)

Comment 21

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

Comment 22

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=63f429734ff3
(Assignee)

Comment 23

4 years ago
Created attachment 789584 [details] [diff] [review]
patch v2.2

forgot to destroy server in test
(Assignee)

Comment 24

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=fdf537f42bb5
(Assignee)

Comment 25

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/bb057c2ca845
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/bb057c2ca845
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.