Created attachment 280940 [details] [diff] [review] Fix v1 As an optimization especially for async providers that watch calendar prefs, there should be no notification if the calendar preferences are not changed while calling setCalendarPref
I'm not sure if this pach is correct. You can longer be sure of the observer will be called. So you don't have any 'feedback' on setting a pref (if you need it). If there are observers that need to know if the value really changed, we can add a |in boolean aChanged| parameter to onCalendarPrefSet. Another way would be to add a method onCalendarPrefChanged.
(In reply to comment #1) Right, but to me this sounds like that code (mvl is mentioning) already lacks an explicit notification mimic; relying on onCalendarPrefSet sounds rather fragile to me.
After having a bit more sleep, i'm not sure of this would really be a problem. But I still think it would be cleaner if we notify always, and add information about whether the pref really changed.
Created attachment 280999 [details] [diff] [review] Better Fix - v1 As discussed, this is a better approach. The old value is passed to the listener so that it can decide if it should act itself.
Sorry, but I am not convinced by this change; IMO the stated optimization is sensible. I'd prefer that we find and change the code that relies on those side effects, e.g. code that resets a property only to trigger a view refresh.
To make things clear, what about getting rid of onCalendarPrefSet altogether? It could be replaced by onCalendarPrefChanging or onCalendarPrefChanged.
I know I am pretty late with this, but I would really like to have either solution in 0.7. It would really simplify things for gdata. I am attaching another solution that replaces onCalendarPrefSet with onCalendarPrefChanged.
Created attachment 281143 [details] [diff] [review] onCalendarPrefChanged approach - v1
Attachment #281143 - Flags: review?(mvl)
Note I missed two files that have onCalendarPrefSet in most patches. They only have an empty function though, so its only renaming the functions. I'll do that at latest before checkin. Note also, that the only code that actually does something in onCalendarPrefSet is the calendar-management.js. There, its fine to only notify if the value changed. I think if we change the semantics this early and document correctly, then its fine to change it. http://mxr.mozilla.org/seamonkey/search?string=onCalendarPrefSet http://mxr.mozilla.org/seamonkey/search?string=setCalendarPref
I'm not in the position to grant or to deny this approval, but what I really would like to see, before anyone makes a decision here, is a short risk analysis. What is the regression risk? Which areas should we look at when looking for potential regressions?
Looking at the links I provided in c9, I don't see much of a regression risk at all. The only code that actually implements onCalendarPrefSet is calendar-management.js, which sets up the color and name of the calendar. If the information doesn't change, there is no need to update that information. My motivation for this bug is bug 392028, which without a patch like the above ends up continually committing the calendar info. I would have to check if each pref value has changed before setting, which will bloat the code a bit.
Comment on attachment 281143 [details] [diff] [review] onCalendarPrefChanged approach - v1 mvl, no hard feelings on me moving the review to daniel please. We want to spin rc1 on monday, and I'd like to have this changed for 0.7. Since there is not much code that depends on this change, I think its viable to take this patch and if needed discuss any changes after 0.7 is out of the door. If you have any suggestions tomorrow, feel free to comment.
Attachment #281143 - Flags: review?(mvl) → review?(daniel.boelzle)
Created attachment 281961 [details] [diff] [review] onCalendarPrefChanged approach - v2 Just to make sure, here an updated patch with the missing files patched.
Comment on attachment 281961 [details] [diff] [review] onCalendarPrefChanged approach - v2 r=dbo
Attachment #281961 - Flags: review?(daniel.boelzle) → review+
Checked in on HEAD and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.7
You need to log in before you can comment on or make changes to this bug.