1022797 - Change FFOS device settings from WebIDE

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Jukka Jylänki]

I know that I can change B2G prefs with the edit-prefs.sh script in the B2G repository, but those don't seem to correspond with the device settings in the Settings API: https://developer.mozilla.org/en-US/Firefox_OS/Platform/Settings_list

How can I change the phone settings from the host computer like I can do for prefs? I tried to set the same items as prefs, but that doesn't quite work.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I don't believe this exists yet, but it's something we could add to the App Manager.

Comment 2

[Jukka Jylänki]

Do the settings get stored in some file on the disk that one could adb pull and push in the meanwhile?

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

No, the settings are stored in IndexedDB.  I am not aware of a safe way to edit the on-disk storage.  Likely the best approach is for us to add a DevTools actor that can manipulate the DB.

But, perhaps someone in #gaia has a way to do this against the on-disk files...  Not sure.

Comment 4

[Jukka Jylänki]

Oh, also, for such an actor, as well as setting a particular value, it would be useful to get the current value, and to clear/unset/reset a setting back to its default value.

Comment 5

[Paul Rouget [:paul]]

Building an actor for that should be very simple.

Comment 6

[Paul Rouget [:paul]]

Maybe we could just add a method to /toolkit/devtools/server/actors/device.js. We already have _getSetting.

Comment 8

[Jen Fong-Adwent [:ednapiranha]]

Hey Paul, what are your thoughts about accessing the settings in devtools in the default access restricted mode?

Comment 9

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Settings is lower risk that preferences. Actually settings are already completely exposed since you can side-load a certified apps with the settings permission. 

Some suggestions:
- put a scary warning/disclaimer telling people that messing with settings can brick your phone (e.g. change homescreen url and turn of debugging)
- reading settings you can get the user's passcode without them knowing (steal their phone while its unlocked, plug it into firefox grab the setting, put the phone back). We can (and should) probably mitigate this by storing a digest of the password instead. (though the attack will likely still be possible since a 4-digit pin could be brute forced. Maybe we should blacklist lockscreen.* But again, you could sideload a certified app to achieve the same thing, so maybe not worth the effort. 

You could do lots of nasty things to the phone (turn on FMD and track the user etc etc). But its certainly nothing that isn't already possible if you allow a sideloading certified app with the settings permission.

So my thought would be that this would be ok, but we just need to have the appropriate warning that its possible that you will brick your phone if you mess with settings. (worst case you might need to reflash, all not all vendors provide flashing tools afaik)

Comment 10

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1022797.patch

Still WIP but I am having issues with getting AppManager.settingsFront to be true. It seems to only be true during the local runtime but not any of the FX OS simulators. I also noticed it not being listed in vim gecko-dev/addon-sdk/source/lib/dev/volcan.js. Any idea on what I might have missed?

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jen Fong-Adwent [:ednapiranha] from comment #10)
> Created attachment 8526881 [details] [diff] [review]
> Bug1022797.patch
> 
> Still WIP but I am having issues with getting AppManager.settingsFront to be
> true. It seems to only be true during the local runtime but not any of the
> FX OS simulators.

As discussed on IRC, you need to rebuild the simulator[1] with your actor changes.

> I also noticed it not being listed in vim
> gecko-dev/addon-sdk/source/lib/dev/volcan.js. Any idea on what I might have
> missed?

volcan is for add-ons only.  It's not used at all by core DevTools code in the tree.  You can safely ignore it.

[1]: https://gist.github.com/jryans/b7e190342ab49c16ccca

Comment 12

[Jen Fong-Adwent [:ednapiranha]]

Attachment: bug1022797.patch

WIP: mainly looking for feedback on the following right now -

1. Whether the refactoring looks like it's correct
2. Ignore test_device_settings.html for now since I haven't updated it with the latest changes
3. I noticed that the tests fail if I call new DeviceManager in both preferences and settings - is there something I am doing incorrectly to make it fail if a new instance is created for both? If I comment out the call on devicesettings.js then devicepreferences.js works fine.

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Planning to check this out tomorrow (Weds).

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8533257 [details] [diff] [review]
bug1022797.patch

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

For the UI refactor, overall it looks you are able to share most of the view code, so that's great!

I wanted to test and provide more comments, but that's blocked by the bug you noted where you can share DeviceManager with both prefs and settings at the moment.  I added some comments on how to resolve this.

This has ended up being quite a large patch.  In the future, it's easier on you and reviewers to split up work a bit more.  That way you can get earlier feedback in case the reviewer sees a design flaw, for example.  For the reviewer, it mean less files to review in a given session.  It's better overall!  In this case, I think it would have been good to separate at least the actor side from the UI part.  If separate bugs feels like too much overhead, you can also use several patches in the same bug.  But this is just feedback for future work.  I don't think there's a reason to change for this bug.

::: browser/devtools/webide/content/devicepreferences.js
@@ +11,2 @@
>  
> +let DMPref = new DeviceManager(window.document);

Name this variable something starting with a lowercase letter since it's an instance, like |deviceManager|.  No need to say "Pref" in the name, as we're already in the prefs file and we can't collide with others inside an iframe.

@@ +19,5 @@
> +  document.getElementById("device-fields").onclick = CheckReset;
> +  document.getElementById("search-bar").onkeyup = document.getElementById("search-bar").onclick = SearchField;
> +  document.getElementById("custom-value").onclick = UpdateNewField;
> +  document.getElementById("custom-value-type").onchange = ClearNewFields;
> +  document.getElementById("add-custom-field").onkeyup = CheckNewFieldSubmit;

You can connect these to DeviceManager instance methods directly, to avoid all the tiny wrapper functions below.

::: browser/devtools/webide/modules/device-manager.js
@@ +8,5 @@
> +const EventEmitter = require("devtools/toolkit/event-emitter");
> +const {Services} = Cu.import("resource://gre/modules/Services.jsm");
> +const Strings = Services.strings.createBundle("chrome://browser/locale/devtools/webide.properties");
> +
> +let doc;

This is the cause of things not working on both settings and prefs.

You can't store the document in this module global as every instance will refer to it.

So, remove this and...

@@ +12,5 @@
> +let doc;
> +
> +let DeviceManager;
> +
> +module.exports = DeviceManager = function(document) {

Let's name this thing |TableView| or something...  That name isn't really the best either, but I want to make it clear that this file is a UI helper.  It's also not specific to devices.  It just helps you implement a particular UI component.

The module should have a comment block about it's purpose and how it's meant to be used.

@@ +14,5 @@
> +let DeviceManager;
> +
> +module.exports = DeviceManager = function(document) {
> +  EventEmitter.decorate(this);
> +  doc = document;

...store this in |this._doc| instead.

@@ +40,5 @@
> +    }
> +    return input;
> +  },
> +
> +  set deviceFront(options) {

Maybe just |front| instead.

@@ +45,5 @@
> +    this._deviceFront = options.front;
> +    this._shortName = options.shortName;
> +  },
> +
> +  set deviceKeys(keys) {

Maybe just |keys| instead.

@@ +53,5 @@
> +  get deviceKeys() {
> +    return this._deviceKeys;
> +  },
> +
> +  search: function(event) {

I am assuming all these methods were roughly cut and paste from preferences work, so I have not reviewed them in detail for now.

::: toolkit/devtools/server/actors/settings.js
@@ +4,5 @@
> +
> +const {Cc, Ci, Cu, CC} = require("chrome");
> +const protocol = require("devtools/server/protocol");
> +const {Arg, method, RetVal} = protocol;
> +const {Promise: promise} = Cu.import("resource://gre/modules/Promise.jsm", {});

The loader exposes a version of promises already, use:

const promise = require("promise");

@@ +76,5 @@
> +  _hasUserSetting: function(name, value) {
> +    return (settingsObj[name] !== value);
> +  },
> +
> +  getBoolSetting: method(function(name) {

I am assuming you used get<TYPE>Setting and set<TYPE>Setting to make the interface look like the one for prefs, which is a a good thought overall...

However, the Settings API does not have special per-type method, so could we just expose get/setSetting for this actor instead?  It seems odd to impose types here when the underlying API does not have them already.

Does that make the UI refactor much more complex if the interface is different from prefs?

::: toolkit/devtools/server/tests/mochitest/chrome.ini
@@ +70,5 @@
>  [test_memory_attach_02.html]
>  [test_memory_census.html]
>  [test_memory_gc_01.html]
>  [test_preference.html]
> +[test_settings.html]

This test may or may not run in all environments.  A try push will help figure that out.

If it fails, we can think about what to change, or at least disable it in unexpected environments.

Comment 15

[Jen Fong-Adwent [:ednapiranha]]

(In reply to J. Ryan Stinnett [:jryans] from comment #14)
> Comment on attachment 8533257 [details] [diff] [review]
> bug1022797.patch
> 
> Review of attachment 8533257 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For the UI refactor, overall it looks you are able to share most of the view
> code, so that's great!
> 
> I wanted to test and provide more comments, but that's blocked by the bug
> you noted where you can share DeviceManager with both prefs and settings at
> the moment.  I added some comments on how to resolve this.
> 
> This has ended up being quite a large patch.  In the future, it's easier on
> you and reviewers to split up work a bit more.  That way you can get earlier
> feedback in case the reviewer sees a design flaw, for example.  For the
> reviewer, it mean less files to review in a given session.  It's better
> overall!  In this case, I think it would have been good to separate at least
> the actor side from the UI part.  If separate bugs feels like too much
> overhead, you can also use several patches in the same bug.  But this is
> just feedback for future work.  I don't think there's a reason to change for
> this bug.
> 
> ::: browser/devtools/webide/content/devicepreferences.js
> @@ +11,2 @@
> >  
> > +let DMPref = new DeviceManager(window.document);
> 
> Name this variable something starting with a lowercase letter since it's an
> instance, like |deviceManager|.  No need to say "Pref" in the name, as we're
> already in the prefs file and we can't collide with others inside an iframe.
> 
> @@ +19,5 @@
> > +  document.getElementById("device-fields").onclick = CheckReset;
> > +  document.getElementById("search-bar").onkeyup = document.getElementById("search-bar").onclick = SearchField;
> > +  document.getElementById("custom-value").onclick = UpdateNewField;
> > +  document.getElementById("custom-value-type").onchange = ClearNewFields;
> > +  document.getElementById("add-custom-field").onkeyup = CheckNewFieldSubmit;
> 
> You can connect these to DeviceManager instance methods directly, to avoid
> all the tiny wrapper functions below.
> 
> ::: browser/devtools/webide/modules/device-manager.js
> @@ +8,5 @@
> > +const EventEmitter = require("devtools/toolkit/event-emitter");
> > +const {Services} = Cu.import("resource://gre/modules/Services.jsm");
> > +const Strings = Services.strings.createBundle("chrome://browser/locale/devtools/webide.properties");
> > +
> > +let doc;
> 
> This is the cause of things not working on both settings and prefs.
> 
> You can't store the document in this module global as every instance will
> refer to it.
> 
> So, remove this and...
> 
> @@ +12,5 @@
> > +let doc;
> > +
> > +let DeviceManager;
> > +
> > +module.exports = DeviceManager = function(document) {
> 
> Let's name this thing |TableView| or something...  That name isn't really
> the best either, but I want to make it clear that this file is a UI helper. 
> It's also not specific to devices.  It just helps you implement a particular
> UI component.

I don't really like the name TableView since it assumes we'll always be tied to a table (which we will likely but you never know) so what do you think of something like ConfigView?

> 
> The module should have a comment block about it's purpose and how it's meant
> to be used.
> 
> @@ +14,5 @@
> > +let DeviceManager;
> > +
> > +module.exports = DeviceManager = function(document) {
> > +  EventEmitter.decorate(this);
> > +  doc = document;
> 
> ...store this in |this._doc| instead.
> 
> @@ +40,5 @@
> > +    }
> > +    return input;
> > +  },
> > +
> > +  set deviceFront(options) {
> 
> Maybe just |front| instead.
> 
> @@ +45,5 @@
> > +    this._deviceFront = options.front;
> > +    this._shortName = options.shortName;
> > +  },
> > +
> > +  set deviceKeys(keys) {
> 
> Maybe just |keys| instead.
> 
> @@ +53,5 @@
> > +  get deviceKeys() {
> > +    return this._deviceKeys;
> > +  },
> > +
> > +  search: function(event) {
> 
> I am assuming all these methods were roughly cut and paste from preferences
> work, so I have not reviewed them in detail for now.
> 
> ::: toolkit/devtools/server/actors/settings.js
> @@ +4,5 @@
> > +
> > +const {Cc, Ci, Cu, CC} = require("chrome");
> > +const protocol = require("devtools/server/protocol");
> > +const {Arg, method, RetVal} = protocol;
> > +const {Promise: promise} = Cu.import("resource://gre/modules/Promise.jsm", {});
> 
> The loader exposes a version of promises already, use:
> 
> const promise = require("promise");
> 
> @@ +76,5 @@
> > +  _hasUserSetting: function(name, value) {
> > +    return (settingsObj[name] !== value);
> > +  },
> > +
> > +  getBoolSetting: method(function(name) {
> 
> I am assuming you used get<TYPE>Setting and set<TYPE>Setting to make the
> interface look like the one for prefs, which is a a good thought overall...
> 
> However, the Settings API does not have special per-type method, so could we
> just expose get/setSetting for this actor instead?  It seems odd to impose
> types here when the underlying API does not have them already.
> 
> Does that make the UI refactor much more complex if the interface is
> different from prefs?

It does in the sense that we use the HTML5 input types for numbers, booleans and strings. If we just use the plain get/set for Settings then we still need to find a way to check what type it should be and render it correctly on the interface. We don't want someone to pass 'a' as a number or a boolean and a 5 as a boolean, etc. Not really sure of a better way to get around this but am open to ideas!

> 
> ::: toolkit/devtools/server/tests/mochitest/chrome.ini
> @@ +70,5 @@
> >  [test_memory_attach_02.html]
> >  [test_memory_census.html]
> >  [test_memory_gc_01.html]
> >  [test_preference.html]
> > +[test_settings.html]
> 
> This test may or may not run in all environments.  A try push will help
> figure that out.
> 
> If it fails, we can think about what to change, or at least disable it in
> unexpected environments.

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jen Fong-Adwent [:ednapiranha] from comment #15)
> (In reply to J. Ryan Stinnett [:jryans] from comment #14)
> > @@ +12,5 @@
> > > +let doc;
> > > +
> > > +let DeviceManager;
> > > +
> > > +module.exports = DeviceManager = function(document) {
> > 
> > Let's name this thing |TableView| or something...  That name isn't really
> > the best either, but I want to make it clear that this file is a UI helper. 
> > It's also not specific to devices.  It just helps you implement a particular
> > UI component.
> 
> I don't really like the name TableView since it assumes we'll always be tied
> to a table (which we will likely but you never know) so what do you think of
> something like ConfigView?

Okay, seems fine to me.

> > @@ +76,5 @@
> > > +  _hasUserSetting: function(name, value) {
> > > +    return (settingsObj[name] !== value);
> > > +  },
> > > +
> > > +  getBoolSetting: method(function(name) {
> > 
> > I am assuming you used get<TYPE>Setting and set<TYPE>Setting to make the
> > interface look like the one for prefs, which is a a good thought overall...
> > 
> > However, the Settings API does not have special per-type method, so could we
> > just expose get/setSetting for this actor instead?  It seems odd to impose
> > types here when the underlying API does not have them already.
> > 
> > Does that make the UI refactor much more complex if the interface is
> > different from prefs?
> 
> It does in the sense that we use the HTML5 input types for numbers, booleans
> and strings. If we just use the plain get/set for Settings then we still
> need to find a way to check what type it should be and render it correctly
> on the interface. We don't want someone to pass 'a' as a number or a boolean
> and a 5 as a boolean, etc. Not really sure of a better way to get around
> this but am open to ideas!

I think it's harder to enforce that in the case of Settings though, because (potentially) someone might use a setting to hold different types, since the backend API doesn't mind.  As in, perhaps there is a setting "foo" that allows both 0 and "yes" as meaningful values.  Maybe there isn't, but nothing really prevents it today.

However, I've noticed we do store more complex types too, like arrays[1], in Settings.  So, we can't restrict ourselves to just bool, int, and string types.

Maybe for Settings we can't do as much type checking?  Granted, it does seem odd (and hopefully quite unusual) for a single setting to have different types over time.  If we want to keep the type checking though, it should be based on the type of the value you initially pull down I'd think.  I don't see a reason to enforce that with explicit set<TYPE> methods because then you have something that is not as powerful as the actual Settings API.

It does seem like we'll at least need some way to edit array / object types.  Maybe we should treat everything as one type and present the JSON representation of the value for editing (at least for these array / object values), since that's probably the best we can do with a simple text field.

[1]: https://github.com/mozilla-b2g/gaia/blob/90c1b9c146b2a4abe545bf758f2e47d898820ad1/build/config/common-settings.json#L35

Comment 17

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1022797.patch

Looking for the following feedback/assistance:

1. General overall structure/code setup

2. Still getting that error about "Protocol error (unrecognizedPacketType): Actor conn1.settingsActor19 does not recognize the packet type setSetting" when trying to trigger a setting update but not really sure what is causing it. I suspect it does not like my assignment of this._front.setSetting/getSetting but not sure why. Device preferences seems to work fine as is so I feel like I am missing something minor somewhere. I did try rebuilding the b2g version and that did not help.

3. When trying to do a direct call of configView's functions in devicesettings.js and devicepreferences.js in the click/change/keyup handlers, it did not work because the document (through configView's reference) did not seem to be loaded yet. So setting it in the way you suggested makes configView.doc end up undefined.

4. Since the settings interface contains an object type, I set up an additional "object" in the dropdown when creating a new setting. Seems to work but let me know if you have any improvements you see/recommend.

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8538996 [details] [diff] [review]
Bug1022797.patch

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

(In reply to Jen Fong-Adwent [:ednapiranha] from comment #17)
> Created attachment 8538996 [details] [diff] [review]
> Bug1022797.patch
> 
> Looking for the following feedback/assistance:
> 
> 1. General overall structure/code setup

Overall, I think it seems good!  I added a few specific comments.

> 2. Still getting that error about "Protocol error (unrecognizedPacketType):
> Actor conn1.settingsActor19 does not recognize the packet type setSetting"
> when trying to trigger a setting update but not really sure what is causing
> it. I suspect it does not like my assignment of
> this._front.setSetting/getSetting but not sure why. Device preferences seems
> to work fine as is so I feel like I am missing something minor somewhere. I
> did try rebuilding the b2g version and that did not help.

I am not quite sure what to suggest... :/ For me, it worked fine with both b2g and local runtime.

Perhaps the directory you are building b2g in does not have your actor changes applied?

> 3. When trying to do a direct call of configView's functions in
> devicesettings.js and devicepreferences.js in the click/change/keyup
> handlers, it did not work because the document (through configView's
> reference) did not seem to be loaded yet. So setting it in the way you
> suggested makes configView.doc end up undefined.

Hmm, are you sure it's about |doc|?  I would think the issue might be that |this| is undefined in |configView|.  If that's it, a common fix is to bind methods you use as event handlers to the object they live on.  But, that's about the same amount of work as the separate functions you have now, so it's fine as-is I suppose.

> 4. Since the settings interface contains an object type, I set up an
> additional "object" in the dropdown when creating a new setting. Seems to
> work but let me know if you have any improvements you see/recommend.

The object type seems to work well to me.

Styling:

At lower widths, the first column on the settings view is small enough that the input field wraps below the select menu.  We should make some kind of change to prevent this.

::: browser/devtools/webide/modules/config-view.js
@@ +12,5 @@
> +let ConfigView;
> +
> +module.exports = ConfigView = function(window) {
> +  EventEmitter.decorate(this);
> +  this.doc = window.document;

Our style is to use |this._doc| since this a private field.

@@ +43,5 @@
> +    return input;
> +  },
> +
> +  set front(options) {
> +    this._front = options.front;

This is a bit odd that you set |front| to an object... that contains another thing also called |front|, plus a name.

Maybe call this |target| and keep on passing the object.  Or, break this out into two setters, one for real front, and one for shortName.

@@ +151,5 @@
> +    return table;
> +  },
> +
> +  _getCallType: function(type, name) {
> +    if (this._shortName === "Setting") {

Instead of requiring this specific name, add another option like "typeBasedFront": true / false for each page to set.

Then you can remove uses of "setting" and "pref" from this file.

::: browser/devtools/webide/themes/device.css
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Name this file the same as it's UI module, so |config-view.css|.

::: toolkit/devtools/server/actors/settings.js
@@ +57,5 @@
> +    };
> +    return deferred.promise;
> +  }, {
> +    request: { value: Arg(0) },
> +    response: { value: RetVal("string") }

I am a bit surprised that this works!

Do you get a "real" value (not stringified version) back for all possible types?

@@ +65,5 @@
> +    let deferred = promise.defer();
> +    let data = {};
> +    // Try parsing string as an object - if this works, then we'll save as an object
> +    try {
> +      value = JSON.parse(value);

Looking at the protocol traffic, value is already an object.  So, do you need this?

@@ +92,5 @@
> +    return (settingsObj[name] !== value);
> +  },
> +
> +  // For tests
> +  setSettings: method(function (settings) {

Name this |_setSettings| since it's for tests only.

::: toolkit/devtools/server/main.js
@@ +377,5 @@
>          constructor: "ActorRegistryActor",
>          type: { global: true }
>        });
>      }
> +    if ("nsISettingsService" in Ci) {

This seems to be true even when navigator.mozSettings is not available.

So I think we'll need to do:

let win = Services.wm.getMostRecentWindow(DebuggerServer.chromeWindowType);
if (win.navigator.mozSettings) {

instead.

Comment 19

[Jen Fong-Adwent [:ednapiranha]]

(In reply to J. Ryan Stinnett [:jryans] (on PTO Dec. 23 - Dec. 28) from comment #18)
> Comment on attachment 8538996 [details] [diff] [review]
> Bug1022797.patch
> 
> Review of attachment 8538996 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Jen Fong-Adwent [:ednapiranha] from comment #17)
> > Created attachment 8538996 [details] [diff] [review]
> > Bug1022797.patch
> > 
> > Looking for the following feedback/assistance:
> > 
> > 1. General overall structure/code setup
> 
> Overall, I think it seems good!  I added a few specific comments.
> 
> > 2. Still getting that error about "Protocol error (unrecognizedPacketType):
> > Actor conn1.settingsActor19 does not recognize the packet type setSetting"
> > when trying to trigger a setting update but not really sure what is causing
> > it. I suspect it does not like my assignment of
> > this._front.setSetting/getSetting but not sure why. Device preferences seems
> > to work fine as is so I feel like I am missing something minor somewhere. I
> > did try rebuilding the b2g version and that did not help.
> 
> I am not quite sure what to suggest... :/ For me, it worked fine with both
> b2g and local runtime.
> 
> Perhaps the directory you are building b2g in does not have your actor
> changes applied?
> 
> > 3. When trying to do a direct call of configView's functions in
> > devicesettings.js and devicepreferences.js in the click/change/keyup
> > handlers, it did not work because the document (through configView's
> > reference) did not seem to be loaded yet. So setting it in the way you
> > suggested makes configView.doc end up undefined.
> 
> Hmm, are you sure it's about |doc|?  I would think the issue might be that
> |this| is undefined in |configView|.  If that's it, a common fix is to bind
> methods you use as event handlers to the object they live on.  But, that's
> about the same amount of work as the separate functions you have now, so
> it's fine as-is I suppose.
> 
> > 4. Since the settings interface contains an object type, I set up an
> > additional "object" in the dropdown when creating a new setting. Seems to
> > work but let me know if you have any improvements you see/recommend.
> 
> The object type seems to work well to me.
> 
> Styling:
> 
> At lower widths, the first column on the settings view is small enough that
> the input field wraps below the select menu.  We should make some kind of
> change to prevent this.
> 
> ::: browser/devtools/webide/modules/config-view.js
> @@ +12,5 @@
> > +let ConfigView;
> > +
> > +module.exports = ConfigView = function(window) {
> > +  EventEmitter.decorate(this);
> > +  this.doc = window.document;
> 
> Our style is to use |this._doc| since this a private field.
> 
> @@ +43,5 @@
> > +    return input;
> > +  },
> > +
> > +  set front(options) {
> > +    this._front = options.front;
> 
> This is a bit odd that you set |front| to an object... that contains another
> thing also called |front|, plus a name.
> 
> Maybe call this |target| and keep on passing the object.  Or, break this out
> into two setters, one for real front, and one for shortName.
> 
> @@ +151,5 @@
> > +    return table;
> > +  },
> > +
> > +  _getCallType: function(type, name) {
> > +    if (this._shortName === "Setting") {
> 
> Instead of requiring this specific name, add another option like
> "typeBasedFront": true / false for each page to set.
> 
> Then you can remove uses of "setting" and "pref" from this file.

Do you mean add a setter for ConfigView to handle this?

> 
> ::: browser/devtools/webide/themes/device.css
> @@ +1,1 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> Name this file the same as it's UI module, so |config-view.css|.
> 
> ::: toolkit/devtools/server/actors/settings.js
> @@ +57,5 @@
> > +    };
> > +    return deferred.promise;
> > +  }, {
> > +    request: { value: Arg(0) },
> > +    response: { value: RetVal("string") }
> 
> I am a bit surprised that this works!
> 
> Do you get a "real" value (not stringified version) back for all possible
> types?

I am pretty sure that is incorrect and returns a string - wasn't sure what to put there - RetVal("json")?

> 
> @@ +65,5 @@
> > +    let deferred = promise.defer();
> > +    let data = {};
> > +    // Try parsing string as an object - if this works, then we'll save as an object
> > +    try {
> > +      value = JSON.parse(value);
> 
> Looking at the protocol traffic, value is already an object.  So, do you
> need this?
> 
> @@ +92,5 @@
> > +    return (settingsObj[name] !== value);
> > +  },
> > +
> > +  // For tests
> > +  setSettings: method(function (settings) {
> 
> Name this |_setSettings| since it's for tests only.
> 
> ::: toolkit/devtools/server/main.js
> @@ +377,5 @@
> >          constructor: "ActorRegistryActor",
> >          type: { global: true }
> >        });
> >      }
> > +    if ("nsISettingsService" in Ci) {
> 
> This seems to be true even when navigator.mozSettings is not available.
> 
> So I think we'll need to do:
> 
> let win = Services.wm.getMostRecentWindow(DebuggerServer.chromeWindowType);
> if (win.navigator.mozSettings) {
> 
> instead.

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jen Fong-Adwent [:ednapiranha] from comment #19)
> > @@ +151,5 @@
> > > +    return table;
> > > +  },
> > > +
> > > +  _getCallType: function(type, name) {
> > > +    if (this._shortName === "Setting") {
> > 
> > Instead of requiring this specific name, add another option like
> > "typeBasedFront": true / false for each page to set.
> > 
> > Then you can remove uses of "setting" and "pref" from this file.
> 
> Do you mean add a setter for ConfigView to handle this?

Yes.  My name for the option wasn't that good... maybe you'll have a better one!

> > 
> > ::: browser/devtools/webide/themes/device.css
> > @@ +1,1 @@
> > > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > 
> > Name this file the same as it's UI module, so |config-view.css|.
> > 
> > ::: toolkit/devtools/server/actors/settings.js
> > @@ +57,5 @@
> > > +    };
> > > +    return deferred.promise;
> > > +  }, {
> > > +    request: { value: Arg(0) },
> > > +    response: { value: RetVal("string") }
> > 
> > I am a bit surprised that this works!
> > 
> > Do you get a "real" value (not stringified version) back for all possible
> > types?
> 
> I am pretty sure that is incorrect and returns a string - wasn't sure what
> to put there - RetVal("json")?

I looked into this a bit more, and my read of protocol.js is that all of the following types:

* primitive
* string
* number
* boolean
* json 

are functionally equivalent because it assumes all of them are safe to go over the wire as-is.  Even though some of your return types here are not really valid JSON, let's go with RetVal("json") as that seems to already be the general "normal stuff that is safe" used in other actors.

Comment 21

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1022797.patch

I tried rebuilding gecko b2g with the latest changes from fx-team (with full rebuild) and am still having an issue with loading the simulator. Tests are passing and when it was working it worked okay. I think this is ready for a review and hopefully it works on your local setup? If not, we'll have to investigate what the issue is.

Comment 22

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8549082 [details] [diff] [review]
Bug1022797.patch

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

Overall, getting closer!

I am able to run b2g with this patch, so that is good news at least.  Usually for me, the issues are in Gaia with problems like this.  Gecko and Gaia track each other closely, so if one is much older than the other, things will break.  Please retry:

1. Updating fx-team
2. Updating Gaia
3. Delete the Gaia profile folder
4. Rebuild the Gaia profile

The device_front_shared.js file is missing from this patch.

Still present since last review:

Styling:

At lower widths, the first column on the settings view is small enough that the input field wraps below the select menu.  We should make some kind of change to prevent this.

::: browser/devtools/webide/modules/config-view.js
@@ +163,5 @@
> +  },
> +
> +  _setCallType: function(type, name, value) {
> +    if (this._settingActiveFront) {
> +      return this._front.setSetting(name, value);

This is not quite what I was imagining last time...

ConfigView wants to be a general view component, so we should try to abstract away uses of the words "setting" and "pref", at least from the method names of ConfigView.

One approach is for each page is set some options, as in:

devicesettings.js
=================

configView.kind = "Setting";
configView.includeTypeName = false;

deviceprefs.js
==============

configView.kind = "Pref";
configView.includeTypeName = true;

Then every use of setting and pref can be removed from ConfigView.

Or, an alternate approach is for each page to supply an object that wraps the actual front:

devicesettings.js
=================

let front = AppManager.settingsFront;
configView.front = {

  getBool(name) {
    return front.getSetting(name);
  }

...

};

deviceprefs.js
==============

let front = AppManager.prefsFront;
configView.front = {

  getBool(name) {
    return front.getBoolPref(name);
  }

...

};

Either approach seems good to me, since they both mean ConfigView does not need to know anything about what a "setting" or "pref" is.

::: browser/devtools/webide/test/browser.ini
@@ +1,5 @@
>  [DEFAULT]
>  subsuite = devtools
>  support-files =
>    addons/simulators.json
> +  device_front_shared.js

This file is not used in any browser_* tests, so you don't need to add it here.

::: toolkit/devtools/server/actors/settings.js
@@ +65,5 @@
> +    let deferred = promise.defer();
> +    let data = {};
> +    // Try parsing string as an object - if this works, then we'll save as an object
> +    try {
> +      value = JSON.parse(value);

As with the previous review, I don't think this parsing step is needed.

Comment 23

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1022797.patch

Oops, sorry about forgetting to add that shared test file! As for gaia, I tried deleting the profile dir and building again - no such luck. Today I tried to just clone gaia from fresh with the custom settings, building rebuilding the gecko b2g too - still having that weird rendering issue.

Comment 24

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1022797.patch

Comment 25

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8549737 [details] [diff] [review]
Bug1022797.patch

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

Great, just one small cleanup!  No need for another review.

Time to push to try and hopefully land!

Also, be aware that the patch in bug 1122123 may land first, it fixes a test issue in preferences, so you may need to resolve a conflict if that happens.

::: browser/devtools/webide/modules/config-view.js
@@ +157,5 @@
> +
> +    return table;
> +  },
> +
> +  set settingActiveFront(active) {

Remove this.

Comment 26

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1022797.patch

Comment 27

[Jen Fong-Adwent [:ednapiranha]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=918b29d9a724

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing :(

Comment 29

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1022797.patch

I've updated the patch with a rebase from your device preferences changes. When I tried to add similar async changes to settings, I ran into problems with createLock and lock.get("*") (not existing) which caused failures in the mochitest-chrome tests.

Let me know if there is something I'm missing to get this working for that set of tests or if I should leave it as is.

Comment 30

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8550466 [details] [diff] [review]
Bug1022797.patch

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

Sorry for causing the rebase...

I think we should still yield on the |getAllPrefs| promise to ensure loading happens as expected and reduce intermittent test failures, so update the test to continue this.  Settings should also expose a similar |getAllSettings| or something for tests, and yield on it.

> When I tried to add similar async changes to settings, I ran into problems
> with createLock and lock.get("*") (not existing) which caused failures in
> the mochitest-chrome tests.

I am not sure what you mean here...  I believe the only change settings would need to line up with the changes in bug 1122123 is to also expose a |getAllSettings| promies and yield on it.  Is that what causes problems?

r+ assuming these issues are resolved.

Comment 31

[Jen Fong-Adwent [:ednapiranha]]

(In reply to J. Ryan Stinnett [:jryans] from comment #30)
> Comment on attachment 8550466 [details] [diff] [review]
> Bug1022797.patch
> 
> Review of attachment 8550466 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for causing the rebase...
> 
> I think we should still yield on the |getAllPrefs| promise to ensure loading
> happens as expected and reduce intermittent test failures, so update the
> test to continue this.  Settings should also expose a similar
> |getAllSettings| or something for tests, and yield on it.
> 
> > When I tried to add similar async changes to settings, I ran into problems
> > with createLock and lock.get("*") (not existing) which caused failures in
> > the mochitest-chrome tests.
> 
> I am not sure what you mean here...  I believe the only change settings
> would need to line up with the changes in bug 1122123 is to also expose a
> |getAllSettings| promies and yield on it.  Is that what causes problems?
> 
> r+ assuming these issues are resolved.

Yep, when I wrap the same changes to getAllSettings, all tests pass except for the `mach mochitest-chrome browser/devtools/webide` tests. It fails at reading settingsService.createLock().lock.get("*") (thinks it is undefined).

Comment 32

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1022797.patch

Updated with the changes for settings - this passes in all tests except `mach mochitest-chrome browser/devtools/webide`

Comment 33

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8551912 [details] [diff] [review]
Bug1022797.patch

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

As I said on IRC, I am sorry for missing these things in previous review rounds.  Some of them I just did not notice until I really dug in to debug the testing issue.

::: browser/devtools/webide/test/device_front_shared.js
@@ +86,5 @@
> +  newField.value = "test2";
> +  newField.click();
> +  is(newField.value, "test2", "Custom string existing value is correct");
> +
> +  // Add a new custom integer preference with a valid integer

Why is this work in a function called |editExistingField|?  It seems to make a new field?

@@ +105,5 @@
> +  is(customName.value, "", "Custom integer name reset");
> +  is(customValue.value, 0, "Custom integer value reset");
> +}
> +
> +function editFieldInteger() {

We don't want to have a normal function here, since you are using a |yield| statement later on.  If you test locally, you can throw an error at the top of this function, but the test passes anyway.

Change this to:

let editFieldInteger = Task.async(function*() {

...

});

@@ +107,5 @@
> +}
> +
> +function editFieldInteger() {
> +  // Edit existing custom integer preference
> +  newPref.value = 3;

This variable in undeclared.

@@ +126,5 @@
> +    ok(!found, "Custom field removed");
> +  }
> +}
> +
> +function resetExistingField(id) {

Change this to:

let resetExistingField = Task.async(function*(id) {

...

});

::: browser/devtools/webide/test/test_device_preferences.html
@@ +65,2 @@
>  
> +          editFieldInteger();

yield

@@ +67,2 @@
>  
> +          resetExistingField("accessibility.accesskeycausesactivation");

yield

::: browser/devtools/webide/test/test_device_settings.html
@@ +46,5 @@
> +
> +          win.Cmds.showSettings();
> +          is(deck.selectedPanel, settingIframe, "device settings iframe selected");
> +
> +          yield settingIframe.contentWindow.getAllSettings;

The key to getting this to pass as a standalone test is to add:

if (SpecialPowers.isMainProcess()) {
  Cu.import("resource://gre/modules/SettingsRequestManager.jsm");
}

You can add this near the top of the test like the other Cu.imports.

This yield line is actually quite critical.  What we've revealed by adding it is that we weren't actually testing the full integration before, since the connection to the settings actor was never working during this test.

@@ +59,5 @@
> +          addNewFieldWithEnter();
> +
> +          editExistingField();
> +
> +          editFieldInteger();

This will become

yield editFieldInteger();

@@ +61,5 @@
> +          editExistingField();
> +
> +          editFieldInteger();
> +
> +          resetExistingField("wifi.enabled");

This will become

yield resetExistingField("wifi.enabled");

::: toolkit/devtools/server/actors/settings.js
@@ +11,5 @@
> +Cu.import("resource://gre/modules/FileUtils.jsm");
> +Cu.import("resource://gre/modules/NetUtil.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +let settingsObj = {};

Name this |defaultSettings| or something else more explicit about its purpose.

@@ +13,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +let settingsObj = {};
> +let win = Services.wm.getMostRecentWindow(DebuggerServer.chromeWindowType);
> +let settingsService = win.navigator.mozSettings;

Caching the |settingsService| here is the main cause of the test failure.  This keeps the window and mozSettings alive beyond their useful lifetime.

Another issue is that actor files are run on both the server and the client (to create the front).  So, this means lines in the top-level scope like this are also run on the client, but that's not expected, since we only want them in the context of the server.  So, they have to be a part of the actor itself, instead of top-level in the file.

Create a |settingsService| getter on the actor, and use that to retrieve the window and settingsService instead.

@@ +86,5 @@
> +    return (settingsObj[name] !== value);
> +  },
> +
> +  // For tests
> +  _setSettings: method(function(settings) {

I don't think it's that good for this to be a |method|, since it is still exposed in production, but only meant for tests.

First, let's call it |_setDefaultSettings|.  Once it becomes a regular function, you'll need to get access to the actor instance from your test, since you can't access this method from the front anymore.  In your test, use something like:

  let connID = Object.keys(DebuggerServer._connections)[0];
  let conn = DebuggerServer._connections[connID];

  conn.getActor(front.actorID);

to get the actor instance, which you can then call |_setDefaultSettings| on.

@@ +96,5 @@
> +
> +  getAllSettings: method(function() {
> +    loadSettingsFile();
> +
> +    if (settingsFile && settingsFile.exists()) {

This is not the right place to populate |settingsObj|.  There are other actor methods that use it too.  If someone calls a different one before |getAllSettings|, the behavior will be incorrect.

Create a |defaultSettings| getter on the actor that does this work on the first call, and caches it after that.

::: toolkit/devtools/server/main.js
@@ +376,5 @@
>          type: { global: true }
>        });
>      }
> +    let win = Services.wm.getMostRecentWindow(DebuggerServer.chromeWindowType);
> +    if (win.navigator.mozSettings) {

Not every server use case has a window, such as a testing harness.

Use win && win.navigator.mozSettings.

::: toolkit/devtools/server/tests/mochitest/test_settings.html
@@ +23,5 @@
> +  Cu.import("resource://gre/modules/devtools/dbg-client.jsm");
> +  Cu.import("resource://gre/modules/devtools/dbg-server.jsm");
> +
> +  if (SpecialPowers.isMainProcess()) {
> +    SpecialPowers.Cu.import("resource://gre/modules/SettingsRequestManager.jsm");

Don't need |SpecialPowers| on this line, just use |Cu| directly.

Comment 34

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1022797.patch

Let's try this one :)

Comment 35

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8552789 [details] [diff] [review]
Bug1022797.patch

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

Much closer!  Will re-test the functionality tomorrow.

Here are a few coding fixes to make for now.

::: toolkit/devtools/server/actors/settings.js
@@ +12,5 @@
> +Cu.import("resource://gre/modules/NetUtil.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +let defaultSettings = {};
> +let win = Services.wm.getMostRecentWindow(DebuggerServer.chromeWindowType);

Move this into |_getSettingsService|.

@@ +36,5 @@
> +
> +  try {
> +    defaultSettings = JSON.parse(rawstr);
> +  } catch(e) {
> +    if (DEBUG) debug("Error parsing " + settingsFile.path + " : " + e);

Remove this, DEBUG is never defined.

@@ +37,5 @@
> +  try {
> +    defaultSettings = JSON.parse(rawstr);
> +  } catch(e) {
> +    if (DEBUG) debug("Error parsing " + settingsFile.path + " : " + e);
> +    return;

Remove this, we should always close the stream.

@@ +140,5 @@
> +    request: {},
> +    response: { value: RetVal("json") }
> +  }),
> +
> +  clearUserSetting: method(function(name) {

Add loadSettingsFile()

::: toolkit/devtools/server/tests/mochitest/test_settings.html
@@ +23,5 @@
> +  Cu.import("resource://gre/modules/devtools/dbg-client.jsm");
> +  Cu.import("resource://gre/modules/devtools/dbg-server.jsm");
> +
> +  if (SpecialPowers.isMainProcess()) {
> +    SpecialPowers.Cu.import("resource://gre/modules/SettingsRequestManager.jsm");

Remove SP, see prev review.

Comment 36

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1022797.patch

Comment 37

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8553270 [details] [diff] [review]
Bug1022797.patch

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

Okay, looks great!  I hope try agrees.

Comment 38

[Jen Fong-Adwent [:ednapiranha]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c35aff20b4a

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 41

[Francesco Lodolo [:flod]]

I'm really having a hard time understanding this bug. What's the difference between preferences and settings in this context?

Comment 42

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Francesco Lodolo [:flod] from comment #41)
> I'm really having a hard time understanding this bug. What's the difference
> between preferences and settings in this context?

They are different data storage APIs:

Preferences[1] vs. Settings[2]

Preferences are used widely in many Gecko apps.  Settings (AIUI) were added for Firefox OS, and they are what you control via the Settings app in FxOS.

Now that this bug has landed, WebIDE is able to control both preferences and settings of a remote device.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPrefBranch
[2]: https://developer.mozilla.org/en-US/docs/Web/API/Settings_API

Comment 43

[Will Bamberg [:wbamberg]]

-> https://developer.mozilla.org/en-US/docs/Tools/WebIDE#Runtime_menu_items - let me know if this is OK.

Comment 44

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Will Bamberg [:wbamberg] from comment #43)
> -> https://developer.mozilla.org/en-US/docs/Tools/WebIDE#Runtime_menu_items
> - let me know if this is OK.

Looks good!  Maybe it's helpful to distinguish prefs and settings?  Device Prefs are the Gecko / platform level configuration values, exposing the same set of data as Firefox's about:config (but for the device).  Device Settings are the configuration values used by the Settings app and others on Firefox OS.  So, most things on device with a UI control to change (volume, alarm, etc.) are found in Device Settings.

Comment 45

[Ton [:Tonnes]]

>> +<!ENTITY runtimeMenu_showSettings_label "Device Settings">
>> +<!ENTITY runtimeMenu_showSettings_accesskey "s">

This key is causing a (second) double access key in the Runtime menu. Also please keep access keys case sensitive.