Closed Bug 221078 Opened 21 years ago Closed 21 years ago

unfork firebird/mozilla cookie prefs

Categories

(Core :: Networking: Cookies, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: dwitte, Assigned: dwitte)

References

Details

Attachments

(2 files)

this is a spinoff of bug 170705 (i'm cc'ing the folks that were listed on that bug). we really need to fix the forkage between firebird and mozilla in cookies and nsGlobalWindow.cpp (see http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#6062). having #ifdef MOZ_PHOENIX in these places is completely bogus, and prevents them from becoming part of the GRE. (cookies will be moving into necko, and thus the GRE, very soon). there's absolutely no need for these prefs to be forked, apart from the fact that it correlates well with the respective UI's of the two products. so the simplest thing to do is just unfork them. there are three prefs involved: firebird has network.cookie.enable and network.cookie.enableForOriginatingWebsiteOnly, both booleans; whereas mozilla has network.cookie.cookieBehavior, an integer pref which is a superset of the two firebird prefs. (firebird has two boolean checkboxes, whereas mozilla has a set of radiobuttons). unforking the prefs will break one of the products (those cookie prefs will be lost, and the user will have to set them again). it's easier to break firebird. depending on how the firebird folk feel about this, we can introduce migration code to convert the prefs. in addition, we will need to introduce a tad of prefpanel code to munge the two UI checkbox boolean values into one integer value. comments on this proposal would be much appreciated.
Ben should probably be in on this discussion, as should some SeaMonkey ui people...
Attached patch unfork backend prefs (landed) (deleted) — Splinter Review
this unforks the firebird "network.cookie.enabled" and "network.cookie.enabledForOriginatingWebsiteOnly" prefs, and replaces them with the seamonkey "network.cookie.cookieBehavior" integer pref. together with some patches in other bugs, this will solve all cookie-related forkage for necko and the GRE.
so is there a bug on fixing firebird's UI?
this fixes the firebird prefpanel bustage resulting from unforking the prefs. i'm not going to push this patch, because i don't have the authority to... it's really up to the firebird people as to how they want to resolve it. it's just what i think is the best solution. i've tested it, and it appears to work fine. the patch preserves firebird's checkbox layout for the "enable cookies" and "enable cookies for originating website" options, and then munges those two booleans into an integer for the "network.cookie.cookieBehavior" pref. from what has been explained to me, the easiest way to do this is to create a "display:none" set of radioboxes that actually interface to that pref, and then have the checkboxes update those radioboxes when they change. this leverages prefpanel code to preserve those values when another panel is selected. (just having the checkboxes call a function to update the pref, and not having the non-displayed radioboxes, won't work). there is also a few lines of code to delete the old firebird prefs (this is optional). of course, the user will still lose their old cookie prefs with this patch, but it preserves UI behavior and keeps the pref database tidy.
>so is there a bug on fixing firebird's UI? sure, why not do that here?
Attachment #132903 - Flags: superreview?(darin)
Comment on attachment 132903 [details] [diff] [review] unfork backend prefs (landed) r=caillon, but per our discussion move the define in globalwindow outside of the method somewhere? And tack on braces on the if to follow the preferred style in the module.
Attachment #132903 - Flags: review+
> i'm not going to push this patch I will! :o) Thanks dan for the patch! dwitte, could you provide a new patch to address caillon's review? darin, could you please sr? :op
no need to post a new patch just for those nitfixes... so the sr request still stands. ben goodger said he was ok with the firebird changes (but don't hold me to that). thanks for the help pch :)
Pch, I think the firebird patch could probably land before the backend patch without breaking things. Which might be prudent if you can arrange it.
Comment on attachment 132903 [details] [diff] [review] unfork backend prefs (landed) yeah baby! sr=darin
Attachment #132903 - Flags: superreview?(darin) → superreview+
Comment on attachment 132903 [details] [diff] [review] unfork backend prefs (landed) this has landed (with nitfixes). ben/pch, i suggest you check in the firebird patch sometime soon ;)
Attachment #132903 - Attachment description: unfork backend prefs → unfork backend prefs (landed)
Great! I'll take care of the Firebird part. A big thank you to Dan and the reviewers for removing this BIG nonsense!
no problem. donations of beer accepted.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
*** Bug 222599 has been marked as a duplicate of this bug. ***
I checked in the Firebird part after having to reinstall my distro, build environment etc... I used a <data/> element instead of a <radiogroup/> to store the pref value. I also refactored the code to clearly distinguish the update of the broadcaster and the pref value (then avoiding an unnecessary call). Some renaming and other nits. This patch also migrate the pref so that new users won't lose their preferences.
Should Firebird's other two boolean cookie prefs be folded into the same new int pref? Such change seems a logical extension to the current change.
nice work pch! i learnt some new xul today. ;) walter: no. there's no point in changing the other prefs. they're fine the way they are
dwitte: you're welcome! Beers are waiting in the fridge. But hurry up, they won't last... :)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: