72402 - re-do fixes to pref-cookies/images

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect)

Description

[Peter "jag" Annema]

When cookies moved to extensions/, previous fixes to pref-images.xul and
pref-cookies.xul were thrown away. Also, the old files weren't removed from
xpfe/components/prefwindow. I'll attach a patch against the current files.

Comment 1

[Peter "jag" Annema]

Attachment: [patch] re-do the changes

Comment 2

[Peter "jag" Annema]

Attachment: diff -uw between versions in prefwindow/ and cookie/

Comment 3

[Peter "jag" Annema]

The first attachment re-does the fixes for bug 64473. The second attachment
shows the changes between the versions in prefwindow/ and cookies/ due to them
being moved and some refactoring that was done. I provide it so anyone
interested can verify the patch doesn't itself cause a regression.

Comment 4

[Stephen P. Morse]

Well the reason we lost those changes is because you were making them without 
notifying me (the cookie module owner) that it was happening. ;-)

I have a couple of comments on the patches presented here:


FIRST COMMENT:
--------------

 function init()
 {
   parent.initPanel('chrome://cookie/content/pref-cookies.xul');
   var enabled = document.getElementById("networkCookieBehaviour").data != "2";
   setWarnAboutCookiesEnabled(enabled);
 }

 function setWarnAboutCookiesEnabled(aEnabled)
 {
   document.getElementById("networkWarnAboutCookies").disabled = !aEnabled;
 }

It would be cleaner to define disabled instead of enabled.  Then you would have

 function init()
 {
   parent.initPanel('chrome://cookie/content/pref-cookies.xul');
   var disnabled = document.getElementById("networkCookieBehaviour").data=="2";
   setWarnAboutCookiesEnabled(disabled);
 }

 function setWarnAboutCookiesEnabled(aDisabled)
 {
   document.getElementById("networkWarnAboutCookies").disabled == aDisabled;
 }

and similarly for images.


SECOND COMMENT:
---------------

The ordering of the radio buttons used to be always/sometimes/never.  You 
changed it to never/always/sometimes.  That's not a monotonic progression.  A 
cleaner thing would be to make it never/sometimes/always.

Comment 5

[Håkan Waara]

The reason I did never/always/sometimes was that I could put the checkbox after
the radigroup then, and it would look nicer.

where will the checkbox for sometimes gets placed if we do never/sometimes/always?

Comment 6

[Peter "jag" Annema]

> Well the reason we lost those changes is because you were making them
> without notifying me (the cookie module owner) that it was happening. ;-)

Well, I figured these files to be part of Preferences (since that's where they
were located), you didn't come to mind as module owner. So how come you moved
around old versions of these files instead of the ones on the tip?

> It would be cleaner to define disabled instead of enabled.  Then you would
> have
> ...
>   function setWarnAboutCookiesEnabled(aDisabled)

Cleaner in what sense? First of all I'd change that to be:

  function setWarnAboutCookiesDisabled(aDisabled)

and a potential call will be:

  setWarnAboutCookiesDisabled(false)

i.e. a double negative. I personally prefer to use Enabled, and hide the double
negative inside the implementation of it.


> The ordering of the radio buttons used to be always/sometimes/never.  You
> changed it to never/always/sometimes.  That's not a monotonic progression.
> A cleaner thing would be to make it never/sometimes/always.

You're absolutely right. I'll make that change and attach a new patch if you
like :-)

Comment 7

[Stephen P. Morse]

I'm a little confused by your comment.  The checkbox is valid for both the 
always and sometimes case and not valid for the never case.  I assumed that the 
reason you decided to rearrange them was because it was strange to put the 
checkbox after a case for which it does not apply (the never case).  But if you 
do never/sometimes/always, then the checkbox is after the always case for which 
it does apply.

Maybe you were confused into thinking that the checkbox does not apply for the 
always case.  It does apply there.  In fact the code that you added for 
disabling the checkbox was precisely for the never case -- you keep it enabled 
for the always and sometime case and that was the correct thing to do.

Comment 8

[Stephen P. Morse]

BTW, my comment above applies to hwaara's comment further above rather than to 
jags which immediately precedes it.  Just wanted to make sure I didn't confuse 
anybody.

Comment 9

[Håkan Waara]

sorry, i was confused.  :)

I am, as jag, interested in knowing why you replaced the tip files with older
versions? 

Comment 10

[Stephen P. Morse]

> So how come you moved around old versions of these files instead of the
> ones on the tip?

The ones I took were on the tip when I started moving them around. :-(

Comment 11

[Peter "jag" Annema]

hwaara, morse: I was about to comment on the confusion with nice little ascii
art drawings explaining things :-)

So, I agree, this looks better:

 ( ) Disable cookies
 ( ) Enable cookies for the originating web site only
 (o) Enable all cookies

 [ ] Warn me before storing a cookie

But I would rename "Enable all cookies" to "Enable cookies for all web
sites", which makes more sense imo.

Comment 12

[Stephen P. Morse]

Don't go redo-ing the wording, at least not in this bug report.  There were long 
and discussions in the past into getting this just right and I don't even recall 
all the arguments that were raised.  If you are not happy with the current 
wording, open a new bug report on that.

Comment 13

[Peter "jag" Annema]

morse: ugh. It took you over a month to get these changes checked in? Tough
process :-/

I guess I would've kept an eye on the old files, or at least do a final check
for changes to them before checking in...

Oh well, it happened, it got discovered, now let's focus on fixing this :-)

(damn mid-air collissions! ;-) )

So, no, I wasn't planning on making those wording changes a part of this bug,
I'd file a new one, but I'll go look for that lengthy discussion first now :-)

Comment 14

[Peter "jag" Annema]

Attachment: [patch] also re-order "originating host", "all"

Comment 15

[Stephen P. Morse]

r=morse

cc'ing alecf for super review

Comment 16

[Alec Flett]

sr=alecf

Comment 17

[Stephen P. Morse]

Checked in.

Comment 18

[Peter "jag" Annema]

morse: I see you fixed the patch to work in the post-(value->label) world.
However, those changes were also needed in the javascript bits and it looks like
you checked in a typo (lavel instead of label). And you've got funky indentation
(3 instead of 2). New patch coming up, reopening this bug.

Comment 19

[Peter "jag" Annema]

Attachment: [patch] Fix up stuff, take 2

Comment 20

[Peter "jag" Annema]

Oh, and pref-images.* and pref-cookies.* haven't been cvs removed from
xpfe/prefwindow/resources/ yet.

Comment 21

[Stephen P. Morse]

Oops, sorry about the "lavel".

r=morse

Comment 22

[Stephen P. Morse]

alecf, we need another super-review here.

Comment 23

[Stephen P. Morse]

alecf:  Never mind the sr.  jag just pointed out that the latest patch is simply 
the original patch as it was intended to be so your original sr= is still valid.

Revised patch checked in.

Comment 24

[Tom Everingham]

verified 7/10/01 builds