107418 - about:config todo list

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, defect)

Description

[(not reading, please use [e-mail] instead)]

1) get sort indicators to show up
2) support column reordering (lxr on "ordinal")
3) persist the sort type 
4) persist the sorted column
5) bonus, restore selection after sorting 
6) bonus, ensure selected index is visible after sorting.
7) big bonus, support inline editing. (#17199)

I don't think we should waste a lot of time on about:config

it's most valuable for debugging, and for sample code on how to implement
nsIOutlinerView from js.

Comment 1

[Daniel Bratell]

about:config also used to be color coded in NS4.x.

Comment 2

[Chip Clark]

Attachment: patch to update sort indicators

Comment 3

[Chip Clark]

I removed the initial sort of the array,
added functions to handle the update of one or all of the sort indicators
added to the xul file the rdf to support this

Sorry - I should have done this earlier - when Seth made his fix for the sort

Comment 4

[Chip Clark]

kerz - can you review the code (and give me an r= if appropriate)?

Comment 5

[Jason Kersey]

Seems ok at first glance, but i know not of outliner so you'll want to find 
someone who does to review it properly.

Comment 6

[Chip Clark]

Attachment: correction for remnant code (which should not have been left in)

Comment 7

[sairuh (rarely reading bugmail)]

another item to add: context menus here need cleanup. most items don't do
anything, and might not even be relevant.

let me know if i should file this as a seperate bug.

Back - works
Forward - works
Reload - works
Stop - [actually, it never takes long enough to appear enabled]
----
View Page Source - shows source for window, but no content
View Page Info - sparse: info displayed undefined, unknown
----
Bookmark this Page - works
----
Save Page As - saving as an .html page results in the same content seen in view
source [saved page appears blank when loaded in browser]
----
Edit Page in Composer - launches blank composer window
Send Page - nothing happens
----
Select All - no selection occurs
Copy - nothing copied to clipboard [nothing to paste]

Comment 8

[(not reading, please use [e-mail] instead)]

taking about:config bugs from chipc.

Comment 9

[[e-mail]]

CCing some likely reviewers.

Comment 10

[[e-mail]]

Attachment: Proposed Patch

1) got sort indicators to show up
2) supported column reordering (lxr on "ordinal")
3) persisted the sort type 
4) persisted the sorted column
5) restored selection after sorting 
6) ensured selected index is visible after sorting.
7) added context menu with four options:
a) copy pref name
b) copy pref value
c) edit pref (if not locked) using prompt dialog
d) reset pref (if not locked or default)

Comment 11

[[e-mail]]

Attachment: forgot double-click :-)

Comment 12

[[e-mail]]

Attachment: Made about:config completely dynamic

This now makes about:config track pref changes and update its internal array.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

While you're at it, can you add an "Delete" to the context menu?

Comment 14

[Jeremy M. Dolan]

Applied the patch, works well. Was thinking of doing this myself just
yesterday... should have figured there was a bug on it. ('Reset' is quite nice!)
Only a few small nits:

1) Do we really need the ability to add new prefs?
2) I wasn't able to add a pref without a '.' in it. This may be correct.
3) When prefs were sorted by name, and I added a pref named 'aaa.bbb', it was
   added at the bottom, under xul prefs, rather then the top.
4) As cbiesinger pointed out, there was no way for me to delete 'aaa.bbb' after
   creating it.
5) Probably not easy, but just in case it is... when values are changed, or
   prefs added/removed, might be nice to have undo/redo in the edit menu work.
6) How about edit->copy to copy an entire line (name, status, type, value).
7) Again, if it's easy enough, ability to select multiple prefs at once with
   ctrl and shift, to copy them would be nice.
8) Can Copy Name and Copy Value also copy the info to the X cutbuffer? The
   current 'copying' as relatively useless to me.

Comment 15

[Boris Zbarsky [:bzbarsky]]

You mean PRIMARY, not CUT_BUFFER, right?

Comment 16

[Jeremy M. Dolan]

I have no idea what it's called. Just make it so middle click into my mail and
IRC apps running in an xterm actually paste. :)

There's a bug on JS console to do this too. Currently I go to a bugzilla page,
ctrl-v paste into the additional comments text box, select it, THEN middle click
my xterm. Fun.

Comment 17

[[e-mail]]

> Do we really need the ability to add new prefs?
It's easier to remove it then add it in later...
> I wasn't able to add a pref without a '.' in it. This may be correct.
Where there any JS console errors?
> When prefs were sorted by name, and I added a pref named 'aaa.bbb', it was
  added at the bottom, under xul prefs, rather then the top.
Well, I didn't want to force a sort, but it could be done.
> As cbiesinger pointed out, there was no way for me to delete 'aaa.bbb' after
  creating it.
You'll need to file a bug under preferences back end to fix that.
> Probably not easy, but just in case it is... when values are changed, or
  prefs added/removed, might be nice to have undo/redo in the edit menu work.
I'm not sure how easy it is to access the edit menu at all, actually.
> How about edit->copy to copy an entire line (name, status, type, value).
See above.
> Again, if it's easy enough, ability to select multiple prefs at once with
  ctrl and shift, to copy them would be nice.
There's a problem with line delimiters (it also affects the JS console)
> Can Copy Name and Copy Value also copy the info to the X cutbuffer? The
  current 'copying' as relatively useless to me.
Sure, Pike told me what the fix is.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> > As cbiesinger pointed out, there was no way for me to delete 'aaa.bbb' after
> >  creating it.
> You'll need to file a bug under preferences back end to fix that.

See clearUserPref()

Comment 19

[[e-mail]]

> > > As cbiesinger pointed out, there was no way for me to delete 'aaa.bbb'
> > > after creating it.
> > You'll need to file a bug under preferences back end to fix that.
> See clearUserPref()
clear != delete. After using clearUserPref getPrefType still works (and
getPrefValue also works if the pref has a default value).

Comment 20

[[e-mail]]

Attachment: Two new fixes

The copy functions set both clipboards and new prefs get sorted into place for
name or type sort order (it would be too slow for other sort orders).

Comment 21

[[e-mail]]

Attachment: Fix bug in previous patch

Comment 22

[[e-mail]]

Comment on attachment 90338 [details] [diff] [review]
Fix bug in previous patch

>+  <treechildren id="configTreeBody" ondblclick="Modify();"/>

Nit: This should read

<treechildren id="configTreeBody" ondblclick="if (event.button == 0)
Modify();"/>

Comment 23

[[e-mail]]

Attachment: Updated for bitrot

Comment 24

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: comments about attachment 95384

Comment 25

[[e-mail]]

Attachment: Addressed comments

Comment 26

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 107913 [details] [diff] [review]
Addressed comments

I just did a quick run through of this patch, and found some issues I'd like to
see addressed before I do a more thorough review.


>Index: content/config.js
>@@ -20,245 +20,377 @@
>+const nsISupportsString = Components.interfaces.nsISupportsString;

Use nsIPrefLocalizedString instead.


>+const nsIPromptService = Components.interfaces.nsIPromptService;
>+const nsIPrefService = Components.interfaces.nsIPrefService;
>+const nsIClipboardHelper = Components.interfaces.nsIClipboardHelper;
> const nsIAtomService = Components.interfaces.nsIAtomService;
>+const nsPrompt_CONTRACTID = "@mozilla.org/embedcomp/prompt-service;1";
>+const nsPref_CONTRACTID = "@mozilla.org/preferences-service;1";

Please use nsPrefService_CONTRACTID as the variable name.  Otherwise it is too
close to nsIPref which is deprecated.


> // XXX get these from a string bundle
>-var gLockStrs = ["default", "user set","locked"]; 
>+const gLockStrs = ["default", "user set", "locked"];
>+const gTypeStrs = ["string", "integer", "boolean"];
>+
>+const kDefault = 0;
>+const kUserSet = 1;
>+const kLocked = 2;
> 
> const kStrType = 0;
> const kIntType = 1;
> const kBoolType = 2;

Can these consts be removed?  Why not use nsIPrefBranch.PREF_STRING and
friends?  Your array can then be changed to a map between them as such. 
Further, it would make getting them from a string bundle fit right in if you
wish to do something like:

var gTypeStrs = [];
gTypeStrs[nsIPrefBranch.PREF_BOOL] = "bool"; // you can do this for now I guess
if you aren't doing bundles...
gTypeStrs[nsIPrefBranch.PREF_STRING] =
strBundle.getStringFromName("about-config-pref_string");


>+function fetchPref(prefName, prefIndex)
>+{
...
>+  try {
>+    switch (prefBranch.getPrefType(prefName)) {
>+      case prefBranch.PREF_BOOL:

This would be cleaner if you had a const nsIPrefBranch =
Components.interfaces.nsIPrefBranch at the top of the file, and used that here
for resolving your pref type constants.

>+        pref.typeCol = kBoolType;
>+        // convert to a string
>+        pref.valueCol = prefBranch.getBoolPref(prefName) ? "true" : "false";

pref.valueCol = prefBranch.getBoolPref(prefName).toString();

>+        break;
>+      case prefBranch.PREF_INT:
>+        pref.typeCol = kIntType;
>+        // convert to a string
>+        pref.valueCol = "" + prefBranch.getIntPref(prefName);

pref.valueCol = prefBranch.getIntPref(prefName).toString();

>+        break;
>+      //case prefBranch.PREF_STRING:

Uncomment the previous line?  It makes the code more readable.

>+      default:
>+        pref.valueCol = prefBranch.getComplexValue(prefName, nsISupportsString).data;
>+        break;
>+    }
>+  }

I hate to see exceptions like this get eaten, especially on a mainly power user
feature.  A dump may be nice here, although I don't care too much.  In any
case, if you ARE going to eat the exception, at least comment that you are
doing so intentionally.

>+  finally {
>+    return pref;
>+  }
>+}

>+function ModifyPref(entry)
>+{
...
>+    default:
>+      var SupportsString = {
>+        data: result.value,
>+        QueryInterface: function(iid) { return this; }
>+      };

Eek!  No.  Don't do that.  you want to .createInstance() here.	If a method
expects an XPCOM object, make sure you give it one and not a JS object which
looks like one.  If the c++ implementation of the methods you call ever change,
it is possible that you can crash if it wants things on the object which you
don't define in JS.  (And make sure to use nsIPrefLocalizedString like I asked
earlier, btw)

>+      prefBranch.setComplexValue(entry.prefCol, nsISupportsString, SupportsString);
>+      break;
>+  }
>+  return true;
>+}

Comment 27

[[e-mail]]

> Eek!  No.  Don't do that.  you want to .createInstance() here.	If a method
> expects an XPCOM object, make sure you give it one and not a JS object which
> looks like one.

Since when is passing JS objects through XPConnect verboten?

Comment 28

[Christopher Aillon (sabbatical, not receiving bugmail)]

Read the contract.

http://lxr.mozilla.org/mozilla/source/modules/libpref/public/nsIPrefBranch.idl#162

174    * @param aValue    The XPCOM object into which to the complex preference 
175    *                  value should be retrieved.

It does not say JS Object, it says XPCOM object.  If you break the contract,
then the method can do whatever it wants to spite you.  If that means crashing,
or hanging, or throwing exceptions, or any other thing like giving you something
unexpected back, well you broke the contract so you should not expect the method
to follow the contract either.

Comment 29

[[e-mail]]

For the record, caillon provided a more thorough argument on IRC.

Comment 30

[[e-mail]]

Attachment: Addressed caillon's comments

Comment 31

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 108752 [details] [diff] [review]
Addressed caillon's comments

Sorry for the delay in getting to this!

>Index: content/config.js

>+const nsIPrefLocalizedString = Components.interfaces.nsIPrefLocalizedString;
>+const nsIPromptService = Components.interfaces.nsIPromptService;
>+const nsIPrefService = Components.interfaces.nsIPrefService;
>+const nsIPrefBranch = Components.interfaces.nsIPrefBranch;
>+const nsIClipboardHelper = Components.interfaces.nsIClipboardHelper;
> const nsIAtomService = Components.interfaces.nsIAtomService;

Could you add a new line here, to make it easier to see that the types of
values you're getting change in between here?

>+const nsPrefLocalizedString_CONTRACTID = "@mozilla.org/pref-localizedstring;1";
>+const nsPrompt_CONTRACTID = "@mozilla.org/embedcomp/prompt-service;1";
>+const nsPrefService_CONTRACTID = "@mozilla.org/preferences-service;1";
>+const nsClipboardHelper_CONTRACTID = "@mozilla.org/widget/clipboardhelper;1";
> const nsAtomService_CONTRACTID = "@mozilla.org/atom-service;1";
> 
>-var atomService = Components.classes[nsAtomService_CONTRACTID].getService(nsIAtomService);
>-var gLockAtoms = [atomService.getAtom("default"), atomService.getAtom("user"), atomService.getAtom("locked")];
>+const promptService = Components.classes[nsPrompt_CONTRACTID].getService(nsIPromptService);
>+const prefService = Components.classes[nsPrefService_CONTRACTID].getService(nsIPrefService);
>+const prefBranch = prefService.getBranch(null).QueryInterface(Components.interfaces.nsIPrefBranchInternal);
>+const clipboardHelper = Components.classes[nsClipboardHelper_CONTRACTID].getService(nsIClipboardHelper);
>+const atomService = Components.classes[nsAtomService_CONTRACTID].getService(nsIAtomService);

prefer names prefixed with 'g' for the above instances.


>+const gLockAtoms = [atomService.getAtom("default"), atomService.getAtom("user"), atomService.getAtom("locked")];
...
>+const gLockStrs = ["default", "user set", "locked"];
>+const gTypeStrs = [];

Just use |var| rather than |const| for the three above declarations.  |const|
here is useless, since it does not prevent an array from being written to (as
you yourself demonstrate just below)

>+gTypeStrs[nsIPrefBranch.PREF_STRING] = "string";
>+gTypeStrs[nsIPrefBranch.PREF_INT] = "integer";
>+gTypeStrs[nsIPrefBranch.PREF_BOOL] = "boolean";

...

>+const kDefault = 0;
>+const kUserSet = 1;
>+const kLocked = 2;

Also, if you're using consts, use uppercase, and maybe more descriptive names. 
PREF_IS_DEFAULT_VALUE, PREF_IS_USER_SET, PREF_IS_LOCKED maybe?

...

>+  cycleHeader: function(colID, elt) {
>+    var index = this.selection.currentIndex;
>+    if (colID == gSortedColumn)
>+      gSortDirection = -gSortDirection;

Hmm, don't you want to switch the direction only if you're going to be doing
the reverse() below?  Shouldn't they be in the same block?

>+    if (colID == gSortedColumn && gFastIndex == gPrefArray.length) {
>+      gPrefArray.reverse();
>+      if (index >= 0)
>+        index = gPrefArray.length - index - 1;
>+    }

...

>+function getIndexOfPref(pref)
>+{
>+  var low = -1, high = gFastIndex;
>+  var index = (low + high) >> 1;

Why not use division (and subtracting maybe?) here rather than bitshifting? 
That is the standard way to write searches like this.  If nothing else, there
should definitely be a comment, explaining it, but I'd prefer the division. 

>+  while (index > low) {
>+    var mid = gPrefArray[index];
>+    if (mid == pref)
>+      return index;
>+    if (gSortFunction(mid, pref) < 0)
>+      low = index;
>+    else
>+      high = index;
>+    index = (low + high) >> 1;
>+  }

New line here?

Right below, would it be faster to cache the length in a var rather than look
it up every iteration?	(if nothing else, we probably will save the time to go
through XPConnect each iteration)

>+  for (index = gFastIndex; index < gPrefArray.length; ++index)
>+    if (gPrefArray[index] == pref)
>+      break;
>+  return index;
>+}

...

>+function fetchPref(prefName, prefIndex)
>+{
>+  var pref = {
>+    prefCol: prefName,
>+    lockCol: kDefault,
>+    typeCol: nsIPrefBranch.PREF_STRING,
>+    valueCol: ""
>+  };

You end up returning the above object type, though it is local to this
function.  I know it works in JS, but "it works" is no reason to do it.  I'd
prefer you declare it in the global scope, with a more descriptive name and a
comment as to its intended usage and users.  Then when you want to use a new
local pref object, just do var prefObj = new PrefDataObject(prefName, prefIdx);
or similar.

>+
>+  gPrefHash[prefName] = pref;
>+  gPrefArray[prefIndex] = pref;
>+
>+  if (prefBranch.prefIsLocked(prefName))
>+    pref.lockCol = kLocked;
>+  else if (prefBranch.prefHasUserValue(prefName))
>+    pref.lockCol = kUserSet;
>+
>+  try {
>+    switch (prefBranch.getPrefType(prefName)) {
>+      case prefBranch.PREF_BOOL:
>+        pref.typeCol = prefBranch.PREF_BOOL;
>+        // convert to a string
>+        pref.valueCol = prefBranch.getBoolPref(prefName).toString();
>+        break;
>+      case prefBranch.PREF_INT:
>+        pref.typeCol = prefBranch.PREF_INT;
>+        // convert to a string
>+        pref.valueCol = prefBranch.getIntPref(prefName).toString();
>+        break;
>+      default:
>+      case prefBranch.PREF_STRING:
>+        pref.valueCol = prefBranch.getComplexValue(prefName, nsIPrefLocalizedString).data;
>+        break;
>+    }
>+  }

When I asked you to comment this, I meant to put it into an empty catch.  That
way we know that you are "handling" exceptions per the comment.  Move the
return to the function scope, rather than a finally.

>+  // There are obscure cases in which you can't tell in advance
>+  // that the pref exists but has no user or default value...
>+  finally {
>+    return pref;
>+  }
>+}

...

>+function onConfigLoad()
>+{
...
Should you cache the prefCount.value?

>+  for (var i = 0; i < prefCount.value; ++i) 
>+  {
>+    var prefName = prefArray[i];
>+    if (/capability/.test(prefName)) // avoid displaying "private" preferences
>+      continue;
> 
>-        prefArray[i] = htmlEscape(prefArray[i]);
>-        baseArray[k] = {indexCol:prefIndex, prefCol:prefArray[i], lockCol:prefLockState, typeCol:prefType, valueCol:prefValue};
>-    }
>+    fetchPref(prefName, gPrefArray.length);

Hmm, what are you doing with the pref object you fetch?  Seems like this is a
wasted call?

....

>+var gSortFunctions = { prefCol: prefColSortFunction, 
>+                       lockCol: lockColSortFunction, 
>+                       typeCol: typeColSortFunction, 
>+                       valueCol: valueColSortFunction};

var foo = {
  bar: val1,
  bat: val2,
  baz: val3
};

> 
>-function lockColSortFunction(x,y)
>+function copyValue()
> {
>-  return stringSortFunction(x.lockCol, y.lockCol);
>+  clipboardHelper.copyString(gPrefArray[view.selection.currentIndex].valueCol);
> }
> 
>-function typeColSortFunction(x,y)
>+function Modify()
> {
>-  return intSortFunction(x.typeCol, y.typeCol);
>+  ModifyPref(gPrefArray[view.selection.currentIndex]);
> }
> 
>-function valueColSortFunction(x,y)
>+function Reset()
> {
>-  return stringSortFunction(x.valueCol, y.valueCol);
>+  var entry = gPrefArray[view.selection.currentIndex];
>+  prefBranch.clearUserPref(entry.prefCol);
> }
> 
>-var gSortFunctions = {indexCol:indexColSortFunction, 
>-                 prefCol:prefColSortFunction, 
>-                 lockCol:lockColSortFunction, 
>-                 typeCol:typeColSortFunction, 
>-                 valueCol:valueColSortFunction};
>-
>-function HandleColumnClick(id)
>+function New(element)

Can you use a different name besides |New|?  It is too close to the reserved
keyword |new|, and not very descriptive of what it does.

> {
>-  if (id == gSortedColumn) {
>-    gSortAscending = !gSortAscending;
>-    baseArray.reverse();
>+  var type = parseInt(element.getAttribute("value"));
>+  var result = {value: ""};
>+  var dummy = {value: 0};

Space between the braces and the literals, e.g.  |{ foo: bar };| rather than
|{foo: bar};|

>+  // XXX get these from a string bundle

Since you're already doing extensive changes, a string bundle isn't really that
much work.  But I'll let this slide...

>+  if (promptService.prompt(window,
>+                           "New " + gTypeStrs[type] + " value",
>+                           "Enter the preference name",
>+                           result,
>+                           null,
>+                           dummy)) {
>+    var pref;
>+    if (result.value in gPrefHash)
>+      pref = gPrefHash[result.value];
>+    else
>+      pref = {prefCol: result.value, lockCol: kDefault, typeCol: type, valueCol: ""};

Brace space.

>+    if (ModifyPref(pref))
>+      setTimeout(gotoPref, 0, result.value);
>   }
>-  else {
>-    baseArray.sort(gSortFunctions[id]);
>-    gSortedColumn = id;
>-  }
>-
>-  gTree.treeBoxObject.invalidate();
> }

...

>+function ModifyPref(entry)
>+{
>+  if (entry.lockCol == kLocked)
>+    return false;
>+  var result = {value: entry.valueCol};
>+  var dummy = {value: 0};
>+  // XXX get this from a string bundle
>+  if (!promptService.prompt(window,
>+                            "Enter " + gTypeStrs[entry.typeCol] + " value",
>+                            entry.prefCol,
>+                            result,
>+                            null,
>+                            dummy))
>+    return false;
>+  switch (entry.typeCol) {
>+    case nsIPrefBranch.PREF_BOOL:
>+      prefBranch.setBoolPref(entry.prefCol, eval(result.value));
>+      break;
>+    case nsIPrefBranch.PREF_INT:
>+      prefBranch.setIntPref(entry.prefCol, eval(result.value));
>+      break;
>+    default:
>+    case nsIPrefBranch.PREF_STRING:
>+      var prefLocalizedString = Components.classes[nsPrefLocalizedString_CONTRACTID].createInstance(nsIPrefLocalizedString);
>+      prefLocalizedString.data = result.value;
>+      prefBranch.setComplexValue(entry.prefCol, nsIPrefLocalizedString, prefLocalizedString);
>+      break;
>+  }

Setting a pref could fail if the prefs file was somehow not able to be written
to, couldn't it?  (disk full, someone accidentally chmod'd it, etc).  Or
possibly you're OOM and can't create a new prefLocalizedString?  Rare edge
cases surely, but they probably should be expected and dealt with.


Otherwise, everything looks fine and I'm about ready to r= this but I want to
see another patch before doing so.

Comment 32

[[e-mail]]

>>+  cycleHeader: function(colID, elt) {
>>+    var index = this.selection.currentIndex;
>>+    if (colID == gSortedColumn)
>>+      gSortDirection = -gSortDirection;
>
>Hmm, don't you want to switch the direction only if you're going to be doing
>the reverse() below?  Shouldn't they be in the same block?
I can only use reverse() if I know that I can do a fast reverse().
Otherwise I always have to sort.

>>+function getIndexOfPref(pref)
>>+{
>>+  var low = -1, high = gFastIndex;
>>+  var index = (low + high) >> 1;
>
>Why not use division (and subtracting maybe?) here rather than bitshifting?
Difficult when I don't have an integer division operator.
 
>Right below, would it be faster to cache the length in a var rather than look
>it up every iteration?	(if nothing else, we probably will save the time to go
>through XPConnect each iteration)
>
>>+  for (index = gFastIndex; index < gPrefArray.length; ++index)
>>+    if (gPrefArray[index] == pref)
>>+      break;
>>+  return index;
>>+}
I didn't think .length would be slow on a JS array.
Also I don't expect many iterations here.

>>+function fetchPref(prefName, prefIndex)
>>+{
>>+  var pref = {
>>+    prefCol: prefName,
>>+    lockCol: kDefault,
>>+    typeCol: nsIPrefBranch.PREF_STRING,
>>+    valueCol: ""
>>+  };
>
>You end up returning the above object type, though it is local to this
>function.  I know it works in JS, but "it works" is no reason to do it.  I'd
>prefer you declare it in the global scope, with a more descriptive name and a
>comment as to its intended usage and users.  Then when you want to use a new
>local pref object, just do var prefObj = new PrefDataObject(prefName, prefIdx);
>or similar.
Well I almost never need to use the object - there's only one case were I need
the object itself after creating it and adding it to the array and hash. So
perhaps I should figure out a way to avoid returning it instead.

>>+function onConfigLoad()
>>+{
>...
>Should you cache the prefCount.value?
It's a JS property, should be reasonably fast?

>Setting a pref could fail if the prefs file was somehow not able to be written
>to, couldn't it?  (disk full, someone accidentally chmod'd it, etc).  Or
>possibly you're OOM and can't create a new prefLocalizedString?  Rare edge
>cases surely, but they probably should be expected and dealt with.
How?

Comment 33

[Boris Zbarsky [:bzbarsky]]

> I know it works in JS, but "it works" is no reason to do it.

No.  And using functions "just works", but that's not a reason to do it.  For
that matter, using classes shouldn't be done just because it's doable...

Is there a real reason for declaring a constructor for that object if it's only
ever constructed in one place?  Apart from that, your suggested code is
identical to Neil's (and also just returns a local)...

Comment 34

[[e-mail]]

Attachment: Addressed some of caillon's comments

Comment 35

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 111164 [details] [diff] [review]
Addressed some of caillon's comments

>+function getIndexOfPref(pref)
> {
>+  var low = -1, high = gFastIndex;
>+  var index = (low + high) >> 1;
>+  while (index > low) {
>+    var mid = gPrefArray[index];
>+    if (mid == pref)
>+      return index;
>+    if (gSortFunction(mid, pref) < 0)
>+      low = index;
>+    else
>+      high = index;
>+    index = (low + high) >> 1;
>+  }
>+  for (index = gFastIndex; index < gPrefArray.length; ++index)
>+    if (gPrefArray[index] == pref)
>+      break;
>+  return index;
>+}

It is very unlikely to ever happen, but if the pref does not exist, you will be
returned an index that is equal to your array length, and that may break
callers.  You might want to check that you have a valid index before returning,
and return -1 otherwise.  And your callers probably should check for a value !=
-1.



>+function onConfigLoad()
>+{
>+  document.title = "about:config";

Wanna add an XXX comment about a stringbundle here?  I can see how someone may
want to change this title.



>Index: content/config.xul
>===================================================================

>-  <treechildren id="configTreeBody"/>
>+  <treechildren id="configTreeBody" ondblclick="if (event.button == 0) Modify();"/>

This should call ModifySelected(), no?


r=caillon with the comments addressed.

Comment 36

[[e-mail]]

caillon: not true, getIndexOfPref is only passed an existing pref object.

Also, the document title is the url, so I don't see the point of changing it...

But thanks for pointing out my typo :-/

Comment 37

[[e-mail]]

Comment on attachment 111164 [details] [diff] [review]
Addressed some of caillon's comments

OK, I've changed Modify() to ModifySelected() in my copy of the patch.

Comment 38

[Christopher Aillon (sabbatical, not receiving bugmail)]

If it's only passed real prefs, fine.  But at the very least, add a comment
stating that it's a precondition for the pref to exist.  (Don't assume future
callers will do that unless you tell them to with a comment)

Why should the title be the URL?  That's already in the urlbar.  It is redundant
to have it as the title, IMO.  It would be cooler to name it "My preferences" or
something, so that someone who has no idea what about:config is and stumbles on
it by accident would have a quicker idea.  Plus, about:config only really has a
meaning in english.  In other languages, it is just a random URL.  IMO, all
chrome urls should have descriptive titles.  However, that is a different bug
and I don't think you should change it here.

Comment 39

[(not reading, please use [e-mail] instead)]

Comment on attachment 111164 [details] [diff] [review]
Addressed some of caillon's comments

automatic rs=sspitzer, since callion has given a thorough review.

about:config is such a power user thing, I'm ok with letting neil do what ever
he sees fit.

Comment 40

[Andreas Kunz]

The CVS checkin comment mentions bug 107148 which is almost, but not exactly,
this bug... may be confusing...

Comment 41

[[e-mail]]

Oops, looks like I mistyped the checkin comment for bug 107418 as bug 107148 :-(

Comment 42

[[e-mail]]

Any new enchancements can be filed separately. Marking FIXED.

Comment 43

[Koike Kazuhiko]

*** Bug 155179 has been marked as a duplicate of this bug. ***

Comment 44

[Paul Baker]

is this supposed to be working under Mac OS X? (build 2003012008)

Comment 45

[Juha Luoma]

Attachment: Screenshot about what I get with about:config

With trunk build 2003012008 on w2k I only get empty list, see attachment.

Comment 46

[Andreas Kunz]

With 2003012005 on Win2k it works for me.

Comment 47

[Karthik Sheka]

Does *not* work for me.
WinXP Build 2003012008

Looks like we narrowed it down. :-)

Comment 48

[Andreas Kunz]

Bug 189452?
Checked in at 01/20/2003 05:39 (not marked resolved yet).

Comment 49

[Andreas Kunz]

See bug 189452 comment #4 (or your JS console...) for more info.
That bug caused the problem, not this bug here.

Comment 50

[Adam D. Moss]

Does not work for me, Linux CVS HEAD.
I get the headers, no content.

Comment 51

[Andreas Kunz]

Adam, that is bug 189835.
*This* bug here is fixed.

Comment 52

[amutch]

This does not work properly in MFCEmbed. Using the latest nightly (1/27/2003), 
entering "about:config" shows the headers and all of the preferences. You can 
even edit the preferences and they appear as changed in the preferences 
displayed. However, the changes don't get saved and don't appear changed when 
the browser is restarted. Let me know if this needs to be filed as a separate 
bug.

Comment 53

[sairuh (rarely reading bugmail)]

vrfy'd fixed with comm trunk (2003.02.12) builds on linux rh8.0, win2k and mac
10.2.3. tested the items in comment 10 and comment 11.

neil, thanks so much for doing this!

Comment 54

[Brian 'netdragon' Bober]

See also bug 195845 about a description for prefs.

Comment 55

[Brendan Eich [:brendan]]

Comment on attachment 107902 [details]
comments about attachment 95384 [details] [diff] [review]

>>+    if (colID == gSortedColumn && gFastIndex == gPrefArray.length) {
>>+      gPrefArray.reverse();
>>+      if (index >= 0)
>>+        index = gPrefArray.length - index - 1;
>>+    } else {
>
>else on its own line

Is that prevailing style?  Not in lots of Mozilla code.

>>+  isEditable: function(row, colID) {return false},
>
>missing ;

Why care?

>>+  for (index = gFastIndex; index < gPrefArray.length; index++)
>
>++index

No.  There's no good reason for such style mongering here, in C, or in C++ with
scalar types.  One size does not fit all.

Pedantry is bad if there's no payoff.  Pedagogy is good for kids.  We're all
adults here, right?

I'm not sure what else to say.

/be

Comment 56

[Brendan Eich [:brendan]]

I'm also wondering (late to this bug, sorry) why JS code is being reviewed as if
it were written in C++ ((low + high) >> 1 is *necessary*; object literals are
good; etc.).  Nuff said.

Grump.

/be