943251 - Server support for preferences

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect, P2)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

It would be handy for developers to modify the preferences without restart b2g.

Comment 1

[Paul Rouget [:paul]]

(In reply to Kan-Ru Chen [:kanru] from comment #0)
> It would be handy for developers to modify the preferences without restart
> b2g.

What kind of preference would you like to change for example? And wouldn't it be more useful to have access to the settings instead?

Comment 2

[Shian-Yow Wu [:swu]]

Some modules use preferences rather than settings.

Take WebRTC for example, there are a couple of preferences in:
https://dxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#231

On Firefox OS, there seems no way to modify preferences with restarting b2g process, and this makes debugging/testing complicated.

Comment 3

[Shian-Yow Wu [:swu]]

(In reply to Shian-Yow Wu [:shianyow] from comment #2)
> On Firefox OS, there seems no way to modify preferences with restarting b2g
> process, and this makes debugging/testing complicated.

s/with/without/

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Paul Rouget [:paul] from comment #1)
> (In reply to Kan-Ru Chen [:kanru] from comment #0)
> > It would be handy for developers to modify the preferences without restart
> > b2g.
> 
> What kind of preference would you like to change for example? And wouldn't
> it be more useful to have access to the settings instead?

All the tunnable preferences we currently have in desktop about:config. I could image developers use this to experiment with pref off features or other preferences.  Having access to the settings is great too, we should have another bug for it ;)

Comment 5

[Paul Rouget [:paul]]

For security reasons, we could only offer this if the pref devtools.debugger.forbid-certified-apps is false.

Comment 6

[Alexandre Poirot [:ochameau]]

Bug 947897 is going to implement the necessary backend on device.
The only part left will be the UI in the app manager.

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

The patch in bug 947897 doesn't provide interface like |getAllPrefs| if we want to implement something like about:config in app-manager. We probably need to implement it in this bug.

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] Part 1 - support-get-all-prefs.patch

Load all preferences from device.

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] Part 2 - about-config-tab-in-app-manager.patch

Display the preference table in "Preferences" tab.

TODO:
1. preference editing
2. automatically reload the changed preference or provide a manual reload button
3. backward capability to 1.2/1.3 devices.

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Screen Shot of current status

Comment 11

[Paul Rouget [:paul]]

Hi Shih. Thanks for looking at this. This looks great so far!

Comment 12

[Paul Rouget [:paul]]

Also, please don't allow pref reading/editing if the pref "devtools.debugger.forbid-certified-apps" is true.

Comment 13

[Paul Rouget [:paul]]

Shih-Chiang, anything I can do to help you?

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Paul Rouget [:paul] from comment #13)
> Shih-Chiang, anything I can do to help you?
I was occupied by other activities last week. I have a patch for editing preference via window.prompt() but need to polish before uploading it. However, I did ran into a no response warning from _unregisterNodes in template.js [1] while disconnecting device. My guess is that the warning is caused by too many instance generated in preference tab. @paul, have you ever seen this warning before?

[1] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/app-manager/content/template.js#176

Comment 15

[Paul Rouget [:paul]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #14)
> (In reply to Paul Rouget [:paul] from comment #13)
> > Shih-Chiang, anything I can do to help you?
> I was occupied by other activities last week. I have a patch for editing
> preference via window.prompt() but need to polish before uploading it.
> However, I did ran into a no response warning from _unregisterNodes in
> template.js [1] while disconnecting device. My guess is that the warning is
> caused by too many instance generated in preference tab. @paul, have you
> ever seen this warning before?

Nope. Sounds bad though. Does it happen all the time? I can look at this.

Comment 16

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Yeah, all the some on my nightly debug build, haven't tried the release build though.

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Part 1 - support-get-all-prefs-v2.patch

This folds in the patch from bug 947897 and addresses all the outstanding action items/requests from that bug plus a little cleanup.

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Part 2 - about-config-tab-in-app-manager-v2.patch

So, first, apologies about not checking about the state of :schien's patches more; it sounds like there are some local changes that weren't reflected on the bug.  I really wanted to try out this feature and got sucked into doing some cleanup and enhancements.

With both patches applied the net result is:
- You can see all the preferences from the device as long as devtools.debugger.forbid-certified-apps is false.
- You can edit preferences by double-clicking on them or right-clicking on them and choosing "Modify Preference"
- You can clear preferences by right-clicking on them and choosing "Clear Preference"
- You can create new preferences by right-clicking.
- There is incremental filtering support.

about:config parity features that aren't happening right now:
- Showing user-modified preferences in bold.  The template system only seems to want to let you do one thing per node, and the incremental search mechanism is already doing something with the node we'd want the attribute/class/whatever on.

Ugliness:
- Initial display/layout is not great.  The layout seems to shift around a bit, so I think flex-box may be one of the major responsible parties.  template.js could also be involved, but I think it's okay.  The main thing is that it would probably be smart to defer building the preferences DOM until displayed.  Probably also should only fetch preferences on demand.
- (Using a virtual list widget or something smart like that would probably go a bit better.)
- There are performance problems when adding a new preference.  I think that's because of template.js and its path mechanisms, I guess this because...
- I implemented and ended up disabling an option to entirely refresh the preferences.  The slow script dialog literally comes up about 4 times in a row for me on my beefy machine.  I think this is due to the observable object/template.js stuff.
- There are some cop-outs about mutation relating to template.js performance fears.
- I'm not sure I totally understand how the device.js UI is supposed to interact with the device-store.js.  For the refreshPreferences() logic I tried to make them talk via events because that seemed like the only exposure, but generic events don't get relayed, so I just saved off the deviceStore in the first place.  I'm also not sure if the DeviceStore wants to be more in control or what.
- Along those lines, there's a cop-out on clearUserPref about this where I now realize I could just issue the "front" call to check on the pref and poke the result in depending on what happened.

In general, this seems at the 80% point now.  It's totally usable, but fixing that last 20%, especially if it involves tests, seems like it will be a doozy.  I'm able to put a little more time into this, but if someone else/:schien would like to finish it out, I'd be appreciative!

Comment 19

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Thanks, Andrew. You patch looks a lot better than I have in local. I don't think I am capable of finishing out the last 20%, however, I would like to help submitting the initial patch for the test cases.

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8383603 [details] [diff] [review]
Part 2 - about-config-tab-in-app-manager-v2.patch

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

::: browser/themes/shared/devtools/app-manager/device.css
@@ +341,5 @@
> +.preference-type,
> +.preference-value {
> +  border-right: 1px solid #CCC;
> +  border-bottom: 1px solid #CCC;
> +  flex-grow: 1;

I did some experiments. By adding "overflow: auto; text-overflow: ellipsis; white-space: nowrap;" will force display each preference in single line.

Comment 21

[Paul Rouget [:paul]]

(In reply to Andrew Sutherland (:asuth) from comment #18)
> With both patches applied the net result is:
> - You can see all the preferences from the device as long as
> devtools.debugger.forbid-certified-apps is false.
> - You can edit preferences by double-clicking on them or right-clicking on
> them and choosing "Modify Preference"
> - You can clear preferences by right-clicking on them and choosing "Clear
> Preference"
> - You can create new preferences by right-clicking.
> - There is incremental filtering support.

Excellent!


> - I implemented and ended up disabling an option to entirely refresh the
> preferences.  The slow script dialog literally comes up about 4 times in a
> row for me on my beefy machine.  I think this is due to the observable
> object/template.js stuff.

Alright, this is what I suggest: we are rebooting the UI of the app manager. It should land in ~2 months. This new UI will get rid of template.js. If, on your side, this feature can wait until the new UI, let's re-visit this bug once this is done. If this needs to land earlier, let's rebuild this patch with a xul treeview. And I can take care of the last 20%.

Comment 22

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Paul Rouget [:paul] from comment #21)
> Alright, this is what I suggest: we are rebooting the UI of the app manager.
> It should land in ~2 months. This new UI will get rid of template.js. If, on
> your side, this feature can wait until the new UI, let's re-visit this bug
> once this is done. If this needs to land earlier, let's rebuild this patch
> with a xul treeview. And I can take care of the last 20%.

2 months is a while, and slippage being what it is, I'd be inclined for us to maybe try and land something usable sooner.  This is pretty handy compared to the alternatives to tweak preferences for Gaia developers.  I can probably personally survive with just the patch, of course! ;)

Looking at the existing about:config UI implemented by:
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/viewconfig/content/config.xul
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/viewconfig/content/config.js

It looks like the existing implementation can be used with only a small set of modifications to use a remote data-model and turn off/alter some live-tracking stuff.  Things could probably be reused with either very ugly monkeypatching, forkage, or a shared refactoring.  Depending on the plan for the UI refactoring in 2-months, perhaps a short-lived fork of the existing implementation would be okay?  If the re-write is not going to involve XUL, refactoring the existing about:config and risking breaking it is probably not a big win.

There is also the android implementation at http://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content but it's a bit biased towards a phone interaction paradigm.  It does lazy infinite scrolling DOM addition stuff.

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

Note that I am expecting all Gaia developers to be using (trunk) nightlies, so trains don't actually matter.

Comment 24

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

Comment on attachment 8383603 [details] [diff] [review]
Part 2 - about-config-tab-in-app-manager-v2.patch

I'll take a look over this in Paul's absence and recommend a way to proceed soon.

Comment 25

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

Comment on attachment 8383603 [details] [diff] [review]
Part 2 - about-config-tab-in-app-manager-v2.patch

I actually like this current approach quite a lot!

I spent some time profiling template.js to see what it's up to, and I found quite a few ways to speed it up, so it's much nicer now.

It seems to fine to continue down this road.  If we truly do trash all uses of template.js later on, we can convert it to something else at that point.  I'll provide more thoughts on this on Monday.

For now, I think our top priority should be aiming to land at least the actor side in 1.4.  I'll plan to review that on Monday as well.

Comment 26

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - testcase for DevTools server

Add test case for patch part 1.

Comment 27

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

Comment on attachment 8383601 [details] [diff] [review]
Part 1 - support-get-all-prefs-v2.patch

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

So, the "device" actor is currently this weird catch-all box that doesn't have great definition around what it should include.  So far it's basically "random info we want to show in the App Manager".  Other actors are more organized: there's a remote object (like a DOM node) and some methods that make sense for it.

Since prefs seem like their own thing, and they have a clearly defined set of related methods, I think it would be best to break this stuff out into its own "prefs" actor.  It also helps be a bit more strict about whether the "prefs" actor is even loaded at all on certain platforms (desktop, Fennec, etc.) should we decide that different policies are needed.

schien / asuth: Would one of you be able to do this refactoring soon?  Please let me know if you won't, so I can plan some other way to get this in for 1.4.

::: toolkit/devtools/server/actors/device.js
@@ +206,5 @@
> +  }),
> +
> +  getBoolPref: method(function(name) {
> +    if (!this._arePrefsAccessible()) {
> +      return null;

In this case, it would be good throw an Error with message about problem.  That way, the message can be seen over the remote protocol for debugging.  Same for the other places this is checked here.

@@ +210,5 @@
> +      return null;
> +    }
> +    return Services.prefs.getBoolPref(name);
> +  }, {
> +    request: { value: Arg(0)},

Nit: space before the closing brace

@@ +220,5 @@
> +      return null;
> +    }
> +    return Services.prefs.getCharPref(name);
> +  }, {
> +    request: { value: Arg(0)},

Nit: space before the closing brace

@@ +230,5 @@
> +      return null;
> +    }
> +    return Services.prefs.getIntPref(name);
> +  }, {
> +    request: { value: Arg(0)},

Nit: space before the closing brace

Comment 28

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

Comment on attachment 8388433 [details] [diff] [review]
Part 3 - testcase for DevTools server

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

Might be good to mention "prefs" in the commit message.

Also, this would probably need to be a separate file after breaking out "prefs" as a separate actor in part 1.

It would be good to test setting prefs via the actor and getting back their values to make this more complete.

::: toolkit/devtools/server/tests/mochitest/test_device.html
@@ +71,5 @@
> +        is(prefs.boolPref, EXPECTED_BOOL_PREF, "read bool pref");
> +        is(prefs.intPref, EXPECTED_INT_PREF, "read int pref");
> +        is(prefs.charPref, EXPECTED_STRING_PREF, "read string pref");
> +
> +        for (var key in prefs.allPrefs) {

Looping over every value and testing it seems a bit excessive...  Is the test slow to run at all?

Comment 29

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - support-get-all-prefs.patch, v3

Create a separate actor for preference.

NOTE: the test case runs less than 2s on my macbook pro, so I think it is affordable to check the entire preference table.

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

Comment 30

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

Comment on attachment 8389087 [details] [diff] [review]
Part 1 - support-get-all-prefs.patch, v3

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

Wow, fast turnaround! :D Thanks so much!

Overall, it looks great.  However, my comments from the last part 1 still remain:

* Throw an error if prefs are not accessible, instead of just returning null
* Fix the closing brace spacing nits

r+ with these changes made.

I'm not sure how fast I'll land my template.js improvements to support the UI here, but I'll do my best.  I'll mark this as leave-open, so we can at least land part 1 now, and possibly do the UI in a later bug if I am too slow.

Comment 31

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - support-get-all-prefs.patch, v4

update according to review comments. I'm not 100% sure about the convention for throwing exception in actor, so I raise the r? again.

Comment 32

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

Comment on attachment 8389574 [details] [diff] [review]
Part 1 - support-get-all-prefs.patch, v4

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

Looks good.  Just a missing comma to fix.  Probably good to do a try run as well.

::: b2g/chrome/content/shell.js
@@ +1145,5 @@
>            // Use an explicit global actor list to prevent exposing
>            // unexpected actors
>            globalActorFactories: restrictPrivileges ? {
>              webappsActor: DebuggerServer.globalActorFactories.webappsActor,
>              deviceActor: DebuggerServer.globalActorFactories.deviceActor

Just tested this on device, and there's a missing comma here, so b2g fails to startup.

::: toolkit/devtools/server/actors/preference.js
@@ +34,5 @@
> +  }),
> +
> +  getBoolPref: method(function(name) {
> +    if (!this._arePrefsAccessible()) {
> +      throw new Error("Operation not permitted");

Yes, that seems fine to me.

Comment 33

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

Since my understanding is that both schien and asuth don't have time to finish out the last parts of the UI, I'm going to move that over to it's own bug, and we'll refocus this to just be about the actor.

Comment 34

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: support-get-all-prefs.patch

update according to review comment, carry r+.

try result: https://tbpl.mozilla.org/?tree=Try&rev=47c8e97ac4f8

Comment 35

[:Paolo Amadini]

I just wanted to drop a quick note about the fact that we're converting the debugger server code to use Promise.jsm in bug 943517, meaning that in the future actors should import Promise.jsm instead of the legacy SDK module. This is part of a project to eventually convert to ES6 Promises when they're ready (https://wiki.mozilla.org/Snappy/Promises_unification).

Since it's likely that the present work on the preferences actor will land first, I'll update my patch accordingly before it lands, to include this actor in the conversion.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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

Comment 37

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

https://hg.mozilla.org/mozilla-central/rev/762cdef7ecb4