Closed Bug 221623 Opened 21 years ago Closed 16 years ago

<wallet.cpp> (and related files) code cleanup

Categories

(SeaMonkey :: Passwords & Permissions, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.8final

People

(Reporter: sgautherie, Assigned: sgautherie)

References

Details

Attachments

(8 files, 1 obsolete file)

1.24 KB, patch
brendan
: superreview-
Details | Diff | Splinter Review
1.57 KB, patch
dwitte
: review+
bryner
: superreview+
Details | Diff | Splinter Review
15.53 KB, patch
Details | Diff | Splinter Review
8.02 KB, patch
mconnor
: review+
bryner
: superreview+
Details | Diff | Splinter Review
2.49 KB, patch
alecf
: review+
bryner
: superreview+
Details | Diff | Splinter Review
595 bytes, patch
mconnor
: review+
bryner
: superreview+
Details | Diff | Splinter Review
1.29 KB, patch
mconnor
: review+
bryner
: superreview+
Details | Diff | Splinter Review
8.91 KB, patch
mconnor
: review+
bryner
: superreview+
Details | Diff | Splinter Review
{It's my first bug on "code cleanup", so mind a little explanation if I'm doing
this in the wrong way...}

I noticed this while looking into bug 209423...

Looking at WLLT_OnSubmit(...) defined at
http://lxr.mozilla.org/seamonkey/source/extensions/wallet/src/wallet.cpp#3904
I am puzzled by:

A) The use of 'passwordcount' (defined at line 3955) at line 3968 without
"#ifdef AutoCapture"...
Same issue with 'OKToPrompt' at lines 3968 and 3974;
Same issue with 'newValueFound', and possibly others.

Would not a compilation with an undefined 'AutoCapture' fail ??

B) Shouldn't the 'WALLET_DONT_CACHE_ALL_PASSWORDS' be removed at lines 3996,
3997 and 4012 ?
If not, then, could a brief comment by added to document its purpose ?

Thanks.
The author of this code is long gone, so it's anyone's guess as to what the
intent was....  
Keywords: helpwanted
Removes the local define;
Improves the comment.
Comment on attachment 135070 [details] [diff] [review]
(Bv1) 'WALLET_DONT_CACHE_ALL_PASSWORDS' removal
[Checked in: comment 11]


Addition to comment 2:

The added line in the comment is adapted from
<http://msdn.microsoft.com/workshop/author/dhtml/reference/properties/autocompl
ete.asp>
{
Standards Information

This property is a Microsoft extension to HTMLs
}

(This is my 2nd patch, using 'cvs co' this time.
But still guessing about who to ask for r=? and sr=?)
Attachment #135070 - Flags: superreview?(brendan)
Attachment #135070 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 135070 [details] [diff] [review]
(Bv1) 'WALLET_DONT_CACHE_ALL_PASSWORDS' removal
[Checked in: comment 11]

something tells me Neil really doesn't want to review this ;)

kicking r= request to me. i'll look at this soon.
Attachment #135070 - Flags: review?(neil.parkwaycc.co.uk) → review?(dwitte)
Part C)

'W*T_FetchFromNetCenter()' do nothing by now:

Creations were done in
{
nsWalletService.cpp 1.25
wallet.h 1.11

<morse@netscape.com> 09 Aug 1999 23:26
load up mapping tables when browser starts up
}
and
{
nsIWalletService.idl (1.0)

(?)
}

Main deletion was done in
{
wallet.cpp 1.268

morse%netscape.com
Nov 14 2000
bug 59687, wallet to determine schema from displayable strings, r=dveditz,
a=alecf
}

Is there any reason to keep theses "empty" functions anymore ?
Attachment #135086 - Flags: review?(dwitte)
Comment on attachment 135070 [details] [diff] [review]
(Bv1) 'WALLET_DONT_CACHE_ALL_PASSWORDS' removal
[Checked in: comment 11]


Addition to comment 2:

The 'WALLET_DONT_CACHE_ALL_PASSWORDS' and the inside initial code was added in
{
wallet.cpp 1.284 

<morse@netscape.com>
08 Mar 2001 14:15
bug 63961, server can't turn off password manager, r=jelwell@netscape.com,
sr=alecf@netscape.com
}
(simple reformatting, while at it)

*Adds '// nsWalletlibService'
*Trims white spaces
Attachment #135258 - Flags: review?(dwitte)
Comment on attachment 135070 [details] [diff] [review]
(Bv1) 'WALLET_DONT_CACHE_ALL_PASSWORDS' removal
[Checked in: comment 11]

How are you generating this diff?

Can you avoid overflowing column 80 (or occupying it, for that matter -- IIRC,
emacs will wrap prematurely in that case)?

Again, use diff -w for patches that change white space, and if the only other
changes are trivial (unused var removal) or non-existent, don't bother asking
for my sr=.

/be
Attachment #135070 - Flags: superreview?(brendan) → superreview-
Comment on attachment 135086 [details] [diff] [review]
(Cv1) 'W*T_FetchFromNetCenter()' removal
[Checked in: comment 14]

r=me... bryner, care to sr this trivial patch?
Attachment #135086 - Flags: superreview?(bryner)
Attachment #135086 - Flags: review?(dwitte)
Attachment #135086 - Flags: review+
Comment on attachment 135070 [details] [diff] [review]
(Bv1) 'WALLET_DONT_CACHE_ALL_PASSWORDS' removal
[Checked in: comment 11]

like brendan said... in future, no need to request r/sr on patches like this -
just ask someone to check them in.

(your part C patch does change things in a meaningful way, so you do need r/sr
on that).
Attachment #135070 - Attachment is obsolete: true
Attachment #135070 - Flags: review?(dwitte)
Comment on attachment 135258 [details] [diff] [review]
(Dv1) white space trimming
[Checked in: comment 11]

checked in this patch and the 'part B' patch.

although, i'm not sure it's worth your time (or anyone's) to clean up
whitespace in this code... this code needs much more love than that. a really
worthy endeavour would be to port firebird's wallet (which bryner wrote from
scratch) over to seamonkey, and just nuke this code altogether ;)

also, in future, please use cvs diff to generate diffs.
Attachment #135258 - Attachment is obsolete: true
Attachment #135258 - Flags: review?(dwitte)
-> gautheri
Assignee: dveditz+bmo → gautheri
Attachment #135070 - Attachment description: (Part B) 'WALLET_DONT_CACHE_ALL_PASSWORDS' removal patch → (Part B) 'WALLET_DONT_CACHE_ALL_PASSWORDS' removal patch [Checked in: comment 11]
Attachment #135258 - Attachment description: (Part D) white space trimming (v1) → (Part D) white space trimming (v1) [Checked in: comment 11]
Re comment 11 and comment 12:

FireBird to seamonkey port:
I guess this is still far beyond my skills !

True 'helpwanted' call :->

Part A)
I guess it will stay as it is for a while...
Attachment #135086 - Flags: superreview?(bryner) → superreview+
Comment on attachment 135086 [details] [diff] [review]
(Cv1) 'W*T_FetchFromNetCenter()' removal
[Checked in: comment 14]

checked in.
Attachment #135086 - Attachment is obsolete: true
Attachment #135070 - Attachment is obsolete: false
Attachment #135258 - Attachment is obsolete: false
Attachment #135086 - Attachment description: (Part C) 'W*T_FetchFromNetCenter()' removal patch → (Part C) 'W*T_FetchFromNetCenter()' removal patch [Checked in: comment 14]
Attachment #135086 - Attachment is obsolete: false
Severity: normal → trivial
OS: Windows 98 → All
Hardware: PC → All
Whiteboard: [Only Part A remains: see comment 13]
Attachment #135070 - Attachment is obsolete: true
Attachment #135086 - Attachment is obsolete: true
Attachment #135258 - Attachment is obsolete: true
Depends on: 59687
Comment on attachment 136782 [details] [diff] [review]
(Av1) 'AutoCapture' removal
[Checked in: Comment 29]

Can you (super-)review, compile, test, check it in ?

|#define AutoCapture| was added in
{
1.149
morse%netscape.com
Feb 3 2000
implement disginguished-schema in wallet
r=waterson
}
and left untouched since then.
Attachment #136782 - Flags: superreview?(bryner)
Attachment #136782 - Flags: review?(dwitte)
Attachment #136782 - Flags: superreview?(bryner) → superreview+
Attachment #135070 - Attachment description: (Part B) 'WALLET_DONT_CACHE_ALL_PASSWORDS' removal patch [Checked in: comment 11] → (Bv1) 'WALLET_DONT_CACHE_ALL_PASSWORDS' removal [Checked in: comment 11]
Attachment #135086 - Attachment description: (Part C) 'W*T_FetchFromNetCenter()' removal patch [Checked in: comment 14] → (Cv1) 'W*T_FetchFromNetCenter()' removal [Checked in: comment 14]
Attachment #135258 - Attachment description: (Part D) white space trimming (v1) [Checked in: comment 11] → (Dv1) white space trimming [Checked in: comment 11]
Attachment #136782 - Attachment description: (Part A) 'AutoCapture' removal patch v1 → (Av1) 'AutoCapture' removal
Comment on attachment 137752 [details] [diff] [review]
(Ev1) 'IgnoreFieldNames' removal
[Checked in: Comment 28]

'r=?': (see comment 17)
I have no compiler: Could you compile/test/review it ? Thanks.
Attachment #137752 - Flags: superreview?(bryner)
Attachment #137752 - Flags: review?(dwitte)
Attachment #137752 - Flags: superreview?(bryner) → superreview+
Attachment #136782 - Flags: review?(dwitte) → review?(alecf)
Attachment #137752 - Flags: review?(dwitte) → review?(alecf)
Serge, please get a compiler! Install linux already.

In the mean time, we should discuss why we are removing autocapture in the first
place. 
(In reply to comment #19)
> Serge, please get a compiler! Install linux already.

I know ... but still not planned to happen soon :-(

> In the mean time, we should discuss why we are removing autocapture in the first
> place. 

(see comment 0 and comment 16)
No reported misbehaviour: I'm only removing the define, and the
(else-)associated code which is actually unused.
Comment on attachment 137752 [details] [diff] [review]
(Ev1) 'IgnoreFieldNames' removal
[Checked in: Comment 28]

sr=alecf
Attachment #137752 - Flags: review?(alecf) → review+
Comment on attachment 136782 [details] [diff] [review]
(Av1) 'AutoCapture' removal
[Checked in: Comment 29]

sorry but AutoCapture is actually defined here, which means you're removing
features that you clearly don't understand. I realize this code is complex, but
I don't see a good reason to drop it until we see some better implementation.

no review here from me.
Attachment #136782 - Flags: review?(alecf) → review-
Comment on attachment 137752 [details] [diff] [review]
(Ev1) 'IgnoreFieldNames' removal
[Checked in: Comment 28]


approval1.7b=?:
Removal of unused code; no risk.
Attachment #137752 - Flags: approval1.7b?
Comment on attachment 137752 [details] [diff] [review]
(Ev1) 'IgnoreFieldNames' removal
[Checked in: Comment 28]

let's do this when the tree opens for 1.8a...  trying to focus on just work
that is critical and has higher impact.
Attachment #137752 - Flags: approval1.7b? → approval1.7b-
Comment on attachment 137752 [details] [diff] [review]
(Ev1) 'IgnoreFieldNames' removal
[Checked in: Comment 28]


'approval1.7=?':
Unused code removal, no risk;
(Trying v1.7 instead of waiting for v1.8a, as v1.7 will become new
long-lived...)
Attachment #137752 - Flags: approval1.7?
Attachment #137752 - Flags: approval1.7?
Comment on attachment 136782 [details] [diff] [review]
(Av1) 'AutoCapture' removal
[Checked in: Comment 29]

r=me on this, this code hasn't been on by default for over four years, and I
can't see anything useful about the code in question that justifies retaining
it

make sure to kill the pref wallet.notified at
http://lxr.mozilla.org/mozilla/source/mail/app/profile/thunderbird.js#199 and
http://lxr.mozilla.org/mozilla/source/xpfe/bootstrap/browser-prefs.js#140 when
this gets checked in
Attachment #136782 - Flags: review- → review+
Fix checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment on attachment 137752 [details] [diff] [review]
(Ev1) 'IgnoreFieldNames' removal
[Checked in: Comment 28]


Check in: { 2004-04-17 12:20	neil%parkwaycc.co.uk	mozilla/ extensions/
wallet/ src/ wallet.cpp    1.360 }
Attachment #137752 - Attachment description: (Ev1) 'IgnoreFieldNames' removal → (Ev1) 'IgnoreFieldNames' removal [Checked in: Comment 28]
Attachment #137752 - Attachment is obsolete: true
(In reply to comment #27)
> Fix checked in.

(In reply to comment #26)
> (From update of attachment 136782 [details] [diff] [review])
> 
> make sure to kill the pref wallet.notified at
> http://lxr.mozilla.org/mozilla/source/mail/app/profile/thunderbird.js#199 and
> http://lxr.mozilla.org/mozilla/source/xpfe/bootstrap/browser-prefs.js#140 when
> this gets checked in

'xpfe' part was checked in at the same time; I guess 'mail' part was too.
Attachment #136782 - Attachment description: (Av1) 'AutoCapture' removal → (Av1) 'AutoCapture' removal [Checked in: Comment 29]
Attachment #136782 - Attachment is obsolete: true
Keywords: helpwanted
Whiteboard: [Only Part A remains: see comment 13]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
This was checked in by
{{
1.144	morse%netscape.com	2000-02-01 00:09		getting ready
to support encrytion, r=jar
}}

But it was not used at the time ... and is not currently either:
{{ <http://lxr.mozilla.org/seamonkey/search?string=morseAssert>
morseAssert
/extensions/wallet/src/wallet.cpp, line 80 -- #define morseAssert NS_ASSERTION
/extensions/wallet/src/wallet.cpp, line 82 -- #define morseAssert(x,y) 0
}}
Comment on attachment 146414 [details] [diff] [review]
(Fv1) <wallet.cpp> |morseAssert| removal
[Checked in: Comment 35]


I have no compiler: Could you compile/test/review it ? Thanks.
Attachment #146414 - Flags: review?(mconnor)
Attachment #146414 - Flags: review?(mconnor) → review+
Attachment #146414 - Flags: superreview?(bryner)
Attachment #146414 - Flags: superreview?(bryner) → superreview+
(Not this bug exactly, but directly related:)
These preferences were never used, or left after previous (wallet.cpp !!?) code
cleanups.

This patch gets rid of
{{
http://lxr.mozilla.org/seamonkey/search?string=wallet.TutorialFromMenu
wallet.TutorialFromMenu
/mail/app/profile/thunderbird.js, line 199 -- pref("wallet.TutorialFromMenu",
"chrome://navigator/locale/navigator.properties");

http://lxr.mozilla.org/seamonkey/search?string=wallet.version
wallet.version
/xpfe/bootstrap/browser-prefs.js, line 142 -- pref("wallet.version", "1");
/mail/app/profile/thunderbird.js, line 202 -- pref("wallet.version", "1");
}}

Should I remove the 2 following ones too ?
{{
http://lxr.mozilla.org/seamonkey/search?string=wallet.Samples
wallet.Samples
/xpfe/bootstrap/browser-prefs.js, line 141 -- pref("wallet.Samples",
"chrome://navigator/locale/navigator.properties");
/xpfe/browser/resources/locale/en-US/navigator.properties, line 42 --
wallet.Samples=http://www.mozilla.org/wallet/samples/
/xpfe/browser/resources/locale/en-US/region.properties, line 23 --
wallet.Samples=http://www.mozilla.org/wallet/samples/
/toolkit/locale/intl.properties, line 25 --
wallet.Samples=http://www.mozilla.org/wallet/samples/
/browser/base/locale/region.properties, line 19 --
wallet.Samples=http://www.mozilla.org/wallet/samples/
/mail/app/profile/thunderbird.js, line 201 -- pref("wallet.Samples",
"chrome://navigator/locale/navigator.properties");

http://lxr.mozilla.org/seamonkey/search?string=wallet.Server
wallet.Server
/extensions/wallet/resources/content/pref-wallet.xul, line 81 --
prefstring="wallet.Server"/>
/xpfe/bootstrap/browser-prefs.js, line 140 -- pref("wallet.Server",
"chrome://navigator/locale/navigator.properties");
/xpfe/browser/resources/locale/en-US/navigator.properties, line 41 --
wallet.Server=http://www.mozilla.org/wallet/tables/
/xpfe/browser/resources/locale/en-US/region.properties, line 22 --
wallet.Server=http://www.mozilla.org/wallet/tables/
/toolkit/locale/intl.properties, line 24 --
wallet.Server=http://www.mozilla.org/wallet/tables/
/browser/base/locale/region.properties, line 18 --
wallet.Server=http://www.mozilla.org/wallet/tables/
/mail/app/profile/thunderbird.js, line 200 -- pref("wallet.Server",
"chrome://navigator/locale/navigator.properties");
}}
Note that in <pref-wallet.xul>, this part is commented out.
Comment on attachment 146634 [details] [diff] [review]
(Gv1) Unused |wallet.*| preference removal


'r=?': (see comment 32)
Attachment #146634 - Flags: review?(mconnor)
Comment on attachment 146634 [details] [diff] [review]
(Gv1) Unused |wallet.*| preference removal

wallet.TutorialFromMenu is still in composer.js, r=me assuming you attach an
updated patch removing it from there too

when you're doing cruft removal, use /mozilla instead of /seamonkey in lxr
please

about the other two prefs, wallet.Samples is good to go, I don't really know
what wallet.Server is supposed to do, but the link gets 403 - Forbidden :)
Attachment #146634 - Flags: review?(mconnor) → review+
Comment on attachment 146414 [details] [diff] [review]
(Fv1) <wallet.cpp> |morseAssert| removal
[Checked in: Comment 35]


Check in: { 2004-04-20 16:22	neil%parkwaycc.co.uk	mozilla/ extensions/
wallet/ src/ wallet.cpp    1.363 }
Attachment #146414 - Attachment description: (Fv1) <wallet.cpp> |morseAssert| removal → (Fv1) <wallet.cpp> |morseAssert| removal [Checked in: Comment 35]
Attachment #146414 - Attachment is obsolete: true
{{
http://lxr.mozilla.org/mozilla/search?string=WalletNotification
WalletNotification
/extensions/wallet/src/resources/locale/en-US/wallet.properties, line 53 --
WalletNotification = You can save information that you enter on forms and later
automatically prefill that information on other forms. To save such
information, select Edit/Save Form Info from the menu while viewing the form.
}}

This is a left over from
{{ '(Av1) 'AutoCapture' removal' patch
-	     PRUnichar * notification = Wallet_Localize("WalletNotification");
}}
Attachment #146674 - Flags: review?(mconnor)
Attachment #146634 - Attachment is obsolete: true
Attachment #146677 - Flags: superreview?(bryner)
Attachment #146677 - Flags: review?(mconnor)
Attachment #146674 - Flags: superreview?(bryner)
Summary: <wallet.cpp> code cleanup → <wallet.cpp> (and related files) code cleanup
Attachment #146674 - Flags: superreview?(bryner) → superreview+
Attachment #146677 - Flags: superreview?(bryner) → superreview+
Attachment #146674 - Flags: review?(mconnor) → review+
Attachment #146677 - Flags: review?(mconnor) → review+
Comment on attachment 146677 [details] [diff] [review]
(Gv1b) Unused |wallet.*| preference removal
[Checked in: Comment 38]


Check in: { 2004-04-22 01:26	neil%parkwaycc.co.uk }
Attachment #146677 - Attachment description: (Gv1b) Unused |wallet.*| preference removal → (Gv1b) Unused |wallet.*| preference removal [Checked in: Comment 38]
Attachment #146677 - Attachment is obsolete: true
Comment on attachment 146674 [details] [diff] [review]
(Hv1) <wallet.properties> |WalletNotification| removal
[Checked in: Comment 39]


Check in: { 2004-04-22 01:28	neil%parkwaycc.co.uk	mozilla/ extensions/
wallet/ src/ resources/ locale/ en-US/ wallet.properties   1.5 }
Attachment #146674 - Attachment description: (Hv1) <wallet.properties> |WalletNotification| removal → (Hv1) <wallet.properties> |WalletNotification| removal [Checked in: Comment 39]
Attachment #146674 - Attachment is obsolete: true
Product: Browser → Seamonkey
Serge, is this done?
Serge, can this be marked FIXED?
Attachment #137752 - Attachment is obsolete: false
Attachment #136782 - Attachment is obsolete: false
Attachment #135258 - Attachment is obsolete: false
Attachment #135086 - Attachment is obsolete: false
Attachment #135070 - Attachment is obsolete: false
Attachment #146677 - Attachment is obsolete: false
Attachment #146674 - Attachment is obsolete: false
Attachment #146414 - Attachment is obsolete: false
(In reply to comment #40)
> Serge, is this done?

I'm sorry I didn't answered you at the time ... Maybe I didn't know yet.

(In reply to comment #41)
> Serge, can this be marked FIXED?

Maybe I was thinking about doing more cleanups back in 2004;
but now that we will eventually fix bug 390025, there is no interest in this anymore.

R.Fixed
Status: ASSIGNED → RESOLVED
Closed: 21 years ago16 years ago
Flags: in-testsuite-
QA Contact: privacy
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8final
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: