Closed
Bug 37592
Opened 25 years ago
Closed 23 years ago
Need about:config implemented
Categories
(SeaMonkey :: Preferences, defect, P3)
SeaMonkey
Preferences
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.4
People
(Reporter: yoel, Assigned: chipc)
References
Details
Attachments
(26 files)
6.19 KB,
patch
|
Details | Diff | Splinter Review | |
6.85 KB,
text/plain
|
Details | |
2.21 KB,
text/plain
|
Details | |
2.23 KB,
patch
|
Details | Diff | Splinter Review | |
3.86 KB,
text/html
|
Details | |
5.03 KB,
text/html
|
Details | |
1.31 KB,
text/html
|
Details | |
4.98 KB,
text/html
|
Details | |
5.16 KB,
patch
|
Details | Diff | Splinter Review | |
1.93 KB,
text/plain
|
Details | |
1.67 KB,
text/plain
|
Details | |
1.32 KB,
text/html
|
Details | |
4.79 KB,
text/html
|
Details | |
5.91 KB,
patch
|
Details | Diff | Splinter Review | |
1.92 KB,
text/plain
|
Details | |
1.67 KB,
text/plain
|
Details | |
1.09 KB,
text/plain
|
Details | |
1.71 KB,
text/plain
|
Details | |
1.91 KB,
text/plain
|
Details | |
5.28 KB,
text/plain
|
Details | |
4.25 KB,
text/plain
|
Details | |
4.16 KB,
text/plain
|
Details | |
20.28 KB,
patch
|
Details | Diff | Splinter Review | |
20.25 KB,
patch
|
Details | Diff | Splinter Review | |
18.71 KB,
patch
|
Details | Diff | Splinter Review | |
18.71 KB,
patch
|
Details | Diff | Splinter Review |
This is a 4.x farity feature request
When "about:config" is entered into the location field of the 4.x browser the
current user preferences would be displayed.
Prefernces were color coated to differemtiate how they were set.
GREEN=default preferences that can be changed through the user
interface.
BLUE=preferences that have been set in the preference file.
BLACK=factory defaults
RED=locked using Mission Control or the Client Customization Kit
Reproducible: Always
Steps to Reproduce:
type about:config in the URL area
Actual Results: browser does not respond
Comment 2•25 years ago
|
||
XPApps or Preferences. Trying XPApps first.
Updating component, owner and setting bug to New.
Assignee: asadotzler → don
Status: UNCONFIRMED → NEW
Component: Browser-General → XPApps
Ever confirmed: true
QA Contact: jelwell → sairuh
Comment 3•25 years ago
|
||
Reassigning as per Don
Putting on [nsbeta2-] radar. Not critical to beta2. Adding nsbeta3 keyword to
make sure this works for beta3. Needed for debugging.
Comment 7•25 years ago
|
||
Sorry for the spam. New QA Contact for Browser General. Thanks for your help
Joseph (good luck with the new job) and welcome aboard Doron Rosenberg
QA Contact: jelwell → doronr
Comment 8•24 years ago
|
||
Removing nsbeta3 keyword. I don't think this is essential to ship.
Keywords: nsbeta3
Comment 10•24 years ago
|
||
->alecf to match bug 57969. but feel free to change as needed!
Comment 11•24 years ago
|
||
We would like to have this in order to test preference locking with
netscape.cfg. This is necessary for CCK and other MCD-style tools.
Updated•24 years ago
|
Assignee | ||
Comment 13•24 years ago
|
||
Assignee | ||
Comment 14•24 years ago
|
||
Assignee | ||
Comment 15•24 years ago
|
||
Assignee | ||
Comment 16•24 years ago
|
||
preliminary work and proof of concept done on Win32.
currently
black = default (it used to be green in 4.x)
blue = user setting
red = locked setting
green = not used (black used to be pref set by config method)
The list is not sorted - just dumped.
Comments welcome - this was just a first pass
Status: NEW → ASSIGNED
Comment 17•24 years ago
|
||
Security implications of about:config? can it be accessed through the DOM and
thus vulnerable to exploits (ala 4.x)?
Comment 18•24 years ago
|
||
Hong, it should not be accessible through the DOM. There are two layers of
protection in place, the URI-loading policy and the same-origin policy, both of
which should block access to any about: URL other than the few we know are safe
(about:, about:blank, about:mozilla, and about:credits). If you find you can
access about:config from a script loaded form the web, please file a bug on me.
Comment 19•24 years ago
|
||
seems pretty reasonable, but please don't check in the first two changes in the
patch (i.e. the NS_WITH_SERVICE upgrade, or the inclusion of nsCOMPtr.h) -
they're not necessary and I'd rather nsCOMPtr.h be included in as few .h files
as possible.
Comment 20•24 years ago
|
||
(that comment was on the patch, I'm reviewing the .h/.cpp files now)
Comment 21•24 years ago
|
||
ok, initial comments on the patch:
- use CSS, dont use the <font> tag. Instead, define something at the top of the
file like
<style type="text/css">
.default: { font-color: green }
(etc)
</style>
then you can define the tags like
<div class="default"><b>mail.foo.blah</b> = value</div>
(this is just a start, but find a book on CSS to get you started)
- there are lots of giant nested if's that seem really unnecessary..
- use stack space for fixed-size buffers, instead of allocating them
- don't hardcode values in variables, use #define instead
- you can't use != for string comparison, please find a book on C and read the
chapter on strings, that is a very basic concept!
- use i++, not ++i unless you know what you're doing
- what is/was childList? it's not actually used anywhere, yet you're allocating
memory for it and not freeing it
bufferSize = 8192;
childList = (char *)nsMemory::Alloc(sizeof(char) * bufferSize);
if (childList != nsnull)
{
for (i = 0; i < theCount; ++i)
{
prefName = childArray[i];
if(prefName != "")
{
This could just be:
for (i = 0; i< theCount; i++) {
if (!childArray[i] || nsCRT::strcmp(childArray[i], "")==0)
continue;
..
}
You should always review your OWN code, before requesting others to review it..
Please read:
http://www.mozilla.org/hacking/mozilla-style-guide.html - a work in progress,
not required reading (yet!)
before submitting another patch
Assignee | ||
Comment 22•24 years ago
|
||
<alecf> thanks for the comments
The patch wasn't expected to be prime-time, but I will certainly add your items
to the existing list of things to be done prior to check-in.
Right now I'm still going thru it for things. Then bnesse will get his shot at
it before I even consider passing it for r or sr.
Comment 23•24 years ago
|
||
sorry :)
I was just scared by some of the more glaring errors..
Comment 24•24 years ago
|
||
might as well get the html 100% right for the start..:
"<B>" should be "<strong>"
"</B>" should be "</strong>"
"<H1>" should be "<h1>"
"</H1>" should be "</h1>"
Comment 25•24 years ago
|
||
Can someone explain why this isn't being done using javascript and the generic
about redirector?
Comment 26•24 years ago
|
||
Ideally preferences would be reflected (until natively implemented) as an RDF
datasource, and about:config display would be done in an outliner / tree widget,
with editable content. ;-)
Seriously though, what would about:config done in JavaScript buy you?
Comment 27•24 years ago
|
||
consistency for one...
Comment 28•24 years ago
|
||
not just consistency, but lower maintenance costs - take about:plugins as an
example.. it's very easy to tweak that. Rule of thumb is if you're dealing with
UI, it's a lot easier to use JS. (there are exceptions to every rule, blah blah..)
Comment 29•24 years ago
|
||
Does this comment in bug 57969 still apply: "- there's no clean way to enumerate
prefs from JS. talked with shaver about this briefly over IRC - I favored fixing
CreateChildList() to be script-friendly".
Brian, anything the new prefs code brought in that would make doing about:config
in JS easy?
Comment 30•24 years ago
|
||
GetChildList is the "script-friendly" version of CreateChildList. Offhand I
can't think of any reason why this couldn't be done in JavaScript...
Comment 31•24 years ago
|
||
One data point: bringing up about:config done in JavaScript: 136 secs. C: 12 secs.
Comment 32•24 years ago
|
||
I'd like to see the javascript implementation. we should be able to implement
this in no more than 2x the c++ speed (and I would say, perhaps even as little
as 1.1x)
Comment 33•24 years ago
|
||
Comment 34•24 years ago
|
||
Comment 35•24 years ago
|
||
ah ha! See, the real difference between these two is that the JS is generating a
giant table, and the C++ is generating a series of <font> tags...
Assignee | ||
Comment 36•24 years ago
|
||
running in debug mode on Win32
HTML file with tables 56.4 seconds
HTML file without tables 33.4 seconds
Native with tables 21.3 seconds
Native without tables 9.3 seconds
So is 56.4 seconds acceptable?
Comment 37•24 years ago
|
||
I don't think so.
Comment 38•24 years ago
|
||
no of course not, but we're getting there. :)
Now, try using CSS and a series of <div> tags, all with the same class
attribute.. that will allow the page to share style information...
Assignee | ||
Comment 39•24 years ago
|
||
Assignee | ||
Comment 40•24 years ago
|
||
After some testing with the above page (and the RTM candidate for 6.1)... I feel
tables are better than CSS formating.
The html file has both sets of code in it.. and can be easily switched from
using tables (current format) to using css code.
with tables (and limiting the number of prefs to 400 - rather than the entire
list) I was able to bring the page up in less than 10 seconds (multiple times).
However, with the css code implementation the same number of prefs take over 170
seconds .... and occasionally taking over 200 seconds.
I'm not sure what this proves other than... tables are better than css at this
point for this implementation.
Comment 41•24 years ago
|
||
well, your css-based view is all crazy - each row has the same ID - you
shouldn't be using ID here, you should be using class. In a given document,
there should only be one element with a given id, not 400!
... it MIGHT be why the CSS one is so slow..(might not, but you never know)
Assignee | ||
Comment 42•24 years ago
|
||
Ok.... changed the ID's to classes and ran it with all prefs (not just 400).
The time just keeps getting slower and slower.
to load 884 prefs (that's 100% of the hash table)
it took 6130.8 seconds (or 102 minutes)
Tables are faster:
27 seconds
Comment 43•24 years ago
|
||
an hour and 42 minutes? something else is clearly wrong.
Assignee | ||
Comment 44•23 years ago
|
||
After some exhausting testing here is what I've found:
Mozilla - # of prefs 100 300 500 900
Internal about:config
builds HTML file
using table 2.25 5.38 10.33 18.67
using CSS 3.75 12.09 33.41 67.43
C++ code
using table 1.67 4.2 8.71 14.95
using CSS 3.24 9.76 27.65 57.55
External HTML page
using table 2.38 5.88 10.67 19.01
using CSS 2.94 8.69 23.65 51.46
I found it odd that CSS would actually run faster using an external HTML page
than it would via a C++ implementation - however, tables are much faster across
the board.
The difference between the tables internal and via the C++ code is insignificant
(IMHO)... vs the advantages of having an HTML file that can be easily edited.
Therefore, I'm going to submit a patch based on the HTML files - formatted via
tables for review.
Assignee | ||
Comment 45•23 years ago
|
||
Assignee | ||
Comment 46•23 years ago
|
||
Comment 47•23 years ago
|
||
What about intl?
hardcodet languages are never good or dont we care for these files?
My other comments:
<script> should be <script type="application/x-javascript"> in the first
config.html
- A lot of blank lines in both files. please delete those.
- Insert a new line after strict.dtd">
- You're missing some " in the var prefWindow line around width=600, height=400
- Make sure to check for javascript strict warnings!
- Make correct indents in the style section. Some are idented some not.
- Indents should be either TABS or SPACES. not both.
Please add the following line to your prefs.js file, so we could avoid all the
strict warning fixup...:
user_pref("javascript.options.strict", true);
Comment 48•23 years ago
|
||
sorry for coming in at the last minute.
how about this design to speed up about:config
use an outliner.
you can implement your own nsIOutlinerView in javascript.
on load of config.xul (or what ever), you enumerate all the prefs names into a
array. config.css has the color rules (see threadpane.css for how we did
priority color in the thread pane). example, all outliner text cells
with "locked" are red.
GetRowCount() returns the array len.
GetCellText(col,row) for col[0] == 'n' (for nameCol) returns prefarr[row]
GetCellText(col,row) for col[0] == 'v' (for valCol) get's the value of the pref
named prefarr[row]
GetCellProperties(row), get the type of prefarr[row] appends the atom
for "locked".
etc.
if doing it all from js is too slow, you can implement the nsIOutlinerView from
C++.
I even think outliner has support for in-place editing, so later on you could
add in place editing of prefs (like all the hidden prefs we tell users to edit
by hand)
comments?
Comment 49•23 years ago
|
||
if we do this, either we do it outside of libpref, or we do it with javascript.
libpref should not require XUL...
Assignee | ||
Comment 50•23 years ago
|
||
I was already going to implement this in an html file (via js)... but the XUL
has definate possibilities.
Assignee | ||
Comment 51•23 years ago
|
||
Assignee | ||
Comment 52•23 years ago
|
||
Assignee | ||
Comment 53•23 years ago
|
||
Assignee | ||
Comment 54•23 years ago
|
||
Assignee | ||
Comment 55•23 years ago
|
||
Comment 56•23 years ago
|
||
try using NS_GENERIC_FACTORY_CONSTRUCTOR to make your Create() routine - saves
you the trouble of hand-writing it..
Comment 57•23 years ago
|
||
oh by the way - you don't have to put your about handler in necko... you can put
it in any dll you want (such as preferences)
Comment 58•23 years ago
|
||
Here are my comments about the last attachments:
What about intl? hardcoded text are never good or dont we care for these files?
"var prefURL" should be "var prefURL". delete one space..
You use:
window.open(prefURL, "preferences",winChrome ,width=600, height=400);
shouldn't width=600, height=400 be in "
ala:
window.open('/help2/help_index.html', 'remote', 'width=600,height=400')
Warning: assignment to undeclared variable i
Source File: file:///c:/temp/c.html
Line: 61
Please add the following line to your prefs.js file, so we could avoid all the
strict warning fixup...:
user_pref("javascript.options.strict", true);
Comment 59•23 years ago
|
||
Unfortunately this works very poorly on the Macintosh...
First it pops open the "Please wait" window, but never draws it. After a really
long pause (74 sec. on my machine) it finally draws the window (which I now have
to close to see the config display in the other window.)
Second, once the config page is created, the machine is stuck in some kind of a
loop where approximately every second it flips to the watch cursor and the
machine becomes totally unresponsive... almost like it's constantly re-laying
out the page or something. Closing the page is the only way to fix this.
The first problem is probably related to the crappy thead support in the MacOS.
the second... who knows. Assuming that these are Mac only issues (they seem to
be), we need to insure that these are actually fixable problems, and then file
the appropriate bugs as blocking this one.
Comment 60•23 years ago
|
||
the current fix is not the right approach.
to solve the performance problems, use the outliner.
it is perfect for this.
Assignee | ||
Comment 61•23 years ago
|
||
Assignee | ||
Comment 62•23 years ago
|
||
Assignee | ||
Comment 63•23 years ago
|
||
Assignee | ||
Comment 64•23 years ago
|
||
Assignee | ||
Comment 65•23 years ago
|
||
Assignee | ||
Comment 66•23 years ago
|
||
Assignee | ||
Comment 67•23 years ago
|
||
Assignee | ||
Comment 68•23 years ago
|
||
Comment 69•23 years ago
|
||
I don't feel comfortable giving an r, since I'm not an expert in this area.
I've got a comment and question.
Formatting: the tabs are kinda goofy. If I had to guess, I would say you were
using the editor in VC++. The modeline says tab-width should be 4 spaces.
For example:
getCellText : function(k, col)
{
if ( !arr[k] )
return;
else
return arr[k][col];
},
code in the onConfigLoad function has the same issues.
It looks like the code only displays a maximum of 100 prefs. How does it
display more than the initial 100?
Assignee | ||
Comment 70•23 years ago
|
||
I'll go back thru config.js to make sure the tabs are set to 4.
actually, prefCount.value is initially set to 100... but gets reset a few lines
later:
var prefArray = prefBranch.getChildList("" , prefCount);
Assignee | ||
Comment 71•23 years ago
|
||
Comment 72•23 years ago
|
||
Please turn on strict javascript warnings via Edit -> Prefs -> Debug and check
the files! I dont want to file bugs on the javascript warnings if these files
get checked in!
Assignee | ||
Comment 73•23 years ago
|
||
strict warnings are turned on... I'm still missing something.
I am getting no warnings related to my code. There are no warnings in the
javascript window, and the ones I see in the debugging window aren't related to
code I'm submitting.
What are the errors you are seeing? (I have tested this on both Win & Linux)
Assignee | ||
Updated•23 years ago
|
Whiteboard: requesting r & sr
Assignee | ||
Comment 74•23 years ago
|
||
Comment 75•23 years ago
|
||
getCellText : function(k, col)
{
if ( !arr[k] )
return;
else
return arr[k][col];
},
else after return is a non-sequitur and a crutch, don't use it.
How come the Emacs modeline comment on line 1 and this function use 4-space
indentation, while the function at the bottom uses 2-space units of indentation?
/be
Comment 76•23 years ago
|
||
I noticed that in the most recent attachment (08/17/01 17:13) you removed the
enablePrivilege calls from the beginning of onConfigLoad(). This is good, those
calls shouldn't be necessary.
There's a security concern here which has bitten us before with other about:
pages. If an attacker can insert script code into a pref value somehow (for
example, if the user visits a page with URL
http://www.attacker.com/index.html?<SCRIPT>some JS code here</SCRIPT> and that
URL gets set as the value of "browser.last_page_visited" or whatever that pref
is, and the user then visits about:config, the JS code from the URL will be run
with full privilege.
Currently, a webpage should not be allowed to load or link to about:config, so
unless an attacker takes advantage of another exploit, this is only a problem if
the user explicitly visits about:config after the attacker has inserted a script
into prefs. So this isn't much of an exploit on its own, but it ups the ante on
other exploits.
To make this safer, we should "escape" all prefs data written to the
about:config page. Basically, this means replacing < with < and > with >,
that should do the trick.
Comment 77•23 years ago
|
||
Chip,
There are potentially '<' and '>' in the pref values that your JS code is
writing to the page, as in the example I mentioned. We need to be sure that the
prefs data we're writing to the page gets written as text, not as "live" HTML.
Basically, after you set prefValue in onConfigLoad(), you should process it like
this:
function htmlEscape(s) {
s = s.replace(/\&/g, "&");
s = s.replace(/\>/g, ">");
s = s.replace(/\</g, "<");
return s;
}
r=mstoltz on config.js with the above change. I don't know XUL or CSS too well,
so someone else should review those.
Comment 78•23 years ago
|
||
I haven't looked at the code, but a couple questions--are there other characters
besides < and > that will cause similar problems? For instance, what if the &
character occurs in the prefs?
If so, does Mozilla already have some more comprehensive HTML escaping code that
could be reused here to cover most or all of the necessary cases?
Comment 80•23 years ago
|
||
r/sr=darin on the netwerk changes
Assignee | ||
Comment 81•23 years ago
|
||
Comment 82•23 years ago
|
||
You don't have to add chrome files to MANIFEST and makefiles anymore, just the
jar.mn.
Also, in config.css you have some rules that reference outliner-bodybox, but
this is a class, and so you need to prepend a period, like so:
outliner:focus > .outliner-bodybox
Assignee | ||
Comment 83•23 years ago
|
||
Comment 84•23 years ago
|
||
Chip, I would recommend one change.
instead of
+ if(prefBranch.prefHasUserValue(prefArray[i]) != 0) // 0 = false
+ prefLockState = "user set";
+ if(prefBranch.prefIsLocked(prefArray[i]) != 0) // 0 = false
+ prefLockState = "locked";
you can do
if(prefBranch.prefIsLocked(prefArray[i]) != 0)
prefLockState = "locked";
else if(prefBranch.prefHasUserValue(prefArray[i]) != 0)
prefLockState = "user set";
with that change r=srilatha on the js file.
Comment 85•23 years ago
|
||
Sorry to barge in. There's no need to note that `0 == false', that's a common
idiom. In fact, this would probably read more easily if you left out the |!= 0|
altogether, and just had
if(prefBranch.prefIsLocked(prefArray[i]))
prefLockState = "locked";
else if(prefBranch.prefHasUserValue(prefArray[i]))
prefLockState = "user set";
Just my $0.02. Haven't looked at the rest of the patch.
Comment 86•23 years ago
|
||
[s]r=hewitt on the outliner stuff
Comment 87•23 years ago
|
||
can you attach a final patch?
Comment 88•23 years ago
|
||
Looks good. sr=waterson. I suspect the first RFE we're going to get on this will
be for the ability to print this... ;-)
Assignee | ||
Comment 89•23 years ago
|
||
Comment 90•23 years ago
|
||
Looks ok for 0.9.4, sorry I didn't approve earlier. A couple of nits that I
hope you'll pick; feel free to post a really-final patch. a=brendan@mozilla.org
in any event.
/be
1.
+ k = (i + 1 - j); // avoid numbering "capability" prefs
and later
+ arr[k-1] = {indexCol:prefIndex, prefCol:prefArray[i],
lockCol:prefLockState, typeCol:prefTypeName, valueCol:prefValue};
in the same loop does extra operations needlessly (i >= j so i-j >= 0 => k > 0,
so there's no need to bias k by 1). When you finally do want k to be one more
than its maximum value as an index into arr[], just add one there:
+ view.rowCount = k + 1;
2.
+ prefType = prefBranch.getPrefType(prefArray[i]);
+ if(prefType == 32)
+ {
+ prefTypeName = "string";
+ prefValue = prefBranch.getCharPref(prefArray[i]);
+ prefValue = htmlEscape(prefValue);
+ }
+ else if (prefType == 64)
+ {
+ prefTypeName = "int";
+ prefValue = prefBranch.getIntPref(prefArray[i]);
+ }
+ else if (prefType == 128)
+ {
Shouldn't you use the PREF_BOOL, etc. manifest constants here? Also, consider
using a switch statement.
/be
Comment 91•23 years ago
|
||
This is crashing for me if I use about:config the second time.
1. Type about:config
2. Load another page
3. type about:config again -> crash
win2k build 20010901.. (CVS opt)
Comment 92•23 years ago
|
||
Chip, why were all of the new files NPL'd?
Comment 93•23 years ago
|
||
Verified Matti's crash (on Mac). Opened as bug 98207.
Assignee | ||
Comment 94•23 years ago
|
||
The bits were checked in Friday at 11pm by Brian Nesse (thanks Brian!)
Changes to "var k" and putting in a switch (rather than else if) were part of
this fix - however, I could not figure out how to get javascript to understand
the defines in nsPrefBranch for PREF_BOOL, PREF_INT & PREF_CHAR
As to the NPL question - I used the same "license" header as found in all the
other about: functions.
While there is a new bug 98207 as a result of this "fix" - it is possibly
related to either JS or XUL so I am marking this fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 95•23 years ago
|
||
you probably wanted to use
Components.interfaces.nsIPrefBranch.PREF_INVALID
or, easier:
const nsIPrefBranch = Components.interfaces.nsIPrefBranch
and then use
nsIPrefBranch.PREF_INVALID
nsIPrefBranch.PREF_INT
and so forth.
Comment 96•23 years ago
|
||
For future reference, mozilla.org will be providing a "what license to use on
new files" doc soon, and the license will be a dual MPL/LGPL whose boilerplate
will be published shortly. Anyway, stay tuned. For now, using NPL if you are a
contributor@netscape.com is ok.
/be
Comment 97•23 years ago
|
||
Filed bug 98667 -- this is not working on Linux (chokes on a particular linux
pref name).
Comment 98•23 years ago
|
||
> I even think outliner has support for in-place editing, so later on you could
> add in place editing of prefs (like all the hidden prefs we tell users to edit
> by hand)
UI for editing arbitary prefs is bug 17199.
Comment 99•23 years ago
|
||
now implemented; vrfy fixed with 2001.09.04 bits.
Status: RESOLVED → VERIFIED
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•