89576 - Widget state manager should save current page data before OK callback

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect, P3)

Description

[[e-mail]]

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

Comment 1

[[e-mail]]

Attachment: proposed patch

Comment 2

[sairuh (rarely reading bugmail)]

->ben? samir's away on sabbatical...

Comment 3

[[e-mail]]

Ben, if you're not going to review this can you suggest another name? Thanks.

Comment 4

[Brian Nesse (gone)]

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?

Comment 5

[eddyk]

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?

Comment 6

[[e-mail]]

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.

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 41405 [details] [diff] [review]
proposed patch

r=[e-mail]

Comment 8

[[e-mail]]

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!

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

-> .9.8 because there's a patch

Comment 10

[[e-mail]]

Attachment: also fix "bugs" in ok handlers

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

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

Can you explain what your latest patch does?

Comment 12

[[e-mail]]

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.

Comment 13

[Blake Ross]

Comment on attachment 60475 [details] [diff] [review]
also fix "bugs" in ok handlers

sr=blake, but please test well

Comment 14

[Blake Ross]

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.

Comment 15

[[e-mail]]

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.

Comment 16

[Peter Trudelle]

ADT triage team needs info: what bad things happen to the user that would
require fixing this for MachV?

Comment 17

[Blake Ross]

None.

Comment 18

[[e-mail]]

Even pref-mailnewsOverlay.js has his own hack now :-)

Comment 19

[Peter Trudelle]

nsbeta1-

Comment 20

[[e-mail]]

Attachment: Updated for bitrot ;-)

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.

Comment 21

[Alec Flett]

Comment on attachment 136197 [details] [diff] [review]
Updated for bitrot ;-)

wow, those were some ugly hacks. good riddens!

sr=alecf

Comment 22

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 136197 [details] [diff] [review]
Updated for bitrot ;-)

spiffy.  r=caillon

Comment 23

[[e-mail]]

Unless anyone has a dire need to save a few milliseconds this can wait :-)

Comment 24

[[e-mail]]

Fix checked in.

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

this checkin caused Bug #235058