further cleanup for pref backend

VERIFIED FIXED in mozilla1.6alpha

Status

()

--
minor
VERIFIED FIXED
15 years ago
3 years ago

People

(Reporter: darin.moz, Assigned: darin.moz)

Tracking

Trunk
mozilla1.6alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

15 years ago
from alfred kayser in bug 98533:

>------- Additional Comment #33 From Alfred Kayser  2003-09-17 00:12 -------
>
>Nice fix! These things makes Mozilla much better, as a whole.
>
>It could be wise however to open a new bug, to implement:
>+  // XXX maybe we should read the file in chunks instead??
>
>Also: may be best to let the parser handle this, by just ignoring (or stopping
>at) an EOF:
>  #ifdef XP_OS2 /* OS/2 workaround - our system editor adds an EOF character */
>      if (readBuf[amtRead - 1] == 0x1A) {
>         amtRead--;
>      }
>
>  #endif
>
>Also: why not remove the 'PRBool bCallbacks' param of
>'PREF_EvaluateConfigScript' (it seems always to be 'PR_TRUE').
>+    NS_ASSERTION(bCallbacks, "no support for disabling callbacks");
>
>Furthermore the 'leafName' thing seems also not needed anymore. At least
>PREF_EvaluateConfigScript doesn't seem to use it.
(Assignee)

Updated

15 years ago
Severity: normal → minor
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.6alpha

Comment 1

15 years ago
darin, I'm planning on touching this code soon, to read prefs from multiple
locations from a category (so that default prefs can be included in the GRE).
What timeframe is this bug?

--BDS
(Assignee)

Comment 2

15 years ago
bds: not sure i'll get to this anytime soon... hopefully during 1.6 alpha, but
not guaranteed... though alfred might have a patch brewing! ;-)

Comment 3

15 years ago
Created attachment 131754 [details] [diff] [review]
Patch to be tested

This is an initial patch.

Changes:
Removed all unused params of openPrefFile.
Set member flags mErrorOpeningUserPrefs and mErrorOpeningSharedUserPrefs
using return value, instead of complicated boolean params.
In openPrefFile, read the pref file per blocks of 4096 byes, just like in the
testcode of prefread.cpp.
Removed globals gErrorOpeningUserPrefs and gSavedLine from prefapi.cpp.
Removed PREF_EvaluateConfigScript as it is not used anymore.
For safety (on OS/2) also check for EOF (0x1A) at 'end-of-line'.

Comment 4

15 years ago
Question: the prefread.cpp parses uses a buffer, which grows 
when needed (on demand!), meaning re-alloc's. Each file parsed
will still thus result in one or more alloc for the resizable buffer.
(on average two (re)alloc's per file parsed). 
While we are removing the large malloc for the whole, would it not
be an option to change this to a fixed (but reasonable size) as names and
values in a preferences value should not exceed any reasonable limits anyway.
Better to warn and ignore too long values or names, and keep on allocating 
memory for it. What if someone really messes up his preferences file?

So, in summary why not replace it with a fixed buffer, as member of the
ParserState struct. Less allocs, and less risk in leaks, and be strict on max
lengths.

However, there is something to say of being flexible, and to allow any length.

Comment 5

15 years ago
Created attachment 131897 [details] [diff] [review]
Newer version, more cleanup

More cleanup:
* Use NS_* errorcodes, instead of PREF_*
* Merged Set*Pref functions with SetDefault*Pref
* Replaced 'PrefAction' with PRBool, as it only indicated Default of User
* Updates some function descriptions in the headers
* Init gCallbacksEnabled to TRUE, as it is immediately set to TRUE, during load

    (see bug 219711: when is best time to enable callbacks?)
Attachment #131754 - Attachment is obsolete: true
(Assignee)

Comment 6

15 years ago
Comment on attachment 131897 [details] [diff] [review]
Newer version, more cleanup

thanks for the patch alfred!  i'll try to review this shortly.
Attachment #131897 - Flags: review?(darin)
(Assignee)

Comment 7

15 years ago
Created attachment 131926 [details] [diff] [review]
slightly revised version

fixed a few nit-picks.	otherwise, this looks really great.  nice little bit of
codesighs savings to boot ;-)
Attachment #131897 - Attachment is obsolete: true
(Assignee)

Comment 8

15 years ago
Comment on attachment 131926 [details] [diff] [review]
slightly revised version

let's get alecf's review of this...
Attachment #131926 - Flags: superreview?(alecf)
Attachment #131926 - Flags: review+
(Assignee)

Updated

15 years ago
Attachment #131897 - Flags: review?(darin)
(Assignee)

Comment 9

15 years ago
alfred: i agree that simply enabling prefs callbacks from the start is probably
the right thing.  i'm not sure what the origins are of that variable, but it
seems like it is appropriate to simply enable it by default.  with this patch,
gCallbacksEnabled is essentially fixed at PR_TRUE, so we might actually want to
remove it altogether.  *shrug* ... maybe save that for another time.  not sure
if we will one day actually want to bring back the functionality of temporarily
disabling callbacks.

Comment 10

15 years ago
Comment on attachment 131926 [details] [diff] [review]
slightly revised version

oh this is so great. I'm so glad to see that stupid _convertRes killed.. its
been on my list for years!

nice cleanup on the getPrefName() - I still wonder if there's a cleaner way to
do that.

sr=alecf
Attachment #131926 - Flags: superreview?(alecf) → superreview+
(Assignee)

Comment 11

15 years ago
fixed-on-trunk

thanks alfred!!
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Verified using LXR
Status: RESOLVED → VERIFIED

Comment 13

11 years ago
> with this patch, gCallbacksEnabled is essentially fixed at PR_TRUE, so we might
> actually want to remove it altogether.

Filed bug 414551 about that.

Updated

3 years ago
See Also: → bug 173451
You need to log in before you can comment on or make changes to this bug.