Closed
Bug 70348
Opened 24 years ago
Closed 23 years ago
Implement an AutoConfig Feature
Categories
(Core :: Preferences: Backend, defect, P2)
Core
Preferences: Backend
Tracking
()
VERIFIED
FIXED
mozilla0.9.2
People
(Reporter: mitesh, Assigned: mitesh)
References
Details
(Whiteboard: Checked in)
Attachments
(30 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
AutoConfig downloads the preferences from a http:// or file:// URL and helps
system administrator to keep the enterprise specific preferences at a central
location.
Assignee | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Summary: Implement an AutoConfig Feature → Implement an AutoConfig Feature
Assignee | ||
Comment 2•24 years ago
|
||
I think PAC is for AutoProxy configuration. This is a facility to system
administrator to put the prefernces specific to the enterprise on a central
location and enforce it through netscape.cfg file.
The autoconfig file simple text JS file.
Assignee | ||
Comment 3•24 years ago
|
||
Assignee | ||
Comment 4•24 years ago
|
||
Here with I am attaching code diffs for initial review. Please send me your
feedback.
There is only one file I changed: modules/libpref/src/nsPref.cpp
I have added two methods: dowloadAutoCfgFile() and readOfflineFailoverJSC() to
nsPref class. Also nsPref is now also inheriting from nsITimerCallBack and
nsIStreamListener.
yes, proxy autoconfig is a js file, just like this thing which appears to be
browser autoconfig. it looks like you're duplicating code. please examine the
pac support in bug 53080.
also, your style is switching between 2 and 4 space indentation, please pick
one.
NS_ERROR("Only http:/ or file:/ URLs are vaild for autoconfig");
that's nice, is there a reason? what's wrong w/ ftp, gopher, https, imap, or
news? [for that matter, do you really object to someone using data, javascript,
resource, chrome, or about?].
Assignee | ||
Comment 6•24 years ago
|
||
Assignee | ||
Comment 7•24 years ago
|
||
The new patch file is created with 'cvs diff -u' for unified context diff.
Comment 8•24 years ago
|
||
mitesh, please coordinate with bnesse, who is in the process of revamping the
prefs interface.
It seems like PAC is more of a necko thing, no?
Assignee | ||
Comment 9•24 years ago
|
||
In the next revision, I will stick to one coding style.
Before started working on AutoConfig Module, I looked into PAC support for
AutoProxyConfig and found it different. I will again look into that bug and see
if we can leverage the code from there.
Regarding ftp://, telnet:// and other URLs, we are actually working on the
feature what 4.x enterprise client supports. AutoConfig documentation says that
it doesn't support other URLs than http:// and file://. Link to the
documentation:
http://blues/users/chipc/publish/MCD_DOC/dg/dgp14.html#1025739
Comment 10•24 years ago
|
||
comments on the patch as it stands now (I still think this belongs somewhere in
necko or something, I don't think nsPref should be burdened with this)
- pref_Alert is going away, not to mention that backend code should not have any
XUL-oriented frontend dependancies. people might want to use libpref in an
environment that does not have a frontend
- Why the JS_BeingRequest/JS_EndRequest? We tried to rip these OUT of nsPref a
while back (and now actually I see that some of the lock file stuff has it..
doh!) - remove them, they are not necessary.
- even if we were to be using FE code, why are there non-localizable strings in
this code? (like JS_BeginRequest/EndRequest, the presence of existing
non-localizable strings does not justify adding new ones)
- even if we were to be popping up dialogs from JS, we do not parent them off of
the hidden window.. yes, there are places in the code that do that, but they are
broken right now.
- the user's e-mail is not stored in mail.identity.usermail, it's stored using
some mailnews stuff. However, nobody outside of the mozilla/mailnews directory
should be depending on any mail code. I'm not sure about how to resolve that
dependancy just yet.
- in the gigantic function readOfflineFailoverJSC, you have an nsXPIDLCString
that is used for nothing other than copying the static string "failover.jsc" -
you should either just use the static string "failover.js", #define it somewhere
in the file, or use nsLiteralCString("failover.js") if you need a
nsAReadableString (which you don't seem to)
- Why are you appending "Essential Files" to the filename for mac? Are you sure
there isn't a pre-defined way of getting to that directory via
NS_GetSpecialDirectory (especially so we don't have to have XP_MAC code in this
mostly XP file!)
- you defined "aFile" as a local variable - the "a" prefix in a variable name
refers to the variable being an argument to the current function - which this is
not.
these are fairly minor nits though
The biggest issue I see right now is that this is rolled into nsPref. I guess
what I'd really like to see is this put into a totally seperate component, and
then fix up the nsIPref/nsIPrefService interface to be able to read vendor and
PAC files that are given to it, either as a buffer or a stream of some kind.
this component could live in the pref dll if it can't live in necko, but it
belongs in a seperate object.
Comment 11•24 years ago
|
||
Is that internal link copywritten in any way? if not, we should post it to
www.mozilla.org, it seems to have some good design info
Assignee | ||
Comment 12•24 years ago
|
||
Thanks Alecf for the feedback. I will work with Brian to find out a better place
for AutoConfig Module if nsPref.cpp is not the right one. I will correct the
other issues in the next revision.
As you mention about not having any XUL or frontend code in libpref, so how
should I go by if I want to bring up a confirm dialog box?
I don't think you want to be ripping out JS_BeginRequest/JS_EndRequest,
actually. I just got assigned 39373 (JS on threads other than "main" must run
within requests), which has the opposite intent.
Comment 14•24 years ago
|
||
well, talk to brendan - he ripped them all out months ago
Comment 15•24 years ago
|
||
I prolly ripped those out well before fixing bug 54743. Here's the current deal
on requests:
- If all JS objects that might be handled by a script are single-threaded,
accessible only by one thread, *and* that thread is the UI/main thread where GC
must run (because of single-threaded DOM finalizers), then you do not need
requests around batches of JS API and non-blocking native code.
- If your thread might create an object that is accessible by other threads, you
need requests.
- If your thread might use an object that was created by another thread, then
you need requests.
So, does any other thread use pref JS objects? If so, we need requests (again).
Sorry for ripping them out before -- then, without bug 54743's fix, the only
reason to have them was to keep the GC at bay, and it seemed like the prefs code
was running on the same thread as the GC, viz, the main thread. So there was no
point in them.
/be
Comment 16•24 years ago
|
||
right. All pref JS objects are completely private to libpref, and all of libpref
works only on the main thread.
Assignee | ||
Comment 17•24 years ago
|
||
Here is the new set of code changes. I have created a separate component under
Preferences module.
- Removed frontend related code.
- Removed JS_BeginRequest and JS_EndRequest code. (as per the last discussion,
it is no longer needed since all JS objects are private to libpref)
- Removed the mailnews dependency.
- There doesn't seem to be a solution to "Essential Files" folder yet so keeping
it as it is.
The component contains three files:
nsIAutoConfig.idl, nsAutoConfig.h, nsAutoConfig.cpp
and changes made to nsPref.cpp and other build related files.
Assignee | ||
Comment 18•24 years ago
|
||
Assignee | ||
Comment 19•24 years ago
|
||
Assignee | ||
Comment 20•24 years ago
|
||
Assignee | ||
Comment 21•24 years ago
|
||
Comment 22•24 years ago
|
||
looking much better! a few issues:
- use nsCOMPtr and do_CreateInstance, don't ever use
nsComponentManager::CreateInstance except under special circumstances
- you should never EVER use NS_DEFINE_xID() to define an IID.. even if you were
using nsComponentManager::CreateInstance(), you should be using NS_GET_IID. What
ancient code are you cutting and pasting this from, so I can go whack it with a
big stick?
- there is something really wierd going on with your cvs diff - it's missing
line numbers, so I can't tell where you're doing the autoconfig download stuff
- can we try to keep the autoconfig stuff as isolated from prefs as possible?
define the generic factory constructor in nsAutoConfig.cpp
- I'm not a huge fan of the "backdoor" of PREF_EvaluateConfigScript but I think
it's good for now - my hope is that we'll eventually switch away from using JS
to read in the user's prefs, but we'll use JS for things like the autoconfig
scripts... so it would be nice if nsAutoConfig were the place where all the JS
stuff lived, and nsPref just dealt with straight preferences, no JS. but THAT
issue is blue sky for now and shouldn't affect this patch.
Assignee | ||
Comment 23•24 years ago
|
||
Alecf,
Thanks for the feedback.
- As you can see this is the first time I am playing with mozilla code so by
mistake I have copied from the old code. I think it's not even in the current
tree. But mozilla lxr's search returned it for some search. I will change it to
use do_CreateInstance and nsCOMPtr<>
- Regarding CVS diff, I have applied this changes on top of Chip's changes and
Chip's changes are not checked in the tree yet. (Bug 5132) The AutoConfig stuff
is actually in method: ReadLockPrefs (Which is not in the tree yet) Once those
changes are in the tree, diffs will be more clear.
- If I define the generic factory constructor in nsAutoConfig.cpp, nsPref.cpp
complains abt missing identifier 'nsAutoConfigConstructor' during the components
registration. How can I solve that issue? Can we just declare the constructor in
the nsAutoConfig.h?
There are still issues with user's email address and frontend dialog boxes but
for now I have removed from this patch untill we resolve them.
Assignee | ||
Comment 24•24 years ago
|
||
Comment 25•24 years ago
|
||
don't worry about the offline icon.
I will be handling that automatically as a part of bug 65704
Assignee | ||
Comment 26•24 years ago
|
||
In addition to the changes to the utilityOverlay.js file, there is some
additional code to nsAutoConfig.cpp:readOfflineFailoverJSC() function:
+ // lock the "network.online" prference so user cannot toggle back to
+ // online mode.
+ rv = prefs->SetBoolPref("network.online", PR_FALSE);
+ if (NS_FAILED(rv)) return NS_ERROR_FAILURE;
+ pref_LockPref("network.online");
+
The file is not yet checked in to CVS so this changes are in addition to the
previous attachments.
Basically, it locks the pref "network.online" and utilityOverlay.js changes will
disable the network icon and menu item (under File) if the pref is locked.
Assignee | ||
Comment 27•24 years ago
|
||
I think this will changes will be in addition to the changes you will be making
for bug 65704
Comment 28•24 years ago
|
||
oh, I see what you're saying. you're right. Thought at a later point I will move
your code into the XBL widget that I'm creating.
Assignee | ||
Comment 29•24 years ago
|
||
Attaching latest set of diffs, requesting code review
- These changes are on top of Chipc's changes for bug 5132
- The creation of nsAutoConfig object has been moved nsApprunner.cpp along with
netscape.cfg reading.
- For the unique identity problem, first we are using the email address if it is
available but if this is a client without messenger and mail address is not
available, we are using the profile name as an unique identifier. This needs to
be documented for IS people.
Thanks.
Assignee | ||
Comment 30•24 years ago
|
||
Assignee | ||
Comment 31•24 years ago
|
||
Comment 32•24 years ago
|
||
nope! there should be no reason to touch nsAppRunner.cpp.
you should add yourself to the "app-startup" category, which will make you get
called at startup...
http://lxr.mozilla.org/seamonkey/search?string=APPSTARTUP_CATEGORY
look at the xml extras example.
Comment 33•24 years ago
|
||
The other problem is that that you're using the 4.x pref for the user's email
address... see:
http://lxr.mozilla.org/seamonkey/search?string=mail.identity.useremail
Note that it is not used anywhere in the tree except to migrate from 4.x to mozilla!
I can think of two possible solutions to this problem:
1) come up with a mail-independant way of determining the user's email address
(because the tree should compile with or without entering the mailnews
directory) - one possibility is to go through the nsIUserInfo service:
http://lxr.mozilla.org/seamonkey/source/xpfe/appshell/public/nsIUserInfo.idl
this will not use the user's preferences though, but will instead go right
through to the respective system's information about the user.
2) get the mail client to set the old 4.x pref whenever the default account is
set - i.e. in nsMsgAccountManager::SetDefaultAccount(), make it so that we set
mail.identity.useremail from the identity that's in that account.
Assignee | ||
Comment 34•24 years ago
|
||
It seems that nsUserInfo::GetEmailAddress is not implemented for WIN32 platform
and on Unix it is created with username@domainname. I think this won't be the
same email address as mainews will be using. In that scenario setting the
preference through mailnews would be a better soltuion.
Comment 35•24 years ago
|
||
assigning QA to Luke for eClient purposes, adding sarah to Cc.
QA Contact: sairuh → lrg
Assignee | ||
Comment 36•24 years ago
|
||
I have added nsAutoConfig to the APP_STARTUP category and it works fine but
there are two problems:
- nsAutoConfig has a dependency on netscape.cfg bug 5132. Actually the value of
the autoconfig URL is set in netscape.cfg so AutoConfig has to be called after
netscape.cfg read.
- nsAutoConfig is also using a current profile name in case where there is no
messenger. So it has to be called after the current profile name is set.
is there a way that we can call the method from AutoConfig module later on even
if we instatiate it with the app-startup category?
Assignee | ||
Comment 37•24 years ago
|
||
Posting latest set of diffs including following changes:
- Email address is now retrived from the set of perferences (by first finding
the default account and then using it's id)
- Mac related change for the filesize in function evaluateLocalFile(). It's 64
bit nunber and I was just casting it to unsigned int. In fact, it's a structure
so using a macro to retrive the 32 bit value.
- The initialization is still under nsAppRunner.cpp, there is no better place.
app-startup category doesn't work, since autoconfig has to be after netscape.cfg
and profile name selection.
(changes are on top of the changes from bug 5132 as before)
Requesting r and sr,
Thanks,
Comment 38•24 years ago
|
||
mitesh: I still object to adding this to nsAppRunner.cpp, so I'm going to
suggest the following:
- use the app-startup mechanism to initialize your object
- when your object is initialized, add yourself as an observer to the
profile-startup (I don't remember the actual topic name) topic.. from there you
can do stuff like block a profile from starting, etc.
Assignee | ||
Comment 39•24 years ago
|
||
Assignee | ||
Comment 40•24 years ago
|
||
Comment 41•24 years ago
|
||
you can make diffs of new files.
just
cvs add <filename>
and then it will be included in your diff if you say
cvs diff -u -N <directory or file>
Assignee | ||
Comment 42•24 years ago
|
||
looks like NS_PROFILE_STARTUP_CATEGORY is the perfect place since the profile
name has been decided at that stage. I have added the nsAutoConfig in that
category. The only problem is that the code in nsProfile.cpp need some change or
I am missing something.
In following part of the code is not retriving the contractid correctly. It
picks up the second argument from the RegisterAutoConfig function. while it
needs to pick up the third argument. (second argument is the name of the module
while the third argument is the contract id string) I looked up the
APPSTARTUP_CATEGORY implementation and it does it differently. I am attaching
both code snippets. When I put the same contract id for second and third
argument to the Register function, the code worked fine. Also, attaching
ResgisterAutoConfig function.
Let me know what should we do.
Assignee | ||
Comment 43•24 years ago
|
||
Assignee | ||
Comment 44•24 years ago
|
||
Attaching latest set of diffs. (New files are included in the diffs now)
Changes:
- nsAutoConfig is instatiaed through APPSTARTUP category
- it is being called through observerservice with topic = "profile-after-change"
Assignee | ||
Comment 45•24 years ago
|
||
Assignee | ||
Comment 46•24 years ago
|
||
Latest set of diffs include following additions compare to the last time:
- It was supposed to write a cache copy of autoconfig.jsc to failover.jsc file.
I have added a method writeFailoverFile() to write the downloaded preferences to
failover.jsc file
- Removed checking for "autoadmin.failover_answer" preference in
readOfflineFailoverJSC() method. It was redundant. Created a documentation bug
75647 to document these changes.
Assignee | ||
Comment 47•24 years ago
|
||
Assignee | ||
Comment 48•24 years ago
|
||
Changing milestone to 0.9.1. Doesn't look like this will get r & sr by the
checkin deadline. Will try again after the tree reopens for 0.9.1 checkins.
Target Milestone: --- → mozilla0.9.1
Assignee | ||
Comment 49•24 years ago
|
||
Comment 50•24 years ago
|
||
talked with mitesh over IRC - basically I have issues with the protocol-specific
nature of this patch - it should not matter if we're using http:, file:, or
gopher: to retrieve the jsc file - we should be using the same technique to suck
down the data, and behaving the same, no matter what the error is. (i.e. no need
to check for http error values, etc)
Also, the more I've though about this, the more I've decided I don't like the
direct calls into the pref library using PREF_* and pref_* - if you cannot get
to these routines through the pref service interface and friends, then we need
to fix those interfaces, and coordinate with bnesse on that.
I could eventually see this object in a seperate DLL along with other eclient
features, and in that case we MUST call across interface boundaries..
Comment 51•24 years ago
|
||
I'm not sure I agree with Alec's assessment of this feature.
I consider this like the cfg file.
This has (or should have) the same functionality (and purpose) as the cfg file.
As such.... why shouldn't it be part of the preference network?
Comment 52•24 years ago
|
||
Well chip, you are right.. this is very much like the .cfg file stuff..and that
only further proves my point.
eventually I would like to see all JS-related configuration happening in an
entirely seperate object than prefs... many clients (i.e. embedding clients for
one) do not need e-client features like that.
Comment 53•24 years ago
|
||
Alec, while I do agree that this is the right approach in the long run, I'm not
sure if we'd want to hold off checking in any of the AutoConfig or netscape.cfg
for the new design.
Probably the best approach would be to have the current implementation land, get
it into the builds and general testing by QA, then open a separate bug for
redoing all JS prefs work, including core prefs.
The high level bug would be "Remove core preferences dependencies on
JavaScript", then have bugs filed for breaking out AutoConfig and netscape.cfg
as part of that work.
Comment 54•24 years ago
|
||
I never said anything about holding up this checkin for a design change...
what I did say is that I want them to use the nsIPref* interfaces instead of the
PREF_* calls.. that's just part of my review.
moving out the JS stuff is a whole different beast, I was explaining my
reasoning for requesting the removal of the PREF* right now.
So what is holding up this checkin right now is:
1) protocol-specific handling of the config files
2) use of PREF_*/pref_* calls
Assignee | ||
Comment 55•24 years ago
|
||
Latest set of diffs include following changes:
- Added unRegisterAutoConfig method in nsPrefsFactory.cpp
- Removed protocol specific downloading of autoconfig.jsc. AsyncOpen is now
being called on any URI and the result expected is javascript code. If the
result cannot be parsed correctly, the module go into failover mode.
- Added the event queue code in DownloadAutoCfg() to stall the thread till the
downloading is completed(success/failure).
- Added two methods getEmailAddr() and reportError() to the module for clarity.
The mLoaded needs to be set to true at all possible paths to avoid deadlock.
- pref_lock is now being called through nsPrefBranch::LockPref(). Still calling
PREF_EvaluateConfigFile() directly from prefapi.c. Talking with Brian, if we can
provide different interface.
Assignee | ||
Comment 56•24 years ago
|
||
Comment 57•24 years ago
|
||
+ rv = inStr->Read(buf, fs, &amt);
+ if (NS_FAILED(rv))
+ NS_ASSERTION((NS_BASE_STREAM_WOULD_BLOCK != rv),
+ "The stream should never block.");
+
I assume you want to bail out here too... maybe you meant
rv = inStr->Read(buf, fs, &amt);
NS_ASSERTION(rv != NS_BASE_STREAM_WOULD_BLOCK,
"The stream should never block.");
if (NS_FAILED(rv)) return rv;
Regarding the use of eventQueues, I think you need to make sure to
PopThreadEventQueue whenever you return, even on an error - otherwise you'll
stop processing events. I think the string bundle code has this same problem,
I'll have to fix that... sorry to direct you at a shabby example...
Also, please don't put "\n" in your assertions (more cut and paste from the
string bundles I see)
Here:
+ rv = prefBranch->GetCharPref("autoadmin.global_config_url",&t);
+ if(NS_FAILED(rv)) return NS_OK; //Return ok if there is no config url set.
+
+ // Converting the char * to nsCString class for efficient string
manuplation
+ if (t) {
+ cfgUrl = t;
+ PR_Free(t);
+ t = nsnull;
+ }
+ else return NS_OK; // There is no cfg URL set so no need to go further.
You're doing a lot more work than you need to. use nsXPIDLCString, and
getter_Copies(); - and what kind of a variable name is "t" - something more
descriptive, please :)
also, you have a giant
+ if (cfgUrl) {
+ ...
+ }
block followed by return NS_OK;
Normally, I would say turn this into
> if (cfgUrl.IsEmpty()) return NS_OK;
to avoid unnecessary indentation
but the thing is, you've already checked if the string has a value... so this
if() check will always succeed.
also, cfgUrl is a nsCString - seems like you should be able to use
nsLiteralString here, if you use nsXPIDLCString.
i.e.
> rv = prefBranch->GetCharPref("autoadmin.global_config_url",getter_Copies(t));
> if(NS_FAILED(rv)) return NS_OK; //Return ok if there is no config url set.
>
> nsLiteralCString cfgUrl(t);
emailAddr is a nsCStrings, it should be an nsCAutoString (or is it
nsAutoCString? I can't remember) - local variables should always use the "Auto"
variety.
in OnDataAvailable:
+ while (aLength) {
+
+ rv = aIStream->Read(buf, aLength, &amt);
+ if (NS_FAILED(rv)) {
+ NS_ASSERTION((NS_BASE_STREAM_WOULD_BLOCK != rv),
+ "The stream should never block.");
+ PR_FREEIF(buf);
+ return readOfflineFile();
+ }
do you really mean to read the offline file every time a read fails? seems like
you should record the error, and read the offline file after you've exited your
event loop... or maybe you're already doing that in OnStopRequest() - need to
straighten that out.
Also in OnDataAvailable, you're allocating a big buffer every time, and then
looping through calling Read() - why not use a fixed size buffer that's
allocated on the stack?
the use of mBytesRead - isn't this ALWAYS equivalent to mBuf.Length()?
this:
+ // Send the autoconfig.jsc to javascript engine.
+ if (PREF_EvaluateConfigScript(mBuf.get(),
+ mBytesRead,
+ nsnull,
+ PR_FALSE,
+ PR_TRUE,
+ PR_FALSE)) {
+ rv = writeFailoverFile(); // Write the autoconfig.jsc to failover.jsc
(cached copy)
+ if(NS_FAILED(rv))
+ NS_WARNING("Error writing failover.jsc file\n");
+ }
is confusing
instead of
> if (some_really_long_call(with,
> lots,
> of,
> parameters)) {
>
> }
use a local temporary variable - the compiler will optimize it out and it will
be easier to read:
> PRBool success = some_really_long_call(with, lots, of, parameters)
> if (success) {
>
> }
etc
Assignee | ||
Comment 58•24 years ago
|
||
- Is it ok to use a label and bunch of goto statements to make sure
PopThreadEventQueue being executed in all error cases?
- Regarding string manuplations, what would be the best way? I tried
nsLiteralCString but it doesn't allow Append on it. The need is to pass it as a
char* to the pref functions to get it initialized and should be able to append
otehr strings to it. nsXPIDLCString also doesn't allow append.
Other corrections will be in the next diffs.
Assignee | ||
Comment 59•24 years ago
|
||
new version of nsAutoConfig.cpp
- Used a label and goto statments to make sure PopThreadEventQueue gets called
in all cases
- changed char * to nsXPIDLCString class but didn't change nsCString to
nsLiteralCString. Later doesn't allow string manipulations.
Assignee | ||
Comment 60•24 years ago
|
||
Comment 61•24 years ago
|
||
you're right about the nsLiteralCString stuff - I forgot that you were modifying
the string.
Everything looks good - the one problem I see is that if you fail with
PREF_EvaluateConfigScript, you'll never set mLoaded to true, and thus never get
out of your event loop! So if you somehow tget a bad config file that isn't
valid JS, it will just hang on startup. There may be similar pitfalls that I
haven't noticed...
Assignee | ||
Comment 62•24 years ago
|
||
There are two places PREF_Evaluate can fail,
First from onStopRequest, it will call readOfflineFile(), that will take care of
mLoaded and second from evaluateLocalFile(), in that case when it
returns to readOfflineFile(), reportError() will take care of setting mLoaded to
true.
I understand that if mLoaded will not set to true in any of the possible path,
the code will just hang on startup. I have tried my best to set it through all
possible code paths.
Thanks for the review.
Assignee | ||
Comment 63•24 years ago
|
||
Comment 64•24 years ago
|
||
My thoughts:
Both NS_WITH_SERVICE and AddObserver can return an error. You should probably be
doing these in an Init function (i.e. use NS_GENERIC_FACTORY_CONSTRUCTOR_INIT)
rather than in the constructor.
While you're in there, I would also get the prefBranch, once, into a member
variable. Also use prefService->GetBranch(nsnull) rather than doing the QI this
will get you a real branch reference rather than a re-directed one.
In your destructor I believe you should be calling RemoveObserver() and/or
deriving yourself from nsSupportsWeakReference.
Assignee | ||
Comment 65•24 years ago
|
||
Posting a latest set of changes including:
- Moved the initialization stuff from constructor to Init() and changed the
generic factory constructor macro to include init
- Initialized prefs branch in a member variable
- Added RemoveObserver() to the destructor.
- Not deriving from nsSupportsWeakReference. It is causing the object to get
deleted at the first Observe() call. The other way would be to add a "service"
word to the module name in the AddCategory() method in nsPrefsFactory.cpp.
- Removed Push/Pop from the event loop code. It was casuing the Linux build to
hang and for this event loop it should be fine (talked to Danm about it)
Assignee | ||
Comment 66•24 years ago
|
||
Comment 67•24 years ago
|
||
r=bnesse.
Comment 68•24 years ago
|
||
Wow. Big bug. I just thought I'd also mention that NS_WITH_SERVICE has long been
deprecated in favour of nsCOMPtr...do_GetService. But they're both basically the
same thing, so I don't know why I'm complaining.
Comment 69•24 years ago
|
||
NS_WITH_SERVICE is depricated? Where was the memo? Oh well, I guess I have some
code to clean up...
Comment 70•24 years ago
|
||
don't worry about it - it used to be a complicated macro, but it's been
redefined just to be do_GetService()
Assignee | ||
Comment 71•24 years ago
|
||
New version of nsAutoConfig with following minor changes:
- Added an extra check for httpStatus since this is primarily going to be used
as http request, might as well add an extra check, instead sending the erroneous
data to javascript.
- Moved the event code under the first_time case, We actually, need to stall the
thread only on the startup, later on when the timer kicks in, it's fine if it's
asynchronus.
Requesting r & sr
Thanks,
Assignee | ||
Comment 72•24 years ago
|
||
Comment 73•24 years ago
|
||
Opps. Please fix utilityOverlay.js
(mozilla/xpfe/communicator/resources/content/) to use nsIPrefService and
nsIPrefBranch rather than nsIPref. And, I guess, change the NS_WITH_SERVICE
macros to do_CreateInstance. No point in checking in depricated code.
Assignee | ||
Comment 74•24 years ago
|
||
Changes as per Brian's review
- Removed NS_WITH_SERVICE macros, replaced with do_GetService
- changed utilityOverlay.js to use nsPrefService and nsIPrefBranch
Requesting r & sr ,
Thanks.
Assignee | ||
Comment 75•24 years ago
|
||
Comment 76•24 years ago
|
||
r=bnesse
Comment 77•24 years ago
|
||
Jumping in with some review, more later:
+ nsIInputStream *inStr;
Use an nsCOMPtr here...
+
+ rv = NS_NewLocalFileInputStream(&inStr, file);
+ if (NS_FAILED(rv)) return rv;
+
+ PRInt64 fileSize;
+ PRUint32 fs, amt=0;
+ file->GetFileSize(&fileSize);
+ LL_L2UI(fs, fileSize); // Converting 64 bit structure to unsigned int
+ char* buf = (char *) PR_Malloc(fs*sizeof(char)) ;
+ if(!buf)
+ return NS_ERROR_OUT_OF_MEMORY;
... or you'll leak if you return this error here...
+
+ rv = inStr->Read(buf, fs, &amt);
+ if (NS_FAILED(rv))
+ NS_ASSERTION((NS_BASE_STREAM_WOULD_BLOCK != rv),
+ "The stream should never block.");
+
+ if (!PREF_EvaluateConfigScript(buf,fs,nsnull, PR_FALSE, PR_TRUE, PR_FALSE))
+ return NS_ERROR_FAILURE;
... or here -- AND you leak buf here, don't forget to free it! ...
+ inStr->Close();
+ PR_FREEIF(buf);
+ return NS_OK;
No NS_RELEASE(inStr), so you always leak that.
+ nsIOutputStream *outStr;
nsCOMPtr please...
+ PRUint32 amt;
+
+ rv = NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR,
+ getter_AddRefs(failoverFile));
+ if (NS_FAILED(rv)) return rv;
+#ifdef XP_MAC
+ failoverFile->Append("Essential Files");
+#endif
+
+ failoverFile->Append("failover.jsc");
+
+ rv = NS_NewLocalFileOutputStream(&outStr, failoverFile);
+ if (NS_FAILED(rv)) return rv;
+ rv = outStr->Write(mBuf.get(),mBuf.Length(),&amt);
+ if (NS_FAILED(rv)) return rv;
...else you leak on the early return after Write failure.
+ outStr->Close();
... and then you return without NS_RELEASE(outStr), so it's always leaked.
Should you close even if the Write fails? If so, just remove the rv failure
check and early return one-liner after the Write call.
+ return NS_OK;
In general, don't use nsCAutoString as a formal parameter type in XPCOM-like
methods. I know, getEmailAddr is protected, but this is a pattern to avoid. Is
it useful here to declare the narrowest type? There's only one call that I can
see, so it "doesn't hurt" -- but could there be more calls in the future?
+nsresult nsAutoConfig::getEmailAddr(nsCAutoString & emailAddr)
+{
+
+ nsresult rv;
+ nsXPIDLCString prefValue;
+ nsCAutoString prefStr;
+
+ /* Getting an email address through set of three preferences:
+ First getting a default account with
+ "mail.accountmanager.defaultaccount"
+ second getting an associated id with the default account
+ Third getting an email address with id
+ */
+
+ rv = mPrefBranch->GetCharPref("mail.accountmanager.defaultaccount",
+ getter_Copies(prefValue));
+ // Checking prefValue and its length, too. Since by default the preference
+ // is set to nothing
+ if (NS_SUCCEEDED(rv) && prefValue && nsCRT::strlen(prefValue) > 0) {
Where did this pattern of checking both NS_SUCCEEDED(rv) && prefValue arise? It
should not be necessary for string (Char) prefs, and it only adds cost and doubt
(at the limit, covering up a bug in GetCharPref where an allocation fails but
NS_ERROR_OUT_OF_MEMORY is not returned).
+ prefStr = NS_LITERAL_CSTRING("mail.account.") +
+ nsLiteralCString(prefValue) + NS_LITERAL_CSTRING(".identities");
Good use of nsLiteralCString -- stylistically, it would be a little better to
use nsLocalCString(prefValue).
+ rv = mPrefBranch->GetCharPref(prefStr,getter_Copies(prefValue));
+ if (NS_SUCCEEDED(rv) && prefValue && nsCRT::strlen(prefValue) > 0) {
+ prefStr = NS_LITERAL_CSTRING("mail.identity.") +
+ nsLiteralCString(prefValue) + NS_LITERAL_CSTRING(".useremail");
A-indenting we will go. Why not just return rv; early if (NS_FAILED(rv) ||
*prefValue == '\0'), for both these cases?
+ rv = mPrefBranch->GetCharPref(prefStr,getter_Copies(prefValue));
+ if (NS_SUCCEEDED(rv) && prefValue && nsCRT::strlen(prefValue) > 0)
{
+ prefStr = nsLiteralCString(prefValue);
nsLocalCString.
+ }
+ else
+ return rv;
+ }
+ else
+ return rv;
+ }
else-return clauses are a sign that you should consider inverting the sense of
the if condition and return early in the revised if's then clause.
+ else {
+ if (mCurrProfile) {
+ prefStr=mCurrProfile;
How about a space on both sides of that =?
+ }
+ }
+
+ if(prefStr) emailAddr = prefStr;
So (reversing myself slightly to allow for the nsCAutoString& parameter) why
didn't you just compute the result in emailAddr, and avoid another nsCAutoString
and a byte-copy here?
+
+ return NS_OK;
+}
Use the proper modeline parameters here:
+++ nsAutoConfig.h Thu May 3 15:56:29 2001
@@ -0,0 +1,63 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
to wit: tab-width: 2 and c-basic-offset: 2 -- or else change the class's
indentation to use 4 space basic offset. Of course, the existing
modules/libpref/src .cpp files use a basic offset of 2, so When In Rome argues
for that in the new files you're adding -- unless you want to move those to a
separate directory?
In utilityOverlay.js, is all the if-testing necessary? Why not just let the
hard case of a service manager error cause an exception to be thrown? Testing
prefService, prefBranch, etc. seems unnecessary to me.
+ if (!offlineLocked ) {
+ if (aToggleFlag)
+ ioService.offline = !ioService.offline;
+ }
+ else {
+ broadcaster.setAttribute("disabled","true");
+ }
Indentation is whacked in the else clause.
/be
Comment 78•24 years ago
|
||
Might want to withdraw that r=bnesse!
/be
Assignee | ||
Comment 79•24 years ago
|
||
Made changes according to Brendan's suggestions. Alecf gave a sr over AIM
Still need to be tested on Linux and Mac before checking in.
Assignee | ||
Comment 80•24 years ago
|
||
Assignee | ||
Comment 81•24 years ago
|
||
Comment 82•24 years ago
|
||
Modeline should use 4, not 2, for c-basic-offset at least:
+++ nsAutoConfig.cpp Thu May 3 18:59:00 2001
@@ -0,0 +1,493 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
tab-width doesn't matter in light of indent-tabs-mode unless there are still
tabs in the file that stand for 4-space indentation units, in which case it too
should be 4.
Why is buf 1025 bytes long?
+ char buf[1025];
else after return is a non-sequitur, and incongruous in light of early returns
above it in the same function:
+ return NS_OK;
+ }
+ else {
+ NS_WARNING("Error reading autoconfig.jsc from the network, reading the
offline version");
What happened to the indentation/brace style here?
+NS_IMETHODIMP nsAutoConfig::Observe(nsISupports *aSubject, const PRUnichar
*aTopic, const PRUnichar *someData)
+{
+ nsresult rv = NS_OK;
+ if (!nsCRT::strcmp(aTopic,
NS_LITERAL_STRING("profile-after-change").get()))
+ {
+ // Getting the current profile name since we already have the
The rest of the file seems to put open braces on the same line as their if,
while, etc. keywords. Anyway, the substance of the then clause (starting with
the comment // Getting the current profile name...) should not be indented yet
again. Also, there's another else after return just below:
+ return DownloadAutoCfg();
+ }
+ else if (!nsCRT::strcmp(aTopic, NS_LITERAL_STRING("app-startup").get()))
Just above this (above the 'return DownloadAutoCfg();') is this:
+ if (NS_SUCCEEDED(rv))
+ // setting the member variable to the current profile name
+ mCurrProfile.AssignWithConversion(profileName);
+ else
+ return rv;
+ }
The if should test NS_FAILED(rv) and return early, not NS_SUCCEEDED -- that way,
you don't overindent the mCurrProfile.AssignWithConversion(profileName); and it
flows into the subsequent return DownloadAutoCfg(); more clearly.
Also, why is mCurrProfile a nsCString, while the only source of it,
nsProfileAccess::GetCurrentProfile, returns an nsString? Use the right type or
non-ISO-Latin-1 characters will be lost (or if GetCurrentProfile is abusing the
nsString type, file a bug on it).
In DownloadAutoCfg, you test firstTime early, presumably to avoid hanging if
there's some unlikely fatal error getting the event queue service or queue:
+ if (firstTime) {
+ nsCOMPtr<nsIEventQueueService> service =
+ do_GetService(NS_EVENTQUEUESERVICE_CONTRACTID, &rv);
+ if (NS_FAILED(rv)) return rv;
+ rv =
service->GetThreadEventQueue(NS_CURRENT_THREAD,getter_AddRefs(currentThreadQ));
+ NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
+ }
+ mLoaded = PR_FALSE;
That's ok, but why then set mLoaded to PR_FALSE unconditionally? It was already
set false in Init, so it shouldn't be necessary to clear it again if firstTime.
And if !firstTime, then what will set it true? Is DownloadAutoCfg meant to be
callable at any time?
In getEmailAddr, I suggested testing *prefValue == '\0' after NS_FAILED(rv) ||
in order to throw out bad string (Char) pref values, but you kept the
nsCRT::strlen() calls -- not a big deal, except you mean == 0, not < 0, here and
later:
+ if (NS_FAILED(rv) || nsCRT::strlen(prefValue) < 0) return rv;
In utilityOverlay.js, why try/catch?
+ } catch(e) {
+ }
This suppresses any exception printing, which would be a useful clue as to why
things went wrong in the rare case that you couldn't get the service or the null
pref branch, or whatever. Let the chips fall where they may, get rid of all try
with empty catch clause instances. Or is there a bug where you sometimes get an
XPCOM failure result (that turns into a JS exception)?
/be
Comment 83•24 years ago
|
||
Closing this as RESOLVED.
(This is basically for the Feature Development)
Open new bugs for the improvement.
Thanks, Mohan.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 84•24 years ago
|
||
Mohan, I asked mitesh some questions (non-trivial ones included those concerning
the decimation from nsString to nsCString, the utterly broken testing of strlen
< 0, and the DownloadAutoCfg non-firstTime logic). He hasn't answered, and may
not if the bug stays closed. It was assigned to him, I think he should answer
my questions, whether here or in new bug(s). Your closing the bug when it is
not assigned to you, and when it has open code review questions pending, is bad
form.
I don't think this code got a proper code review.
/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 85•24 years ago
|
||
Brenden,
I would be working on AutoConfig code for any escalations, while Mitesh is on
vacation till mid June.
Thanks, Mohan.
Comment 86•24 years ago
|
||
Mohan: Since this bug has useful information still alive in it, rather than try
to copy it to a new bug, can you please reassign to yourself?
/be
Assignee | ||
Comment 89•23 years ago
|
||
I will correct the tab width and indentation in the next revision of the file.
Regarding the size of the character buffer, I copied it from an example in a
test directory in netwerk module. What should be the right one then?
I will make the code consistent to have "if(fail) return rv" format.
Regarding mCurrProfile being nsCString, I think all other string operation are
of C format string so I kept it nsCString and when I got the profileName in
nsString format I converted it to nsCString. I need nsCString to append it to
another nsCString. I will see what's better and efficient way but I think one
conversion will sure be needed.
setting of mLoaded to PR_FALSE is redundant, will remove it. mLoaded is not
needed after the first time. Also, mLoaded is being set to true through all
possible code paths to avoid any dead lock(mainly from readOfflineFile() and
OnStopRequest()) yes, DowLoadAutoConfig is callable at any time, the reason why
the event queue is implemented first time is because it's only necessary to
enforce sync read during the startup, all other times, it's ok to have
asyncread and firstTime is already available for Timer implementation so no
extra cost.
yes, strlen(prefValue) < 0 testing is wrong, it should be ==0 or the better way
would be to test it again a null character. I will change it in the next
revision.
Thanks for the review.
Comment 90•23 years ago
|
||
>Regarding the size of the character buffer, I copied it from an example in a
>test directory in netwerk module. What should be the right one then?
1024 is a power of two and (more imp., no big deal still) is 0 mod 4. 1025
wastes a few bytes of stack on alignment padding. Why not be traditional and
use 1024? Nit, minor, still worth doing. Maybe the example you copied from
wanted to put a '\0' terminator after reading at most 1024 bytes, so
overallocated? Better style in that event would be to use 1024+1 or MAXLEN+1
with a manifest constant, and a comment.
>Regarding mCurrProfile being nsCString, I think all other string operation are
>of C format string so I kept it nsCString and when I got the profileName in
>nsString format I converted it to nsCString. I need nsCString to append it to
>another nsCString. I will see what's better and efficient way but I think one
>conversion will sure be needed.
The profile name comes back in a PRUnichar* (wrapped in an nsXPIDLString), so
you should assume it can contain non-ASCII, non-ISO-Latin-1 characters for some
users, and never chop PRUnichars down to chars via AssignWithConversion -- don't
do it unless you know that GetProfileName uses too wide a string type (if you
know that, file a bug on that method). There's no great efficiency difference,
and all string operations available on chars are available on PRUnichars via
mozilla/string/* facilities.
/be
Updated•23 years ago
|
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Updated•23 years ago
|
Priority: -- → P2
Assignee | ||
Comment 92•23 years ago
|
||
I have made few improvements in the next patch as per Brendan's suggestions.
mCurrProfile is still nsCString. I want to add the profilename as a part of the
URL. So even if I use nsString, it will be converted to a CString somewhere in
order to add it to the URL.
Whiteboard: Requesting r and sr
Assignee | ||
Comment 93•23 years ago
|
||
Comment 94•23 years ago
|
||
>mCurrProfile is still nsCString. I want to add the profilename as a part of the
>URL. So even if I use nsString, it will be converted to a CString somewhere in
>order to add it to the URL.
Yes, but *how* will the PRUnichar string be converted to a char string? Will it
be converted to UTF-8? AssignWithConversion does not do that -- it just chops
16-bit chars down to 8-bit chars mercilessly.
/be
Assignee | ||
Comment 95•23 years ago
|
||
Oh! I thought AssignWithConversion will take care of it. Ok I will use
NS_ConvertUCS2toUTF8()
Status: NEW → ASSIGNED
Assignee | ||
Comment 96•23 years ago
|
||
Assignee | ||
Comment 97•23 years ago
|
||
Changes at line @@ -280,7 +275,7 @@ in the previous patch is actually for bug
84559
Assignee | ||
Comment 98•23 years ago
|
||
Requesting r & sr for the new patch.
Thanks,
Comment 99•23 years ago
|
||
In the methods:
Init()
Observe()
DownloadAutoCfg()
you are initializing rv to NS_OK. This is unnecessary as rv is always set before
being tested.
In Observe() the else case
else if (!nsCRT::strcmp(aTopic, NS_LITERAL_STRING("app-startup").get())) {
// This is the object instantiation, do nothing and return NS_OK;
return NS_OK;
}
serves no apparent purpose as NS_OK is returned anyway.
I would prefer to see mLoaded set to PR_TRUE once at the top of
readOfflineFile(), rather than at each NS_OK return point and in reportError().
It's safer and easier to follow.
In evaluateLocalFile() you probably shouldn't pass "buf" to
Pref_EvaluateConfigScript() if the call to Read() failed. Also the PR_FREEIF
macro is overkill for a buffer you know a) is valid, and b) won't outlive the
function. Also, you don't need to check NS_FAILED(rv) before the NS_ASSERTION...
it won't fire unless it failed anyway.
minor nits:
Please stick to one comment style... don't alternate between C & C++ styles.
Please be consistent in your use of spaces [i.e. "if(" and "//Comment" vs. "if ("
and "// Comment"]
This just makes the code easier to read.
Comment 100•23 years ago
|
||
PDT+ per 6/12 mtg.
Whiteboard: Requesting r and sr → [PDT+] Requesting r and sr
Assignee | ||
Comment 101•23 years ago
|
||
Made changes according to the review.
- Removed reportError() and put the mLoaded=PR_TRUE in the beginning of
readOfflineFile(). Actually, at this point we really don't need to block the
main thread so allowing it to start execution while we are reading the offline
file.
- Removed rv=NS_OK since it was unnecessary.
- Removed "app-startup" category checking from Observe().
- Replaced PR_FREEIF with PR_Free.
- Improved comment & coding style.
Assignee | ||
Comment 102•23 years ago
|
||
Comment 103•23 years ago
|
||
In Init()
+ // member initializers and constructor code */
Creating a new comment style? :)
+ nsCOMPtr<nsIObserverService> observerService =
+ do_GetService(NS_OBSERVERSERVICE_CONTRACTID, &rv);
+ if (NS_FAILED(rv)) return rv;
+ if (observerService) {
+ rv = observerService->AddObserver(this,
if rv was NS_OK, you shouldn't need to check if (observerService) here.
You have some wierd indentation in OnStopRequest and Observe. Did you "ignore
white space" on this diff? (I know you mentioned cleaning up indentation)
In evaluateLocalFile() you're going to leak buf if the Read fails. In order to
eliminate yet another PR_Free call, I'd recommend:
rv = inStr->Read(buf, fs, &amt);
if (NS_SUCCEEDED(rv)) {
if (!PREF_EvaluateConfigScript(buf, fs, nsnull, PR_FALSE, PR_TRUE, PR_FALSE))
rv = NS_ERROR_FAILURE;
}
inStr->Close();
PR_Free(buf);
return rv;
Assignee | ||
Comment 104•23 years ago
|
||
Oops, forgot to remove '*/'
yes, I ignored the white space for this diff.
I think, using NS_SUCCEEDED for evaluateLocalFile seems a good idea.
posting new diffs without '-w' option.
Assignee | ||
Comment 105•23 years ago
|
||
Comment 106•23 years ago
|
||
the use of rv in
+NS_IMETHODIMP nsAutoConfig::Observe(nsISupports *aSubject,
+ const PRUnichar *aTopic,
+ const PRUnichar *someData)
+{
+ nsresult rv;
+ if (!nsCRT::strcmp(aTopic,
+ NS_LITERAL_STRING("profile-after-change").get())) {
...
+ return DownloadAutoCfg();
+ }
return NS_OK;
}
appears a bit unusual, if you aren't going to use rv outside the |if| then i'd
suggest moving it inside, however i'm sure no one would like that solution.
oh, an alternative which you might consider to the preceding code (this does
not address my complaint):
nsresult rv;
if (nsCRT::strcmp(aTopic,
NS_LITERAL_STRING("profile-after-change").get()))
return NS_OK;
/*... body of older if block*/
return DownloadAutoCfg();
you use this syntax a lot:
if (NS_FAILED(rv)) return ...;
unfortunately, it makes it hard to place a breakpoint on |return ...;| would
your module owner consider allowing you to split them?
I should note that code you've reindented uses the multiline format.
converting from /* long descriptive multiline comment block */ to // for each
line in a long comment block is rather unappealing.
I'm guessing this was in response to Comments From Brian Nesse 2001-06-12 16:58
I think his comments were misinterpretted.
- // failCache = true
+ // failCache == true
since this isn't a code, could you use words?
Comment 107•23 years ago
|
||
Personally, I'm not a big fan of:
if (NS_FAILED(rv)) return ...;
I won't complain at all if it changes.
And, yes,
/*
* comment block
*/
is ok. I was really complaining about the inline use.
Comment 108•23 years ago
|
||
if you're not a fan, you can always use
if (NS_FAILED(rv))
return rv;
then it's possible to break on the second line
if it's something you really want to debug every time it happens, you can use
NS_ENSURE_SUCCESS(rv, rv);
which will throw an assertion if rv is a failure - you can catch it in the
debugger.. in release builds, it'll just return rv on failure.
Assignee | ||
Comment 109•23 years ago
|
||
Ok, replaced if (NS_FAILED(rv)) return rv; with
if (NS_FAILED(rv)
return rv;
for easy debugging.
- reverted back the comment blocks to /*..*/ style. I misunderstood the comment
by bneese.
- Improved comment for 'failcache == true' case.
Assignee | ||
Comment 110•23 years ago
|
||
Comment 111•23 years ago
|
||
ok, time for me to sound totally ignorant :-(
// Releasing the deadlock
what does that mean?? In normal terms once you have deadlock you're sunk and
you can't possibly do anything to fix it.
i still don't like (again no one cares):
+ return DownloadAutoCfg();
+ }
return NS_OK;
because you could easily have done:
+ nsresult rv=NS_OK;
if (...){
...
+ rv=DownloadAutoCfg();
+ }
- return NS_OK;
+ return rv;
which saves a return and makes consistent use of rv.
Comment 112•23 years ago
|
||
Seems reasonable to me. I would also make the same case at the bottom of Init()
rv = prefs->GetBranch(nsnull,getter_AddRefs(mPrefBranch));
+ if (NS_FAILED(rv))
+ return rv;
return NS_OK;
could just be:
rv = prefs->GetBranch(nsnull,getter_AddRefs(mPrefBranch));
return rv;
The "deadlock" reference is caused in DownloadAutoConfig where it loops while
!mLoaded. Perhaps the comment should refer to releasing from that loop instead?
Assignee | ||
Comment 113•23 years ago
|
||
Points noted. will post the updated patch later on at once.
any more suggestion?
this needs to be checked in before Tuesday. (0.9.2 deadline)
Comment 114•23 years ago
|
||
timeless: "return" is not expensive - there's no reason to "save" them, and the
compiler will optimize out unused local variables like rv..(or optimize them out
once the function isn't using it any more) this is really a personal preference
issue - mitesh, do whatever you prefer.
Comment 115•23 years ago
|
||
re: return whatever, consistency is nice and bnesse seemed to agree.
i think i'm done. sorry to delay. however i'm not sure i like your sense of
urgency in 'this needs to be checked in before Tuesday. (0.9.2 deadline)'
but that's because i'm jaded by dealing w/ bug 77914 which has more status,
keywords, dupes, ccs, and votes than I can shake a stick at. Again i'm really
sorry to delay your patch (wow this bug is 27 pages while that one is only 17).
Assignee | ||
Comment 116•23 years ago
|
||
Comment 117•23 years ago
|
||
For consistency, please change the 2
if (NS_FAILED(rv) || (len = nsCRT::strlen(prefValue)) == 0) return rv;
references in getEmailAddr() to be multi line.
With that change r=bnesse.
Comment 118•23 years ago
|
||
sr=alecf with brian's comments
Assignee | ||
Comment 119•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Whiteboard: [PDT+] Requesting r and sr → [PDT+] , r and sr done, requesting drivers' approval
Comment 121•23 years ago
|
||
a=dbaron for 0.9.2 checkin of attachment 39249 [details] [diff] [review] (on behalf of drivers). Although
this isn't normally the type of thing we're looking for in a stabilization
period before a release, you seem to want to get it in, the only changes to
things other than handling of error conditions seem to be minor ones (to when
you set mLoaded and under what conditions you call PREF_EvaluateConfigScript),
and almost all the changes seem to be only in the codepath of a feature that's
not enabled by default.
However, please note that drivers@mozilla.org has the power not to approve bugs
(even when developers and their managers repeatedly send email requesting such
approval) -- otherwise there would be no point to the approval process.
Also, if you'd said that the entire patch was code cleanup in your initial
message to drivers, I wouldn't have instinctively replied to your one-line
request asking for more details after I saw the 600 line patch. (Under the
pressure of large numbers of approval requests, members of drivers@mozilla.org
don't have the time to read the entire bug for lengthy bugs such as this one,
especially at 17:30 on the last day before the milestone freeze.)
Comment 122•23 years ago
|
||
Thanks for your help dbaron!
Mitesh, please checkin asap.
Whiteboard: [PDT+] , r and sr done, requesting drivers' approval → [PDT+] ready for checkin
Assignee | ||
Comment 123•23 years ago
|
||
Fix checked in to the trunk and the branch
Assignee | ||
Updated•23 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 23 years ago
Resolution: --- → FIXED
Whiteboard: [PDT+] ready for checkin → Checked in
Assignee | ||
Comment 124•23 years ago
|
||
marking it fixed
Comment 125•23 years ago
|
||
Verified this has been fixed in 0626 trunk builds on MacOS, Win32, and Linux.
Status: RESOLVED → VERIFIED
Comment 126•23 years ago
|
||
I have also verified that this functionality is present on the 0625 branch
build.
You need to log in
before you can comment on or make changes to this bug.
Description
•