Closed Bug 89576 Opened 23 years ago Closed 21 years ago

Widget state manager should save current page data before OK callback

Categories

(SeaMonkey :: Preferences, defect, P3)

x86
Windows 95
defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.7alpha

People

(Reporter: neil, Assigned: neil)

Details

(Whiteboard: [need info])

Attachments

(1 file, 2 obsolete files)

Many pref windows register OK callbacks. Some blindly call
document.getElementById while other wiser codes fall back on the wsm page data.
If the wsm could be relied upon to save the page data when OK was clicked then
all pref panels could retrieve their data in a consistent manner. Unfortunately
the wsm saves the page data after calling the OK callbacks...
Attached patch proposed patch (obsolete) — Splinter Review
Keywords: patch, review
->ben? samir's away on sabbatical...
Assignee: sgehani → ben
Ben, if you're not going to review this can you suggest another name? Thanks.
Neil, at first glance your suggestion seems reasonable, but the FE code isn't
really my area. One thing that comes to mind is this...

The way it works right now the OK callbacks could potentially validate the page
data before the WSM saved it. With your change this data would be saved before
the callbacks could have a chance to valdidate it.

Eddy, do you have any thoughts on the potential impact of this change?
Good point about the validation.  I don't see any WSM handled pref panels that
do validation at this point.

I don't see any problems with the patch.  However, shouldn't the few panels that
use an OK handler and use the getElementById calls be changed to be consistent?

Another thing that needs to be done is documenting the preferred usage for
nsPrefWindow and WSM.  I'm working on this (on and off) and hope to have a rough
cut fairly soon.

Neil, would you be interested in helping with this task?


In reply to Brian Nesse's question, the WSM saves data across page switches.
If the page data validates then it will need to be saved anyway.
If the page data does not validate then it will still be saved on the next switch.
Only if the page validates will the WSM write the saved data to your preferences.
I will see if I can find all the pages that use validation.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0
These files currently install an OK callback:
pref-advanced.xul
pref-appearance.xul
pref-fonts.js
pref-history.xul
pref-winhooks.xul
pref-mailnewsOverlay.js

Note that pref-mailnewsOverlay.js gets it wrong!
-> .9.8 because there's a patch
Target Milestone: mozilla1.0 → mozilla0.9.8
Attached patch also fix "bugs" in ok handlers (obsolete) — Splinter Review
nsPrefWindow.js: move callbacks to after page data save
pref-appearance.xul: always get pref value from page data
pref-fonts.js: don't save page data manually
pref-history.xul: always get pref value from page data
pref-winhooks.js: don't save page data manually
pref-contentpacks.xul: always get pref value from page data
pref-mailnewsOverlay.js: fix bug by getting pref value from page data
Attachment #41405 - Attachment is obsolete: true
Can you explain what your latest patch does?
Ben, the problem is obtaining page values during the OK callback, as the current
page may not be loaded. Three pref panels try to get the values from the current
page and fall back on the saved data. Two pref panels manually save the page
data anyway. One pref panel just gets it wrong. The idea of my patch is to save
the current page data before calling the OK callbacks, thus simplifying data access.
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Target Milestone: mozilla0.9.9 → mozilla1.0
Comment on attachment 60475 [details] [diff] [review]
also fix "bugs" in ok handlers

sr=blake, but please test well
Attachment #60475 - Flags: superreview+
Actually, I take that back. Maybe.  The point of the ok callbacks before the
data save is presumably so that panels can do any necessary validation or
canonification before saving.  I do agree that we need a cleaner solution so
each panel doesn't have to hack to get the right value, but I'm not sure this is it.
Blake, the idea is that I use the same save routine as if you change page, so
that the page can always assume that it's not the current page.
ADT triage team needs info: what bad things happen to the user that would
require fixing this for MachV?
Whiteboard: [need info]
None.
Even pref-mailnewsOverlay.js has his own hack now :-)
nsbeta1-
Keywords: nsbeta1nsbeta1-
Target Milestone: mozilla1.0 → mozilla1.1
So, there are now only three issues with OK handlers left:
pref-contentpacks.xul still tries to query the current document
pref-fonts.js and pref-winhooks.js try to save the data twice
(this is quite slow in the case of pref-fonts.js)

FYI attachment 41405 [details] [diff] [review] got checked in by timeless as part of another patch.
Attachment #60475 - Attachment is obsolete: true
Attachment #136197 - Flags: superreview?(alecf)
Attachment #136197 - Flags: review?(caillon)
Comment on attachment 136197 [details] [diff] [review]
Updated for bitrot ;-)

wow, those were some ugly hacks. good riddens!

sr=alecf
Attachment #136197 - Flags: superreview?(alecf) → superreview+
Comment on attachment 136197 [details] [diff] [review]
Updated for bitrot ;-)

spiffy.  r=caillon
Attachment #136197 - Flags: review?(caillon) → review+
Unless anyone has a dire need to save a few milliseconds this can wait :-)
Assignee: bugs → neil.parkwaycc.co.uk
Status: ASSIGNED → NEW
Target Milestone: mozilla1.1alpha → mozilla1.7alpha
Fix checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: