Closed
Bug 221078
Opened 21 years ago
Closed 21 years ago
unfork firebird/mozilla cookie prefs
Categories
(Core :: Networking: Cookies, defect)
Core
Networking: Cookies
Tracking
()
RESOLVED
FIXED
People
(Reporter: dwitte, Assigned: dwitte)
References
Details
Attachments
(2 files)
(deleted),
patch
|
caillon
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Comment 1•21 years ago
|
||
Ben should probably be in on this discussion, as should some SeaMonkey ui people...
Assignee | ||
Comment 2•21 years ago
|
||
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.
Comment 3•21 years ago
|
||
so is there a bug on fixing firebird's UI?
Assignee | ||
Comment 4•21 years ago
|
||
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.
Assignee | ||
Comment 5•21 years ago
|
||
>so is there a bug on fixing firebird's UI?
sure, why not do that here?
Assignee | ||
Updated•21 years ago
|
Attachment #132903 -
Flags: superreview?(darin)
Comment 6•21 years ago
|
||
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+
Comment 7•21 years ago
|
||
> 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
Assignee | ||
Comment 8•21 years ago
|
||
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 :)
Comment 9•21 years ago
|
||
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 10•21 years ago
|
||
Comment on attachment 132903 [details] [diff] [review]
unfork backend prefs (landed)
yeah baby! sr=darin
Attachment #132903 -
Flags: superreview?(darin) → superreview+
Assignee | ||
Comment 11•21 years ago
|
||
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)
Comment 12•21 years ago
|
||
Great!
I'll take care of the Firebird part.
A big thank you to Dan and the reviewers for removing this BIG nonsense!
Assignee | ||
Comment 13•21 years ago
|
||
no problem. donations of beer accepted.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•21 years ago
|
||
*** Bug 222599 has been marked as a duplicate of this bug. ***
Comment 15•21 years ago
|
||
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.
Comment 16•21 years ago
|
||
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.
Assignee | ||
Comment 17•21 years ago
|
||
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
Comment 18•21 years ago
|
||
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.
Description
•