982484 - Manage device preferences

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect, P2)

Description

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

Add a UI to view and edit device preferences (similar to about:config).

Comment 1

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

Attachment: WIP UI

This is same UI patch that was attached to bug 943251, but tweaked to work with new actor.

In bug 943251 comment 18, asuth summarized the current state:

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 2

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

I am still interested in trying to get this landed, if only for my own laziness reasons for setting preferences.  (Although with the server stuff landed I can also just write a command-line JS thing.)  This will obviously not happen for the current/imminent train, but I'm okay with that.

:jryans, can you attach your patch for your template.js fixes to bug 982322 or provide a branch link or what not so I can see how this runs on top of those?

Comment 3

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

Okay, I've attached each tweak to bug 982322.  I'll ask for reviews and explain them a bit more on Monday.

Also, I'll attach a few tweaks here that I made on the UI side as well.

Comment 4

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

Attachment: Tweak 1: Fix root path

The root path drops the last "." after my patches in bug 982322.

Comment 5

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

Attachment: Tweak 2: Simple sort

Using a simple (non-locale aware) sort is faster here.

Comment 6

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

Comment on attachment 8389625 [details] [diff] [review]
WIP UI

Flagging myself for feedback to provide more thoughts on finishing this up.

Feel free to try it out with the other patches in the mean time. :)

Comment 7

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

Comment on attachment 8389625 [details] [diff] [review]
WIP UI

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

I've tried to reply to any obvious questions that I found in the patch, but let me know if there's more you're unsure of!  As I said, in general I think it works quite well.

::: browser/devtools/app-manager/content/device.js
@@ +88,5 @@
>      this.connection.on(Connection.Events.STATUS_CHANGED, this._onConnectionStatusChange);
>  
> +    // hold onto our deviceStore so we can emit and receive events other than
> +    // "set" which is the only thing mergeStores supports via special-case.
> +    this.deviceStore = new DeviceStore(this.connection);

Saving this off here seems fine to me.  I think it's just by chance that this use case seems to be the first where a store needs to be refreshed based on something other that a connection event.

@@ +112,5 @@
>    _onConnectionStatusChange: function() {
>      if (this.connection.status != Connection.Status.CONNECTED) {
>        this.hide();
>        this.listTabsResponse = null;
> +      this.front = null;

I added the "prefs" part quickly...  We should probably rename this to "device" now.

@@ +198,5 @@
> +        // figure out the index of the preference in our array so that we
> +        // can retrieve the pref and twiddle values directly.  The wrapper
> +        // should be sticky and have the changes reflect in the UI immediately.
> +        let prefPath = JSON.parse(elem.getAttribute("template")).rootPath;
> +        let index = parseInt(prefPath.split('.').slice(-2)[0]);

We could probably support this better in template.js somehow...  Though I'm not sure the best approach.  This is probably fine for now, even it's a bit yucky.

@@ +270,5 @@
> +    this.prefs.clearUserPref(pref.name);
> +    // The right thing to do here would be to check what the new value (if
> +    // any) of the preference is after clearing and then update our value if
> +    // it's still around.  Unfortunately, I'm not sure the communication idiom
> +    // for interacting with the store through this.

Maybe the store could offer a |refreshPref(name)| method, that looks up that one pref and updates it in the store?  That would the trigger the UI to update.

@@ +356,5 @@
> +
> +  refreshPreferences: function() {
> +    this.deviceStore.once("refreshedPreferences",
> +                          () => { this.updatePreferenceFilter(); });
> +    this.deviceStore.emit("refreshPreferences");

DeviceStore should have a public method that you call to refresh the prefs.  It's a bit unusual for a object A to force object B to emit an event like this.

::: browser/devtools/app-manager/content/device.xhtml
@@ +116,5 @@
>  
> +  <template id="preference-template">
> +  <!-- XXX somehow express hasUserValue as an attribute on the root div here;
> +       unfortunately the template mechanism does not support multiple actions
> +       per node. -->

Yes, I encountered this in a different problem a while back...  I believe I have patch to add it somewhere...  Let me know if you think you'd like it here.

::: browser/locales/en-US/chrome/browser/devtools/app-manager.dtd
@@ +108,5 @@
> +<!ENTITY preferences.menu.new.label "New Preference">
> +<!ENTITY preferences.menu.new.string.label "String">
> +<!ENTITY preferences.menu.new.integer.label "Integer">
> +<!ENTITY preferences.menu.new.true.label "True Boolean">
> +<!ENTITY preferences.menu.new.false.label "False Boolean">

These two can probably just say "True" and "False", but I don't feel strongly about it.

::: browser/themes/shared/devtools/app-manager/device.css
@@ +294,5 @@
> +.preference-table-header-item {
> +  z-index: 2;
> +  flex-grow: 1;
> +  background: linear-gradient(to bottom, #49535C, #394148);
> +  box-shadow: 0px 1px 3px rgba(12, 20, 30, 0.5), inset 0 1px 0px rgba(255,255,255,0.1);

Drop the "px" for 0 values.

Comment 8

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

We'll also need to be sure we test for existence of the prefs actor before assuming it's there in the UI, since it only exists on 1.4+.

Comment 9

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

Thanks for posting the (great!) optimizations on the other bug.  You are much more disciplined with your patch management than I!  I'll update the patch tonight based on your feedback here and the prior discussion on the progenitor bug.

Comment 10

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

Unfortunately some high-priority stuff came up last week and I'm riding the critical path right now, so realistically it would be another few days before I can get to this.  Apologies for not getting a dose of realism much sooner.  I'll still come back to this if no one else picks it up in the interim.  It's fantastic that the template.js perf improvements have landed!

Comment 11

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

Attachment: Manage devices prefs in App Manager. r=paul

Comment 12

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

Comment on attachment 8400929 [details] [diff] [review]
Manage devices prefs in App Manager. r=paul

Paul, this version is fully functional and is updated to cover my own previous review comments...

However, I haven't yet added any tests, as I was worried you'd say "wait until UI v2". :P I think there's benefit to offering this feature now, but we'll see what you think.

Comment 13

[Paul Rouget [:paul]]

Comment on attachment 8400929 [details] [diff] [review]
Manage devices prefs in App Manager. r=paul

At this point, we should build that in the new app manager.

Comment 14

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

Jen, this is bug for the preference UI.

This has been around for a while, as Andrew and I built an initial version for the old App Manager (pre-WebIDE) that never landed.

At this point, it basically needs to be rewritten to work with WebIDE anyway.  Some approaches you could take:

* Refactor and reuse the about:config UI from Firefox desktop
* Refactor and reuse the about:config UI from Firefox for Android (they are very different)
* Build your own

As we discussed, it would be nice to leverage the same UI to display settings later on too.  So, it might be that building your own makes that easier.

Comment 15

[Jen Fong-Adwent [:ednapiranha]]

Attachment: bug982484.patch

Comment 16

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

Comment on attachment 8509803 [details] [diff] [review]
bug982484.patch

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

Cool, it's coming along nicely! :)

General thoughts:

* We'll should request an icon from UX that means "prefs"
  * It looks like Firefox already uses a "gear" for such purposes
  * Maybe use can reuse the toolbox options icon[1] (would suggest copying the image if so)
* Search field should have label / icon / placeholder text / something to make it's purpose clear
* Search should match any sub-string of the key, not just the beginning
  * Ex: typing "debugger" should give results, like it does on about:config
* Might be nice to keep the search box fixed on screen as you scroll (not required)
* You should add an entry to the "Runtime" menu in the menu bar as well 
  * Probably these menu bar menus should be reworked, but for now let's add this one like the others

[1]: http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/toolbars.inc.css#219

::: browser/devtools/webide/content/devicepreferences.js
@@ +6,5 @@
> +const {Services} = Cu.import("resource://gre/modules/Services.jsm");
> +const {require} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
> +const {AppManager} = require("devtools/webide/app-manager");
> +const {Connection} = require("devtools/client/connection-manager");
> +const {Devices} = Cu.import("resource://gre/modules/devtools/Devices.jsm");

This is not used.

@@ +7,5 @@
> +const {require} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
> +const {AppManager} = require("devtools/webide/app-manager");
> +const {Connection} = require("devtools/client/connection-manager");
> +const {Devices} = Cu.import("resource://gre/modules/devtools/Devices.jsm");
> +const {USBRuntime} = require("devtools/webide/runtimes");

Or this.

@@ +66,5 @@
> +  }
> +  return input;
> +};
> +
> +function SaveByType(name, inputType, value, reset) {

Try to reduce duplicated code here where reasonable.  For example, the "hide" class toggling could be done after the switch block it seems.

Try using an object for the params here, since you have several boolean params.  You can destructure the object in the params if you want, so it shouldn't make the method body change much at all.  Or if you don't like that, add comments at the call sites to label the boolean arguments.

@@ +76,5 @@
> +      let setCheckbox = function(checkType) {
> +        input.checked = checkType;
> +      };
> +
> +      if (reset) {

It may be simpler for the "reset" path to be a separate function entirely, since it looks like there is not much common code.

@@ +77,5 @@
> +        input.checked = checkType;
> +      };
> +
> +      if (reset) {
> +        let defaultPref = AppManager.preferenceFront.getBoolPref(name);

When resetting a pref with no default, there's currently an exception (as you noticed), since there is now no value to get.

I do not believe we have a way to ask "does this pref have a default" today, so you may need to just catch the exception in this case and hide the row if that happens.  Or, we could extend the preference actor to support asking this kind of question, but then there would be some devices with this new support and some without... may be simpler to just catch it.

@@ +121,5 @@
> +      break;
> +  }
> +};
> +
> +function UpdateNewPref() {

Typing in the new pref name field saves a pref called "custom-value-name" (which is the input element's ID).  Not sure why that is happening, but we don't want to do that.

@@ +128,5 @@
> +  let customTextNameEl = document.getElementById("custom-value-name");
> +  let customText = customTextEl.value;
> +  let customTextName = customTextNameEl.value.replace(/[^A-Za-z0-9\.\-_]/gi, "");
> +  if (customText) {
> +    SaveByType(customTextName, "string", customText);

This seems to only allow create new string prefs, but we'll need to be able to make new ones of all the types.

You could try to parse the entered text to detect the appropriate type (hopefully not too many people need a char pref with the text "true"...), or else have some UI control to state the type to use.

@@ +184,5 @@
> +  tr.appendChild(td);
> +  td = document.createElement("td");
> +  td.setAttribute("id", "td-" + name);
> +
> +  if (!prepend) {

Why does this mean "build reset button"?  At least a comment or something would help.

::: browser/devtools/webide/content/webide.js
@@ +747,5 @@
>      document.querySelector("#cmd_importPackagedApp").removeAttribute("disabled");
>      document.querySelector("#cmd_importHostedApp").removeAttribute("disabled");
>      document.querySelector("#cmd_showProjectPanel").removeAttribute("disabled");
>      document.querySelector("#cmd_showRuntimePanel").removeAttribute("disabled");
> +    document.querySelector("#cmd_showDevicePrefs").removeAttribute("disabled");

I don't think this is needed here.  It should become enabled via your later changes below.

@@ +812,5 @@
>        if (AppManager.deviceFront) {
>          detailsCmd.removeAttribute("disabled");
>          permissionsCmd.removeAttribute("disabled");
>          screenshotCmd.removeAttribute("disabled");
> +        devicePrefsCmd.removeAttribute("disabled");

You want to test for "preferenceFront" instead of "deviceFront" when activating this button.

::: browser/devtools/webide/test/head.js
@@ +175,5 @@
>    targetBrowser.removeTab(aTab);
>    return deferred.promise;
>  }
>  
> +function loadSimulator(aWindow) {

Call this something like "connectToLocalRuntime" or similar.

It's not actually connecting to a simulator at all, but instead to the local Firefox instance, which happens to conveniently offer the APIs we want for testing.

::: browser/devtools/webide/test/test_deviceinfo.html
@@ +2,5 @@
>  
>  <html>
>  
>    <head>
> +

Undo this.

::: browser/devtools/webide/themes/deck.css
@@ +57,5 @@
>  th:first-of-type, td:first-of-type {
>    text-align: left;
>  }
> +
> +#device-preferences {

These things should go in your specific devicepreferences.css file.

Maybe you put it here because the devicepreferences.css fails to load at the moment?  You need to add an entry to browser/devtools/webide/themes/jar.mn for the CSS file.

::: browser/locales/en-US/chrome/browser/devtools/webide.properties
@@ +72,5 @@
>  status_error=ERRORS
>  status_unknown=UNKNOWN
> +
> +# Preferences
> +reset_default=Reset to default

Make this ID specific to the page by prefixing with "devicepreferences_"

Comment 17

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

Comment on attachment 8509803 [details] [diff] [review]
bug982484.patch

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

Would be nice to have a test case for filtering too.

Comment 18

[Jen Fong-Adwent [:ednapiranha]]

Attachment: bug982484.patch

Comment 19

[Jen Fong-Adwent [:ednapiranha]]

Darrin, do you have a good icon for 'Device Preferences'?

Comment 20

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

Comment on attachment 8511386 [details] [diff] [review]
bug982484.patch

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

This patch no longer applies on latest fx-team, can you rebase it?

Comment 21

[Jen Fong-Adwent [:ednapiranha]]

Attachment: bug982484.patch

Comment 22

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

Comment on attachment 8512195 [details] [diff] [review]
bug982484.patch

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

* Toggling the type for a new pref makes the columns jump around, since different size input fields are used for the value of each type
  * I'd suggest either using a fixed size for all of the new pref value inputs, or moving the "Add new" button to the third column
* Changing the new pref type should not preserve the existing value in the new pref name input
* (Optional) It would be nice if pressing "return" in the value input also added a new pref (so you don't have to use the button)

Overall, looking much nicer! :)

::: browser/devtools/webide/content/devicepreferences.js
@@ +57,5 @@
> +
> +function ResetToDefault(name, input, button) {
> +  let clearPref = AppManager.preferenceFront.clearUserPref(name);
> +  let defaultPref;
> +  let dataType = input.getAttribute("data-type");

You can also access this via |input.dataset.type| if you prefer.  Either way seems fine though.

@@ +68,5 @@
> +      } else {
> +        input.value = value;
> +      }
> +
> +      SaveByType({

Hmm, I am still confused by this...

Reset should call *only* |clearUserPref|, and then either remove row (if custom) or re-read the default value (if default exists).

@@ +83,5 @@
> +      defaultPref = AppManager.preferenceFront.getBoolPref(name);
> +      defaultPref.then(boolean => {
> +        input.checked = boolean;
> +      }, err => {
> +        resetPreference(false);

Why is |resetPreference| called only for errors?

The steps I would expect in the method are:

1. clearUserPref(pref)
2. get<TYPE>Pref
  a. If success, show pref with default value
  b. If error, remove pref row

It looks we are doing 2 then 1 at the moment.  No saving should take place at any time when resetting.

I did some more testing with |get<TYPE>Pref| for prefs that don't exist (which includes custom prefs after reset / clear), and I was able to catch the error in the promise rejection:

preferenceFront.getCharPref("nonexistant.pref").then(() => {
  // worked
}, e => {
  // failed
  console.log(e); // This error shows up in the Browser Console
})

I guess you got it working though, since I see you are using a rejection handler below.

@@ +114,5 @@
> +function SaveByType(options) {
> +  let name = options.id;
> +  let inputType = options.type;
> +  let value = options.value;
> +  let reset = options.resetDefault || false;

Unused?

@@ +116,5 @@
> +  let inputType = options.type;
> +  let value = options.value;
> +  let reset = options.resetDefault || false;
> +  let input = document.getElementById(name);
> +  let button = document.getElementById("btn-" + name);

Unused?

@@ +133,5 @@
> +      break;
> +  }
> +};
> +
> +function UpdateNewPref(event) {

I started to see many oddities when testing different new pref types and then checking the result in about:config.

Please write more test and check the interactions with different combos.

@@ +141,5 @@
> +  }
> +  let customRow = table.querySelector("tr:nth-of-type(2)");
> +  let customTextEl = table.querySelector("#custom-value-text");
> +  let customTextNameEl = table.querySelector("#custom-value-name");
> +  let customText = customTextEl.checked || customTextEl.value;

If you have ever added a boolean pref and then you add an integer or string, |checked| is still true and the real value is lost.

@@ +191,5 @@
> +            value: inputValue
> +          });
> +          document.getElementById("btn-" + id).classList.remove("hide");
> +        } else {
> +          ResetToDefault(id, event.target, document.getElementById("btn-" + id));

I got this error on load:

JavaScript error: chrome://webide/content/devicepreferences.js, line 195: ReferenceError: id is not defined

Also, I am unsure what this branch means...  So we reset the pref value if the current value is not valid?

@@ +234,5 @@
> +  td = document.createElement("td");
> +  td.setAttribute("id", "td-" + name);
> +
> +  // If this a change to an existing preference add a reset default button.
> +  if (!newPreferenceRow) {

Is this done for now, since you were getting errors when resetting the "custom" prefs, or some other purpose?

At the moment, it's a bit arbitrary, since the button does eventually show up if I restart Firefox and re-list prefs (since this bit about whether it is "new" is now lost).

@@ +279,5 @@
> +  }
> +}
> +
> +let getAllPrefsPromise; // Used by tests
> +function BuildUI() {

Something needs to remove existing rows first.  Currently if the connection changes (different device, etc.), we just keep appending more and more prefs at the bottom, leaving stale data at the top.

::: browser/devtools/webide/test/head.js
@@ +171,5 @@
>    return deferred.promise;
>  }
>  
> +function connectToLocalRuntime(aWindow) {
> +  info("Loading simulator.");

Nit: Fix this log message

::: browser/devtools/webide/test/test_device_preferences.html
@@ +66,5 @@
> +            }
> +          }
> +          ok(found, "Found accessibility preference line");
> +
> +          // Add a new custom preference

Add tests for a new pref of each type

::: browser/devtools/webide/test/test_deviceinfo.html
@@ +33,5 @@
>  
>            yield documentIsLoaded(permIframe.contentWindow.document);
>            yield documentIsLoaded(infoIframe.contentWindow.document);
>  
> +          win.AppManager.update("runtimelist");

You may need to revert this line if this test starts failing.

::: browser/devtools/webide/themes/devicepreferences.css
@@ +38,5 @@
> +#device-preferences {
> +  margin-top: 110px;
> +}
> +
> +#device-preferences tr:nth-child(even) {

I really like this zebra striping, it's quite helpful!

However, it breaks down right now if there is:

1. VISIBLE
2. HIDDEN
3. VISBILE

Currently 1 & 3 are both gray, which is not desired.

::: browser/locales/en-US/chrome/browser/devtools/webide.dtd
@@ +37,5 @@
>  <!ENTITY runtimeMenu_showDetails_accesskey "E">
>  <!ENTITY runtimeMenu_showMonitor_label "Monitor">
>  <!ENTITY runtimeMenu_showMonitor_accesskey "M">
> +<!ENTITY runtimeMenu_showDevicePrefs_label "Device Preferences">
> +<!ENTITY runtimeMenu_showDevicePrefs_accesskey "d">

I believe these need to match the case of the letter they are for, so "D" here.

Harder to check these since they seem to do nothing on OS X...

Comment 23

[Jen Fong-Adwent [:ednapiranha]]

Attachment: bug982484.patch

Comment 24

[Jen Fong-Adwent [:ednapiranha]]

Attachment: bug982484.patch

Comment 25

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

Maybe Shorlander can help with an icon.  Stephen, see comment 19.

Comment 26

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

Comment on attachment 8513059 [details] [diff] [review]
bug982484.patch

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

A few visual issues left, but next round is probably ready for review.  Though, we do still need an icon from somewhere.  Hopefully one will appear!

* The new pref value input no longer jumps around when the type changes (yay!), but columns do still jump around when you search
  * This seems to be caused by the longest visible pref name at any given time
  * One idea is to force the columns to take a fixed % of the width so they don't jump
  * Make sure that long pref names then wrap if they are too long
* The location of the "add new" button still seems odd, since there's a different column with "reset" buttons
  * Can we align all the buttons in the same column by moving the "add new" to the 3rd column?
* Add new button seems to use a different font size than reset
  * If that's intentional, than okay, just looks a bit odd to me
* Oops, I said this the wrong way last time: changing the new pref type *should* preserve the existing value in the new pref name input
  * In other words, if I typed a new pref name first, and then change the type, it would be nice if the pref name would be left alone

Some of the JS issues I noticed using JSHint.  The wiki[1] has a .jshintrc you can drop into your local checkout.

[1]: https://wiki.mozilla.org/DevTools/Hacking#Checking_your_code_using_JSHint

::: browser/devtools/webide/content/devicepreferences.js
@@ +53,5 @@
> +      input.setAttribute("type", "text");
> +      break;
> +  }
> +  return input;
> +};

Super Nit: Don't really need this semicolon

@@ +56,5 @@
> +  return input;
> +};
> +
> +function ResetToDefault(name, input, button) {
> +  let clearPref = AppManager.preferenceFront.clearUserPref(name);

Normally I would say we wait for this promise to resolve before proceeding...

But the DevTools protocol will ensure that this is finished before the next message to getXPref, so I suppose it's not required.

Anyway, if you don't want to wait for the promise, you should remove the unused "clearPref" variable.

@@ +66,5 @@
> +    case "boolean":
> +      defaultPref = AppManager.preferenceFront.getBoolPref(name);
> +      defaultPref.then(boolean => {
> +        input.checked = boolean;
> +      }, err => {

Use () since you don't need the |err| value.

Same for the other two below.

@@ +81,5 @@
> +      break;
> +    default:
> +      defaultPref = AppManager.preferenceFront.getCharPref(name);
> +      defaultPref.then(string => {
> +        if (string === "false") {

What's the purpose of this?

@@ +103,5 @@
> +
> +  switch (inputType) {
> +    case "boolean":
> +      AppManager.preferenceFront.setBoolPref(name, input.checked);
> +      input.type = "checkbox";

Isn't the type and checked state already set by this point?

@@ +117,5 @@
> +}
> +
> +function CheckNewPrefSubmit(event) {
> +  if (event.keyCode === 13) {
> +    document.getElementById('custom-value').click();

Nit: Use double quotes

@@ +135,5 @@
> +
> +  switch (customValueType) {
> +    case "boolean":
> +      customTextEl.type = "checkbox";
> +

Nit: remove blank line

@@ +274,5 @@
> +function BuildUI() {
> +  table = document.querySelector("table");
> +  let trs = table.querySelectorAll("tr:not(#add-custom-preference)");
> +  for (var i = 0; i < trs.length; i++) {
> +    table.removeChild(trs[i]);

You can also do:

trs[i].remove()

if you wish, but either is fine.[1]

[1]: https://developer.mozilla.org/en-US/docs/Web/API/ChildNode.remove

Comment 27

[Jen Fong-Adwent [:ednapiranha]]

Attachment: bug982484.patch

Comment 28

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

For future bugs, it can be helpful to reply to the feedback / review comments in the bug when posting an updated patch.  For example, maybe I misunderstood something or made a comment that does not make sense.  It's much easier to see a reply here to indicate that.

Review replies also make future reviews a bit easier, since I have a better idea of what differences I should see.

Of course, it's not necessary to reply to *every* review item, especially if you agree entirely and have done exactly what was mentioned.  Here's a random example[1] of back and forth discussion in the bug.

As well as helping the reviewer in future rounds, it's also just a good idea to capture this type of discussion in the bug, so future readers can understand how the work evolved.

I will say however that Bugzilla does make this particular activity a lot more annoying than it would be in a first-class code review system.  I am hopeful it will be much better with Review Board!

Anyway, I'll go ahead and review the current patch as-is.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=797639#c71

Comment 29

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

Comment on attachment 8513656 [details] [diff] [review]
bug982484.patch

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

I think I will hold off giving r+ until we have an icon here.  I want to double-check that it is added correctly, etc.  I can still give feedback on other changes before the icon appears, whatever you prefer.

* Currently on a wide screen, the 3rd column (buttons) takes up most of the space, but giving more space to pref name and value is more useful
  * One option is to use "table-layout: fixed", which allows specifying percentage columns widths while also wrapping long pref names
  * There are probably other options!  Play around with the layout a bit more and see what seems most usable to you.
  * Feel free to take some time with this if you like!  We still need an icon anyway, and we could be waiting for a while...
* Clicking in a integer or string value input immediately saves, even though no change was made
  * This is not the worst thing ever, but still would be nice to avoid saving until there's actually new data
* If you click in a integer or string value input, then type new value, it is not actually saved you to press enter or click again
  * We should probably save based on the change event to handle this case

Please do some more manual testing or write more automated tests for different input scenarios (keyboard, mouse)... behavior seems to change wildly each review round.  

Here's what I have been doing: 

1. Open WebIDE on one side of the screen and a tab with about:config on the other
2. Connect to local runtime
3. Open Device Prefs
4. Make changes in Device Prefs
5. Does about:config show the updated value?

about:config should update immediately for every change, so it's a helpful tool to double-check our work.

::: browser/devtools/webide/content/devicepreferences.js
@@ +56,5 @@
> +  return input;
> +}
> +
> +function ResetToDefault(name, input, button) {
> +  let defaultPref;

Wait, where did |clearUserPref| go?  Without it, the reset button does not actually do anything...

Also, let's add a test for resetting a pref.  We need to make sure this works right.

@@ +128,5 @@
> +
> +  switch (customValueType) {
> +    case "boolean":
> +      customTextEl.type = "checkbox";
> +      if (customTextEl.checked) {

This if block should be the same as:

customText = customTextEl.checked;

@@ +143,5 @@
> +      customTextEl.type = "text";
> +      break;
> +  }
> +
> +  if (event.target.classList.contains("new-editable")) {

We should do a validity check for new pref values too, not just old ones.

::: browser/devtools/webide/themes/devicepreferences.css
@@ +38,5 @@
> +#device-preferences {
> +  margin-top: 110px;
> +}
> +
> +#device-preferences tr td {

Nit: Don't need tr

@@ +44,5 @@
> +  border-bottom: 1px solid #ccc;
> +  border-right: 1px solid #fff;
> +}
> +
> +#device-preferences tr td.preference-name {

Nit: Don't need tr

@@ +48,5 @@
> +#device-preferences tr td.preference-name {
> +  width: 400px;
> +}
> +
> +#device-preferences tr td span {

Nit: Don't need tr

The span elements can be removed if you change to "table-layout: fixed".

@@ +58,5 @@
> +#device-preferences button {
> +  display: inline-block;
> +  font-family: sans-serif;
> +  font-size: 0.7rem;
> +  min-width: 100px;

Using fixed width works okay with existing strings, but most other languages are a bit longer, and it would be nice to keep it on one line.

Also, there are two different buttons, so one width setting is not enough.

Remove this and use "white-space: nowrap;" instead.

Comment 30

[Jen Fong-Adwent [:ednapiranha]]

(In reply to J. Ryan Stinnett [:jryans] from comment #29)
> Comment on attachment 8513656 [details] [diff] [review]
> bug982484.patch
> 
> Review of attachment 8513656 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think I will hold off giving r+ until we have an icon here.  I want to
> double-check that it is added correctly, etc.  I can still give feedback on
> other changes before the icon appears, whatever you prefer.

Yeah sounds good, if we don't get an icon by next week, I'm thinking maybe we'll just use the gear icon if there is one available? I believe you mentioned that earlier.

> 
> * Currently on a wide screen, the 3rd column (buttons) takes up most of the
> space, but giving more space to pref name and value is more useful
>   * One option is to use "table-layout: fixed", which allows specifying
> percentage columns widths while also wrapping long pref names

Yeah this might be a good option. I'll change to percentages.

>   * There are probably other options!  Play around with the layout a bit
> more and see what seems most usable to you.
>   * Feel free to take some time with this if you like!  We still need an
> icon anyway, and we could be waiting for a while...
> * Clicking in a integer or string value input immediately saves, even though
> no change was made
>   * This is not the worst thing ever, but still would be nice to avoid
> saving until there's actually new data
> * If you click in a integer or string value input, then type new value, it
> is not actually saved you to press enter or click again
>   * We should probably save based on the change event to handle this case
> 
> Please do some more manual testing or write more automated tests for
> different input scenarios (keyboard, mouse)... behavior seems to change
> wildly each review round.  
> 
> Here's what I have been doing: 
> 
> 1. Open WebIDE on one side of the screen and a tab with about:config on the
> other
> 2. Connect to local runtime
> 3. Open Device Prefs
> 4. Make changes in Device Prefs
> 5. Does about:config show the updated value?
> 
> about:config should update immediately for every change, so it's a helpful
> tool to double-check our work.
> 
> ::: browser/devtools/webide/content/devicepreferences.js
> @@ +56,5 @@
> > +  return input;
> > +}
> > +
> > +function ResetToDefault(name, input, button) {
> > +  let defaultPref;
> 
> Wait, where did |clearUserPref| go?  Without it, the reset button does not
> actually do anything...

Oops, accidental delete - will add w/ a test.

> 
> Also, let's add a test for resetting a pref.  We need to make sure this
> works right.
> 
> @@ +128,5 @@
> > +
> > +  switch (customValueType) {
> > +    case "boolean":
> > +      customTextEl.type = "checkbox";
> > +      if (customTextEl.checked) {
> 
> This if block should be the same as:
> 
> customText = customTextEl.checked;
> 
> @@ +143,5 @@
> > +      customTextEl.type = "text";
> > +      break;
> > +  }
> > +
> > +  if (event.target.classList.contains("new-editable")) {
> 
> We should do a validity check for new pref values too, not just old ones.
> 
> ::: browser/devtools/webide/themes/devicepreferences.css
> @@ +38,5 @@
> > +#device-preferences {
> > +  margin-top: 110px;
> > +}
> > +
> > +#device-preferences tr td {
> 
> Nit: Don't need tr
> 
> @@ +44,5 @@
> > +  border-bottom: 1px solid #ccc;
> > +  border-right: 1px solid #fff;
> > +}
> > +
> > +#device-preferences tr td.preference-name {
> 
> Nit: Don't need tr
> 
> @@ +48,5 @@
> > +#device-preferences tr td.preference-name {
> > +  width: 400px;
> > +}
> > +
> > +#device-preferences tr td span {
> 
> Nit: Don't need tr
> 
> The span elements can be removed if you change to "table-layout: fixed".
> 
> @@ +58,5 @@
> > +#device-preferences button {
> > +  display: inline-block;
> > +  font-family: sans-serif;
> > +  font-size: 0.7rem;
> > +  min-width: 100px;
> 
> Using fixed width works okay with existing strings, but most other languages
> are a bit longer, and it would be nice to keep it on one line.
> 
> Also, there are two different buttons, so one width setting is not enough.
> 
> Remove this and use "white-space: nowrap;" instead.

Comment 31

[Jen Fong-Adwent [:ednapiranha]]

Attachment: bug982484.patch

A few comments:

1. I've changed table: fixed to table: auto instead - it adjusts the layout a bit better when at various widths.
2. I've re-purposed the tool-options.png as the Device Preferences icon for now by adding it to the icons.png sprite file. We can replace it later if we find something more appropriate.
3. I've added some more tests for resetting custom and existing preferences, and a mouse event check.

Comment 32

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

Comment on attachment 8516952 [details] [diff] [review]
bug982484.patch

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

It looks like this patch needs to be rebased.

Also, Paul modified icons.png in bug 1063206, so you'll need to update yours.

Comment 33

[Jen Fong-Adwent [:ednapiranha]]

Attachment: bug982484.patch

rebased with updated icons.png

Comment 34

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

Comment on attachment 8517607 [details] [diff] [review]
bug982484.patch

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

r+ with comments below addressed.  I am excited to use this feature! :D

Please file a follow up bug to change the preference actor to wrap the "getXPref" functions in a try block and send an error back when the pref name is unknown.  Currently we trigger stack traces for the expected case of checking for a pref after resetting.

::: browser/devtools/webide/content/devicepreferences.js
@@ +94,5 @@
> +      });
> +      break;
> +  }
> +
> +  window.parent._defaultPrefPromise = defaultPref;

Remove this.  In the test, I show how to access this without copying.

@@ +96,5 @@
> +  }
> +
> +  window.parent._defaultPrefPromise = defaultPref;
> +  button.classList.add("hide");
> +  return window.parent._defaultPrefPromise;

I don't think anyone uses the return value of this function, so remove this.

@@ +297,5 @@
> +    }
> +  }
> +}
> +
> +let getAllPrefsPromise; // Used by tests

Remove this here, since it's not actually used by tests...

@@ +309,5 @@
> +
> +  if (AppManager.connection &&
> +      AppManager.connection.status == Connection.Status.CONNECTED &&
> +      AppManager.preferenceFront) {
> +    getAllPrefsPromise = AppManager.preferenceFront.getAllPrefs();

...and this can be simplified since you don't need to save to a variable first.

::: browser/devtools/webide/test/test_device_preferences.html
@@ +143,5 @@
> +          let resetBtn = doc.querySelector("#btn-new-integer-pref");
> +          resetBtn.click();
> +
> +          try {
> +            yield win._defaultPrefPromise;

This promise doesn't need to be copied up to the parent window.  Instead, you can use:

prefIframe.contentWindow.defaultPref

::: browser/devtools/webide/themes/devicepreferences.css
@@ +45,5 @@
> +}
> +
> +#device-preferences td.preference-name {
> +  width: 50%;
> +  min-width: 400px;

Is this worth having anymore with percentage widths?

Also, you can wrap super long names (like "security.ssl.allow_unrestricted_renego_everywhere__temporarily_available_pref") on small width screens by adding:

word-break: break-all;

Only add if it seems like an improvement to you.

Comment 35

[Jen Fong-Adwent [:ednapiranha]]

(In reply to J. Ryan Stinnett [:jryans] from comment #34)
> Comment on attachment 8517607 [details] [diff] [review]
> bug982484.patch
> 
> Review of attachment 8517607 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments below addressed.  I am excited to use this feature! :D
> 
> Please file a follow up bug to change the preference actor to wrap the
> "getXPref" functions in a try block and send an error back when the pref
> name is unknown.  Currently we trigger stack traces for the expected case of
> checking for a pref after resetting.
> 
> ::: browser/devtools/webide/content/devicepreferences.js
> @@ +94,5 @@
> > +      });
> > +      break;
> > +  }
> > +
> > +  window.parent._defaultPrefPromise = defaultPref;
> 
> Remove this.  In the test, I show how to access this without copying.
> 
> @@ +96,5 @@
> > +  }
> > +
> > +  window.parent._defaultPrefPromise = defaultPref;
> > +  button.classList.add("hide");
> > +  return window.parent._defaultPrefPromise;
> 
> I don't think anyone uses the return value of this function, so remove this.
> 
> @@ +297,5 @@
> > +    }
> > +  }
> > +}
> > +
> > +let getAllPrefsPromise; // Used by tests
> 
> Remove this here, since it's not actually used by tests...
> 
> @@ +309,5 @@
> > +
> > +  if (AppManager.connection &&
> > +      AppManager.connection.status == Connection.Status.CONNECTED &&
> > +      AppManager.preferenceFront) {
> > +    getAllPrefsPromise = AppManager.preferenceFront.getAllPrefs();
> 
> ...and this can be simplified since you don't need to save to a variable
> first.
> 
> ::: browser/devtools/webide/test/test_device_preferences.html
> @@ +143,5 @@
> > +          let resetBtn = doc.querySelector("#btn-new-integer-pref");
> > +          resetBtn.click();
> > +
> > +          try {
> > +            yield win._defaultPrefPromise;
> 
> This promise doesn't need to be copied up to the parent window.  Instead,
> you can use:
> 
> prefIframe.contentWindow.defaultPref
> 
> ::: browser/devtools/webide/themes/devicepreferences.css
> @@ +45,5 @@
> > +}
> > +
> > +#device-preferences td.preference-name {
> > +  width: 50%;
> > +  min-width: 400px;
> 
> Is this worth having anymore with percentage widths?

Do you mean it is worth it to increase the percentage to > 50 or less?

> 
> Also, you can wrap super long names (like
> "security.ssl.
> allow_unrestricted_renego_everywhere__temporarily_available_pref") on small
> width screens by adding:
> 
> word-break: break-all;
> 
> Only add if it seems like an improvement to you.

Yeah I can add that.

Comment 36

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

(In reply to Jen Fong-Adwent [:ednapiranha] from comment #35)
> > ::: browser/devtools/webide/themes/devicepreferences.css
> > @@ +45,5 @@
> > > +}
> > > +
> > > +#device-preferences td.preference-name {
> > > +  width: 50%;
> > > +  min-width: 400px;
> > 
> > Is this worth having anymore with percentage widths?
> 
> Do you mean it is worth it to increase the percentage to > 50 or less?

No, I mean is the "min-width: 400px;" needed since percentages are also in use?

Comment 37

[Jen Fong-Adwent [:ednapiranha]]

(In reply to J. Ryan Stinnett [:jryans] from comment #36)
> (In reply to Jen Fong-Adwent [:ednapiranha] from comment #35)
> > > ::: browser/devtools/webide/themes/devicepreferences.css
> > > @@ +45,5 @@
> > > > +}
> > > > +
> > > > +#device-preferences td.preference-name {
> > > > +  width: 50%;
> > > > +  min-width: 400px;
> > > 
> > > Is this worth having anymore with percentage widths?
> > 
> > Do you mean it is worth it to increase the percentage to > 50 or less?
> 
> No, I mean is the "min-width: 400px;" needed since percentages are also in
> use?

Oh yeah, yeah that will be needed. Or else you can shrink the window to be really small and that column will look confined :)

The min-width just ensures it won't look too strange and has some visually decent width constraint.

Comment 38

[Jen Fong-Adwent [:ednapiranha]]

Attachment: bug982484.patch

Comment 39

[Jen Fong-Adwent [:ednapiranha]]

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4a4c07a298a1

Comment 40

[Ryan VanderMeulen [:RyanVM]]

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

Comment 41

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

https://hg.mozilla.org/mozilla-central/rev/4bd45c680b98

Comment 42

[Will Bamberg [:wbamberg]]

There were docs on this before, but they got lost somehow when I split up the WebIDE page. The runtime menu has now got its own page, which includes a bit on this: https://developer.mozilla.org/en-US/docs/Tools/WebIDE/The_runtime_menu#Device_preferences.

Please let me know if you need anything else!

Comment 43

[Jen Fong-Adwent [:ednapiranha]]

Looks good, thanks!