Closed Bug 943251 Opened 11 years ago Closed 10 years ago

Server support for preferences

Categories

(DevTools Graveyard :: WebIDE, defect, P2)

Product:
DevTools Graveyard
Component:
WebIDE
Platform:
x86_64
Linux
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 30

People

(Reporter: kanru, Assigned: schien)

References

Details

Attachments

(2 files, 7 obsolete files)

Screen Shot of current status
1.12 MB, image/png
Details
support-get-all-prefs.patch
11.73 KB, patch
schien
: review+
Details | Diff | Splinter Review 
It would be handy for developers to modify the preferences without restart b2g.
(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?
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.
(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/
(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 ;)
For security reasons, we could only offer this if the pref devtools.debugger.forbid-certified-apps is false.
Priority: -- → P2
Bug 947897 is going to implement the necessary backend on device.
The only part left will be the UI in the app manager.
Depends on: 947897
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.
Load all preferences from device.
Assignee: nobody → schien
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.
Hi Shih. Thanks for looking at this. This looks great so far!
Also, please don't allow pref reading/editing if the pref "devtools.debugger.forbid-certified-apps" is true.
Shih-Chiang, anything I can do to help you?
(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
(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.
Yeah, all the some on my nightly debug build, haven't tried the release build though.
This folds in the patch from bug 947897 and addresses all the outstanding action items/requests from that bug plus a little cleanup.
Attachment #8372201 - Attachment is obsolete: true
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!
Attachment #8372203 - Attachment is obsolete: true
Attachment #8383603 - Flags: feedback?(paul)
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 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.
(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%.
(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.
Note that I am expecting all Gaia developers to be using (trunk) nightlies, so trains don't actually matter.
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.
Attachment #8383603 - Flags: feedback?(paul) → feedback?(jryans)
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.
Attachment #8383603 - Flags: feedback?(jryans) → feedback+
Add test case for patch part 1.
Attachment #8388433 - Flags: feedback?(jryans)
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
Attachment #8383601 - Flags: feedback+
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?
Attachment #8388433 - Flags: feedback?(jryans) → feedback+
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
Attachment #8383601 - Attachment is obsolete: true
Attachment #8388433 - Attachment is obsolete: true
Attachment #8389087 - Flags: review?(jryans)
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.
Attachment #8389087 - Flags: review?(jryans) → review+
update according to review comments. I'm not 100% sure about the convention for throwing exception in actor, so I raise the r? again.
Attachment #8389087 - Attachment is obsolete: true
Attachment #8389574 - Flags: review?(jryans)
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.
Attachment #8389574 - Flags: review?(jryans) → review+
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.
No longer depends on: 947897, 982322
Keywords: leave-open
Summary: Allow accessing about:config from app-manager → Server support for preferences
update according to review comment, carry r+.

try result: https://tbpl.mozilla.org/?tree=Try&rev=47c8e97ac4f8
Attachment #8389574 - Attachment is obsolete: true
Attachment #8389724 - Flags: review+
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.
https://hg.mozilla.org/integration/fx-team/rev/762cdef7ecb4
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/762cdef7ecb4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Depends on: 983951
QA Whiteboard: [qa-]
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: