219479 - further cleanup for pref backend

Status: VERIFIED FIXED

(Core :: Preferences: Backend, defect)

Description

[Darin Fisher]

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.

Comment 1

[Benjamin Smedberg]

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

Comment 2

[Darin Fisher]

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

[Alfred Kayser]

Attachment: 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

[Alfred Kayser]

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

[Alfred Kayser]

Attachment: 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?)

Comment 6

[Darin Fisher]

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

thanks for the patch alfred!  i'll try to review this shortly.

Comment 7

[Darin Fisher]

Attachment: slightly revised version

fixed a few nit-picks.	otherwise, this looks really great.  nice little bit of
codesighs savings to boot ;-)

Comment 8

[Darin Fisher]

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

let's get alecf's review of this...

Comment 9

[Darin Fisher]

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

[Alec Flett]

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

Comment 11

[Darin Fisher]

fixed-on-trunk

thanks alfred!!

Comment 12

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified using LXR

Comment 13

[Nickolay_Ponomarev]

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

Filed bug 414551 about that.