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

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Serge Gautherie (:sgautherie)]

{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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The author of this code is long gone, so it's anyone's guess as to what the
intent was....  

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) 'WALLET_DONT_CACHE_ALL_PASSWORDS' removal [Checked in: comment 11]

Removes the local define;
Improves the comment.

Comment 3

[Serge Gautherie (:sgautherie)]

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=?)

Comment 4

[[e-mail]]

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.

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) 'W*T_FetchFromNetCenter()' removal [Checked in: comment 14]

Part C)

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

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

<[e-mail]> 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 ?

Comment 6

[Serge Gautherie (:sgautherie)]

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 

<[e-mail]>
08 Mar 2001 14:15
bug 63961, server can't turn off password manager, r=[e-mail],
sr=[e-mail]
}

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) white space trimming [Checked in: comment 11]

(simple reformatting, while at it)

*Adds '// nsWalletlibService'
*Trims white spaces

Comment 8

[Brendan Eich [:brendan]]

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

Comment 9

[[e-mail]]

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?

Comment 10

[[e-mail]]

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).

Comment 11

[[e-mail]]

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.

Comment 12

[[e-mail]]

-> gautheri

Comment 13

[Serge Gautherie (:sgautherie)]

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...

Comment 14

[[e-mail]]

Comment on attachment 135086 [details] [diff] [review]
(Cv1) 'W*T_FetchFromNetCenter()' removal
[Checked in: comment 14]

checked in.

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) 'AutoCapture' removal [Checked in: Comment 29]

Comment 16

[Serge Gautherie (:sgautherie)]

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.

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) 'IgnoreFieldNames' removal [Checked in: Comment 28]

Spun off of bug 59687 comment 22 and comment 23.

Comment 18

[Serge Gautherie (:sgautherie)]

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.

Comment 19

[Alec Flett]

Serge, please get a compiler! Install linux already.

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

Comment 20

[Serge Gautherie (:sgautherie)]

(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 21

[Alec Flett]

Comment on attachment 137752 [details] [diff] [review]
(Ev1) 'IgnoreFieldNames' removal
[Checked in: Comment 28]

sr=alecf

Comment 22

[Alec Flett]

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.

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 137752 [details] [diff] [review]
(Ev1) 'IgnoreFieldNames' removal
[Checked in: Comment 28]


approval1.7b=?:
Removal of unused code; no risk.

Comment 24

[chris hofmann]

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.

Comment 25

[Serge Gautherie (:sgautherie)]

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...)

Comment 26

[Mike Connor [:mconnor]]

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

Comment 27

[[e-mail]]

Fix checked in.

Comment 28

[Serge Gautherie (:sgautherie)]

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 }

Comment 29

[Serge Gautherie (:sgautherie)]

(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.

Comment 30

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1) <wallet.cpp> |morseAssert| removal [Checked in: Comment 35]

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 31

[Serge Gautherie (:sgautherie)]

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.

Comment 32

[Serge Gautherie (:sgautherie)]

Attachment: (Gv1) Unused |wallet.*| preference removal

(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 33

[Serge Gautherie (:sgautherie)]

Comment on attachment 146634 [details] [diff] [review]
(Gv1) Unused |wallet.*| preference removal


'r=?': (see comment 32)

Comment 34

[Mike Connor [: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 :)

Comment 35

[Serge Gautherie (:sgautherie)]

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 }

Comment 36

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1) <wallet.properties> |WalletNotification| removal [Checked in: Comment 39]

{{
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");
}}

Comment 37

[Serge Gautherie (:sgautherie)]

Attachment: (Gv1b) Unused |wallet.*| preference removal [Checked in: Comment 38]

Gv1, with comment 34 suggestion(s).

{{
http://lxr.mozilla.org/mozilla/search?string=wallet.Samples

http://lxr.mozilla.org/mozilla/search?string=wallet.Server
http://lxr.mozilla.org/mozilla/search?string=walletServer.accesskey
http://lxr.mozilla.org/mozilla/search?string=walletServer.label

http://lxr.mozilla.org/mozilla/search?string=wallet.TutorialFromMenu

http://lxr.mozilla.org/mozilla/search?string=wallet.version
}}

Comment 38

[Serge Gautherie (:sgautherie)]

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 }

Comment 39

[Serge Gautherie (:sgautherie)]

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 }

Comment 40

[Wayne Mery (:wsmwk)]

Serge, is this done?

Comment 41

[Robert Kaiser]

Serge, can this be marked FIXED?

Comment 42

[Serge Gautherie (:sgautherie)]

(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