Should try/catch behavior be changed on a frozen interface?
Mike Kaply
https://bugzilla.mozilla.org/show_bug.cgi?id=487059
was checked in which changed the way clearUserPref works. Previously,
it would throw when there was no pref, now it simply returns silently.
Since nsIPrefBranch.idl is a frozen interface, should this change have
been made to the existing interface?
This seems wrong.
Mike Kaply
Mike Beltzner
> Since nsIPrefBranch.idl is a frozen interface, should this change have
> been made to the existing interface?
What does it mean for it to be a frozen interface? Do you mean that this
should have come with a UUID change?
The change was landed before Firefox 3.6 Beta 1, which makes interface
changes fair game as far I as I understand things.
cheers,
mike
Gijs Kruitbosch
I was under the impression *frozen* interfaces didn't change at all...
http://www.mozilla.org/projects/embedding/Mozilla_API_freezing.html
seems to back up that notion:
The frozen nature of an API crosses all source tree branches based off of the
branch (or trunk) on which the API was frozen, and beyond. This level of
granularity guarantees backwards compatibility (which insulates a consumer from
updating their own integration points in their code).
(seeing as nsIPrefBranch is frozen on trunk)
I also thought that that's why we have 'silly' things like nsIIOService2. What
did I miss?
~ Gijs
PS: ChatZilla actually relies on this throwing behaviour, as of recently. There
is no other reliable way to tell that a pref still exists, as far as I know.
Gijs Kruitbosch
> PS: ChatZilla actually relies on this throwing behaviour, as of
> recently. There is no other reliable way to tell that a pref still
> exists, as far as I know.
Eh, scratch that, I misremembered. We use getCharPref.
~ Gijs
Neil
And there's also prefHasUserValue.
--
Warning: May contain traces of nuts.
Mike Shaver
> On 10/27/2009 6:58 PM, Mike Kaply wrote:
>>
>> been made to the existing interface?
>
> should have come with a UUID change?
Frozen interfaces shouldn't change at all until a major-number (2.0 --
or, as I secretly hope, 17.0) bump in Gecko version. They are, like
my love for heist films, basically forever.
I agree that we should preserve the prefBranch interface completely,
incredibly painful and crappy though it is.
Mike
belt...@mozilla.com
released today and promoted to add-on developers as the version upon
which to base their compatibility.
Is the implication that we need to fix this before we go to beta?
cheers,
mike
On 2009-10-28, at 8:06, Mike Shaver <mike....@gmail.com> wrote:
> On Wed, Oct 28, 2009 at 1:44 AM, Mike Beltzner
> <belt...@mozilla.com> wrote:
>> On 10/27/2009 6:58 PM, Mike Kaply wrote:
>>>
>>> Since nsIPrefBranch.idl is a frozen interface, should this change
>>> have
>>> been made to the existing interface?
>>
Benjamin Smedberg
> Frozen interfaces shouldn't change at all until a major-number (2.0 --
> or, as I secretly hope, 17.0) bump in Gecko version. They are, like
> my love for heist films, basically forever.
>
> I agree that we should preserve the prefBranch interface completely,
> incredibly painful and crappy though it is.
FWIW, this change doesn't affect the binary ABI of the interface, it only
affects the behavior of whether an exception nsresult is thrown. I would not
rev the UUID under normal (non @status FROZEN) circumstances.
I believe we should leave this change in (and fix the documentation as
appropriate): it is more intuitive to have clearUserPref silently succeed
when there is no user pref than to have it throw, and the compatibility
constraints seem minor to me.
mkaply, how exactly did your extension code break? Was it relying on the
exception in a try/catch block to do some other work only if a user pref was
not set? How does this cause user-visible behavior changes?
--BDS
Mike Shaver
<benj...@smedbergs.us> wrote:
> mkaply, how exactly did your extension code break? Was it relying on the
> exception in a try/catch block to do some other work only if a user pref was
> not set? How does this cause user-visible behavior changes?
I think it's relatively common for people to have the "default pref"
behaviour be in a catch block, due to scar tissue from this bogus API.
Mike
Boris Zbarsky
> What does it mean for it to be a frozen interface?
Generally speaking, a frozen interface has meant that code written
against that interface will continue to work exactly the same way,
without recompiling or source-level changes, in the future, even across
major version changes in Gecko. At least for the things documented in
the interface. It's a pretty sweeping promise.
For cases like this, where the behavior was not documented, the
assumption has generally been that the behavior should be preserved.
This has been the case even in situations where the behavior is totally
nonsensical (e.g. some of the nsIFile per-platform differences, or even
divergence from the documented behavior).
> Do you mean that this should have come with a UUID change?
No, it means this change probably should not have happened at all.
> The change was landed before Firefox 3.6 Beta 1, which makes interface
> changes fair game as far I as I understand things.
Interface changes to unfrozen interfaces, yes.
-Boris
Boris Zbarsky
> Thanks for the context. Now, this went in to the beta which is to be
> released today and promoted to add-on developers as the version upon
> which to base their compatibility.
>
> Is the implication that we need to fix this before we go to beta?
If our beta promise is effectively the equivalent of the frozen
interface promise ("code written against beta will be guaranteed to work
against final"), then yes.
-Boris
Dao
> Since nsIPrefBranch.idl is a frozen interface, should this change have
> been made to the existing interface?
The interface hasn't been touched, since it didn't specify the old behavior.
Mike Kaply
What shaver says later.
I was relying on the catch to tell me the pref didn't exist.
Checking for prefHasUserValue and then doing clearUserPref seemed
redundant when I knew that clearing it would work no matter what.
In my case, I was looping through a set of numbered prefs
(.pref1, .pref2, .pref3, ...) expecting clearUserPref to fail when
there were no more.
I fixed my code, but I'm concerned other folks might hit this without
knowing by using this to test for the existence of a pref.
This is another case where searching through the source of add-ons on
AMO might be useful...
Mike Kaply
Mike Beltzner
> If our beta promise is effectively the equivalent of the frozen
> interface promise ("code written against beta will be guaranteed to
> work against final"), then yes.
I've heard conflicting things, here. There's general agreement that
changing the interface is a no-no, but not on whether or not we should
care that much about it.
If we need to hold and respin the beta, I need to know today.
cheers,
mike
Mike Shaver
> If we need to hold and respin the beta, I need to know today.
I think -- god, I hate myself for saying this -- that we need to
respin the beta.
Mike
(fffffffffffffffffffffff)
Boris Zbarsky
Freezing an interface implies promises of forward-compatibility, which
includes preserving even unspecified behavior.
That's why we stopped doing it.
-Boris
Mike Beltzner
> I think -- god, I hate myself for saying this -- that we need to
> respin the beta.
Would people be OK to carry over QA results after we make this single
change? I'd think that should be fine, and will likely result in the
beta only being delayed by a day if we hurry.
cheers,
mike
Mike Shaver
We should MXR for the interface calls in question, to see if anything
there breaks if we go back to throwing, and we should probably retest
the pref window and maybe migration code?
mike
Serge Gautherie
> We should MXR for the interface calls in question, to see if anything
> there breaks if we go back to throwing, and we should probably retest
> the pref window and maybe migration code?
I expect at least some recent tests to fail in m-c and c-c, due to
"missing" try+catch (compared to m-1.9.1 and c-1.9.1) !
Some actual application(s) code may be in the same case...
Dao
> On Wed, Oct 28, 2009 at 8:47 AM, Benjamin Smedberg
> <benj...@smedbergs.us> wrote:
>> mkaply, how exactly did your extension code break? Was it relying on the
>> exception in a try/catch block to do some other work only if a user pref was
>> not set? How does this cause user-visible behavior changes?
>
We (Benjamin, me) were aware of this use case, but it wasn't considered
common. I don't think a single extension changes this picture. (Although
it should be noted that there's no default pref behavior in mkaply's
extension's case.)
Boris Zbarsky
> We (Benjamin, me) were aware of this use case, but it wasn't considered
> common.
I think the point is that for frozen interfaces our bar has generally
been a lot higher than "people don't depend on this much", since the
promise isn't "you won't have to change much to be compatible" but "you
won't have to change at all to be compatible".
-Boris
Mike Kaply
> On 28.10.2009 13:52, Mike Shaver wrote:
>
> > On Wed, Oct 28, 2009 at 8:47 AM, Benjamin Smedberg
> >> mkaply, how exactly did your extension code break? Was it relying on the
> >> exception in a try/catch block to do some other work only if a user pref was
> >> not set? How does this cause user-visible behavior changes?
>
> > have the "default pref" behaviour be in a catch block,
>
> We (Benjamin, me) were aware of this use case, but it wasn't considered
> common. I don't think a single extension changes this picture. (Although
> it should be noted that there's no default pref behavior in mkaply's
> extension's case.)
I don't deny that my extension was doing the wrong thing. I've
corrected it.
The core issue here is if changing the throw behavior was violating
the fact that the interface was frozen.
There's no way to know if this is a "common" case or not.
We all agree that the interface is better this way. But there might be
extensions that break because of the change.
Mike
Mike Shaver
> We all agree that the interface is better this way. But there might be
> extensions that break because of the change.
I'm certain there would be -- catch-for-default is a longstanding
pattern in Mozilla and related code, and very widely copied and
pasted.
Mike
Dao
At least not in mozilla-central, as I reviewed all uses in bug 487059.
Dao
> The core issue here is if changing the throw behavior was violating
> the fact that the interface was frozen.
I guess exception behavior doesn't need to be specified, but since the
legacy implementation did it in an unintuitive manner, I tend to think
that it was violating the interface. The basis for me filing and fixing
that bug was that the exception repeatedly surprised new developers and
that it was neither specified nor documented.
Mike Shaver
To be clearer, perhaps, it is very much an improvement to the
interface. It's just not a *compatible* improvement to the interface,
so we can't take it this way right now.
Mike
Shawn Wilsher
> To be clearer, perhaps, it is very much an improvement to the
> interface. It's just not a *compatible* improvement to the interface,
> so we can't take it this way right now.
Cheers,
Shawn
Mike Kaply
That surprises me since try/catch is so pervasive in the preferences
code especially.
Should we be doing more to encourage add-on developers to use FUEL or
something else?
Or maybe it's time for some to implement
nsIPrefBranchThatDoesntSuck? :)
Robert Kaiser
> On Wed, Oct 28, 2009 at 11:48 AM, Mike Beltzner<belt...@mozilla.com> wrote:
>> If we need to hold and respin the beta, I need to know today.
>
> I think -- god, I hate myself for saying this -- that we need to
> respin the beta.
And actually for making code worse, which is really a pity :(
Robert Kaiser
gavin
reverting this change. We made the change on trunk six and a half
months ago, before we branched. So far we've only heard of one addon
that was doing something highly unusual (resetting several sequential
prefs, and needing to know how many were reset, AIUI), and it's
already been fixed to use existing alternate methods. The "default
behavior on exception" pattern with the get*Pref methods *is* common,
and we certainly couldn't afford to break that, but this use (wanting
to know about the existence of a preference whose value you're
clobbering) is quite unlikely to be widespread, in my opinion. Even if
it was, there are other ways to achieve the same effect in a more
obvious way with the existing APIs (prefHasUserValue).
Given that, my suspicion is that that reverting this change is
probably more trouble than it's worth... Reverting the change in our
own code on trunk is proving to be a pain. We're essentially at the
point where we're sprinkling around try/catches or prefHasUserValue
checks indiscriminately, and while there's very little risk of
changing the code logic, the end result is several large patches that
touch a lot of files, are potentially introducing redundant code, and
are not easy to prove sufficient. We also end up stuck with the
previous sub-optimal behavior.
Arguing to keep a potentially incompatible change is not really an
easy position to be in - I wish measuring the compatibility cost here
was easier than it is, and that I didn't have to rely on opinions and
guesses. I did look through uses of clearUserPref for 20 or so addons
on the out-of-date MXR instance timeless runs, and didn't find any
that relied on clearUserPref throwing, but that's hardly conclusive,
and auditing that entire dataset is not really feasible in a useful
timeframe.
Worst case, what do we lose by releasing the beta without this
backout? Some addons bustage, perhaps, but I don't think I'm wrong
about that being unlikely, and for a beta release, that doesn't sound
so horrible...
Gavin
Mike Beltzner
> I think we are probably overestimating the compatibility benefits to
> reverting this change. We made the change on trunk six and a half
> months ago, before we branched. So far we've only heard of one addon
> that was doing something highly unusual (resetting several sequential
> prefs, and needing to know how many were reset, AIUI), and it's
To date only 34% of the Add-ons (that are hosted on AMO) have been
marked by authors as being compatible with Firefox 3.6. Based on what we
know from previous product releases, a large number of Add-on developers
don't start updating/checking for compatibility until we make assurances
that we're not going to make more changes that would require them to do
the work again. We're planning on making this pledge with the beta
release of Firefox 3.6.
I believe that this means we have no good way of estimating how many
Add-ons will be affected by this change. There are steps we can (and
should) take to investigate, of course.
However, as I understand things, there's another argument at stake here
which is the assurances we make with @frozen interfaces. I lack the
historical context here, but from what I understand, that marking
indicates that Add-on and other platform consumers can be assured that
until a major Gecko version change, the behaviour and interface will not
change. We changed the behaviour, and the fact that Kaply was the first
to notice doesn't mean that he was the only one affected.
Shaver and others have indicated that the code pattern Kaply was using
had been common in our code, and was likely copied and pasted to other
people's code. Are you saying that's not the case?
> already been fixed to use existing alternate methods. The "default
> behavior on exception" pattern with the get*Pref methods *is* common,
> and we certainly couldn't afford to break that, but this use (wanting
> to know about the existence of a preference whose value you're
> clobbering) is quite unlikely to be widespread, in my opinion. Even if
> it was, there are other ways to achieve the same effect in a more
> obvious way with the existing APIs (prefHasUserValue).
Interesting; so you're saying that the behavioural change isn't likely
to be damaging to the pattern that may have been copied by others?
> Given that, my suspicion is that that reverting this change is
> probably more trouble than it's worth... Reverting the change in our
> own code on trunk is proving to be a pain. We're essentially at the
> point where we're sprinkling around try/catches or prefHasUserValue
> checks indiscriminately, and while there's very little risk of
> changing the code logic, the end result is several large patches that
> touch a lot of files, are potentially introducing redundant code, and
> are not easy to prove sufficient. We also end up stuck with the
> previous sub-optimal behavior.
>
> Arguing to keep a potentially incompatible change is not really an
> easy position to be in - I wish measuring the compatibility cost here
> was easier than it is, and that I didn't have to rely on opinions and
> guesses. I did look through uses of clearUserPref for 20 or so addons
> on the out-of-date MXR instance timeless runs, and didn't find any
> that relied on clearUserPref throwing, but that's hardly conclusive,
> and auditing that entire dataset is not really feasible in a useful
> timeframe.
It might be a cost we have to bear. I want to keep this from being a
conversation about whether or not we need to adhere to @frozen notation,
as while that's germane to this issue, exceptions can be made to any
rule with good reason.
Dolske framed it well, I think. There are three questions:
1. Will Add-on authors need to do some compatibility testing, and will
they need to make changes based on functional or interface changes?
2. Is this specific change riskier/more damaging to Add-ons than others
we've made?
3. Have we ever changed an @frozen API before?
I think the answers are yes, no, and no, respectively. Not sure what
that means. I think it means that we can make the change, but need to
communicate strongly about why we're making an exception.
> Worst case, what do we lose by releasing the beta without this
> backout? Some addons bustage, perhaps, but I don't think I'm wrong
> about that being unlikely, and for a beta release, that doesn't sound
> so horrible...
We lose the most if we don't back out this change for beta and then
decide that we have to back it out before we release Firefox 3.6 based
on how it affects other Add-ons. That's why I think we need to really
understand the potential bustage that it can introduce.
cheers,
mike
gavin
> I believe that this means we have no good way of estimating how many
> Add-ons will be affected by this change. There are steps we can (and
> should) take to investigate, of course.
I can't think of any ways to better estimate this without auditing a
representative sample of addon code for calls to this method, and I'm
not sure that's feasible in a useful time frame.
> However, as I understand things, there's another argument at stake here
> which is the assurances we make with @frozen interfaces. I lack the
> historical context here, but from what I understand, that marking
> indicates that Add-on and other platform consumers can be assured that
> until a major Gecko version change, the behaviour and interface will not
> change. We changed the behaviour, and the fact that Kaply was the first
> to notice doesn't mean that he was the only one affected.
I think - and I'm sure this is not controversial - is that we should
only focus on violations of the rule that matter in practice. My
argument, based on an understanding of the code pattern and a guess of
the likelihood of it being used, is that this one doesn't.
> Shaver and others have indicated that the code pattern Kaply was using
> had been common in our code, and was likely copied and pasted to other
> people's code. Are you saying that's not the case?
As far as I know, that's not the case - mkaply's extension and a still-
broken unused file in the tree[1] are the only pieces of code that
relied on this interface quirk.
Unfortunately, it appears that the old behavior was documented on the
interface [2], which doesn't really decrease the odds of other addons
being affected. This weakens my argument somewhat, but I don't think
it does so significantly - others may disagree.
[1] http://hg.mozilla.org/mozilla-central/annotate/79f7ceab9437/layout/tools/tests/regression_tests.js#l96
[2] http://mxr.mozilla.org/mozilla-central/source/modules/libpref/public/nsIPrefBranch.idl#207
> We lose the most if we don't back out this change for beta and then
> decide that we have to back it out before we release Firefox 3.6 based
> on how it affects other Add-ons.
What do we actually lose in that scenario? Some code churn post-beta,
I suppose, but it's the same code churn we're thinking of taking now,
at the last minute before b1. I can see large compatibility issues
negatively affecting the effectiveness of the beta in terms of testing
coverage and exposure, but my argument hinges on the fact that I think
that is incredibly unlikely to occur due to this change specifically.
Gavin
John J Barton
> On 10/28/2009 8:59 PM, gavin wrote:
>> Worst case, what do we lose by releasing the beta without this
>> backout? Some addons bustage, perhaps, but I don't think I'm wrong
>> about that being unlikely, and for a beta release, that doesn't sound
>> so horrible...
>
> We lose the most if we don't back out this change for beta and then
> decide that we have to back it out before we release Firefox 3.6 based
> on how it affects other Add-ons. That's why I think we need to really
> understand the potential bustage that it can introduce.
Just to point out: this is one potential bustage you know about. What
about the ones you don't know? Only way to find out is b1. I think you
should ship what you have. If this is the only problem, create b2 soon.
If you find other problems, create b2 soon.
jjb
Mike Beltzner
> What do we actually lose in that scenario? Some code churn post-beta,
> I suppose, but it's the same code churn we're thinking of taking now,
> at the last minute before b1. I can see large compatibility issues
> negatively affecting the effectiveness of the beta in terms of testing
> coverage and exposure, but my argument hinges on the fact that I think
> that is incredibly unlikely to occur due to this change specifically.
It's hard messaging. We're telling add-on authors that they can update
to compatibility with Firefox 3.6 Beta and then they'll be set for
Firefox 3.6. So I'd rather we not have that churn, basically. The cost
in terms of our ability to release by the end of this calendar year is
quite high.
cheers,
mike
gavin
> It's hard messaging. We're telling add-on authors that they can update
> to compatibility with Firefox 3.6 Beta and then they'll be set for
> Firefox 3.6. So I'd rather we not have that churn, basically. The cost
> in terms of our ability to release by the end of this calendar year is
> quite high.
I think that addon authors can update to compatibility with 3.6 beta
and they'll be set for 3.6, whether we take this change or not.
Sure it would suck if this issue ended up breaking many addons, but
that risk is inherent in shipping a beta, and in my opinion this
change is nowhere near the top of the list of potential addon
compatibility breakers.
I don't know that I'm managing to convince anyone, here, so I guess
I'll work on reviewing the patches in bug 524995 in case we do decide
to proceed with it.
Gavin
Mike Beltzner
> and they'll be set for 3.6, whether we take this change or not.
Yes, that's right. What I said was that changing it, then changing it
back, would be the absolute worst case scenario is. You'll find I'm
pretty agnostic about what the right thing to do is, here. I can only
facilitate this decision and make a call as a driver based on the
information to hand.
> I don't know that I'm managing to convince anyone, here, so I guess
> I'll work on reviewing the patches in bug 524995 in case we do decide
> to proceed with it.
That would be appreciated. I'm really waiting for Kaply, Shaver and
Boris to weigh in, here.
cheers,
mike
Boris Zbarsky
> Just to point out: this is one potential bustage you know about. What
> about the ones you don't know? Only way to find out is b1. I think you
> should ship what you have. If this is the only problem, create b2 soon.
> If you find other problems, create b2 soon.
John, we're promising no breaking changes after b1. Reverting this is a
breaking change. So...
-Boris
John J Barton
Ah, ok, next time don't promise just ship. You need to make less of a
deal out of betas if you are compressing the schedule. Unlike full
release users, beta users love updates.
jjb
Boris Zbarsky
> Ah, ok, next time don't promise just ship. You need to make less of a
> deal out of betas if you are compressing the schedule. Unlike full
> release users, beta users love updates.
It's not about users, it's about extension authors. They seem to hate
updates, or rather they hate having to update their extension. Which is
perfectly understandable.
-Boris
Boris Zbarsky
> That would be appreciated. I'm really waiting for Kaply, Shaver and
> Boris to weigh in, here.
So.... my problem is I don't have a good feel for the size of the
patches dealing with this. What I _do_ know:
1) The API is frozen and has been since October 2001.
2) The function has been documented in the interface as having
this behavior since October 2001 (same checkin, even!).
3) We have no way to tell what binary extensions (remember, the ones
that we have this whole frozen API rigmarole for in the first
place?) are using this function.
4) We know the change shouldn't have happened given items 1 and 2.
5) We know the change broke at least one extension so far, before
the major compatibility update push has happened.
6) Breakage would most likely be somewhat subtle and not easy to spot.
If we weren't trying to ship the beta ASAP this would be a no-brainer to
back out, in my opinion.
If we ship this way and extensions are written on top of it and then we
decide to back it out, that's a much bigger deal, I think, since
breakage for extensions written to the new model would be much more
serious (exceptions thrown when none are expected).
I agree with gavin that chances are we don't have that high a fraction
of our extensions using this function, if only because extensions tend
to not be very good at cleaning up their prefs.
I did just go look at the patches, and it seems there are three so far:
1) A backout of the original landing (should be safe, I'd think!).
2) A whole bunch of changes to test-only code that's been written
since the landing.
3) One EM callsite that testing found.
Is that about right?
-Boris
Shawn Wilsher
> Is that about right?
We think that is about right, but (AFAIK) we are still waiting on try
server results to make sure that Gijs got everything. We'll probably
have to sift through a bunch of random orange too.
Cheers,
Shawn
Boris Zbarsky
> We think that is about right, but (AFAIK) we are still waiting on try
> server results to make sure that Gijs got everything. We'll probably
> have to sift through a bunch of random orange too.
Hmm. Have we done a diff from the landing of the change in question to
now and grepped it for calls to clearUserPref being added? That might
be a more effective way to find relevant callsites than bouncing things
off of try....
That said, my point was that while gavin is right that the patches are
nontrivial, they don't seem _dangerous_ in terms of code we ship to me
so far. Most of the change is backout of the original landing and
changes to test code.
-Boris
Shawn Wilsher
> Hmm. Have we done a diff from the landing of the change in question to
> now and grepped it for calls to clearUserPref being added? That might be
> a more effective way to find relevant callsites than bouncing things off
> of try....
However, that code was in the tree for over 4 months, so it's possible
there is some interaction that just isn't obvious via code inspection.
Cheers,
Shawn
Gijs Kruitbosch
Yes, that was done, it resulted in the big patch which touches a bunch of tests
(but nothing else, apparently). The plain backout and associated non-test code
fixes were smaller and reviewed separately.
Lacking any "decision" here, I've stuck the combination of patches on tryserver
a second time (on 1.9.2). If you want me to actually push the changes somewhere,
let me know - I considered pushing to trunk, but the sheriff suggested
try-server first, so I've gone with that for now.
~ Gijs
Neil
> No, it means this change probably should not have happened at all.
Hasn't this been done before? For prefs with no default value,
clearUserPref used to turn the pref into a ghost pref with the same
type, but now it entirely destroys the pref, so you can then set it with
a different type.
--
Warning: May contain traces of nuts.
Simon Paquet
>>> We all agree that the interface is better this way. But there might be
>>> extensions that break because of the change.
>>
>> I'm certain there would be -- catch-for-default is a longstanding
>> pattern in Mozilla and related code, and very widely copied and
>> pasted.
> At least not in mozilla-central, as I reviewed all uses in bug 487059.
The Mozilla world is far larger than just m-c. There's c-c obviously,
there are a lot of other consumers (Flock, Miro, Songbird, etc.) and
there are also a lot of other users in corporate environments.
My take is:
If an interface is frozen, then it is frozen. Period. Not somewhat
frozen or partly frozen or only frozen if we know that a change would
break someone.
Simon
--
Thunderbird/Calendar Localisation (L10n) Coordinator
Thunderbird l10n blog: http://thunderbird-l10n.blogspot.com
Calendar website maintainer: http://www.mozilla.org/projects/calendar
Calendar developer blog: http://weblogs.mozillazine.org/calendar
Dao
> Dao wrote on 28. Oct 2009:
>
>>>> We all agree that the interface is better this way. But there might be
>>>> extensions that break because of the change.
>>>
>>> I'm certain there would be -- catch-for-default is a longstanding
>>> pattern in Mozilla and related code, and very widely copied and
>>> pasted.
>
>> At least not in mozilla-central, as I reviewed all uses in bug 487059.
>
> The Mozilla world is far larger than just m-c. There's c-c obviously,
> there are a lot of other consumers (Flock, Miro, Songbird, etc.) and
> there are also a lot of other users in corporate environments.
mozilla-central is an important data point when we're talking about "a
longstanding pattern in Mozilla and related code". Nothing more, nothing
less.
Boris Zbarsky
> To be clear, we aren't bouncing things off try to get all the callsites.
OK, good. ;) I figured as much, but just makin sure.
> However, that code was in the tree for over 4 months, so it's possible
> there is some interaction that just isn't obvious via code inspection.
Makes sense.
-Boris
Robert Kaiser
> If we ship this way and extensions are written on top of it and then we
> decide to back it out, that's a much bigger deal, I think, since
> breakage for extensions written to the new model would be much more
> serious (exceptions thrown when none are expected).
We know at least a bunch of our own code that already depends on the
new, clean behavior and will break with the old, dirty, bad behavior
documented in the interface (why the **** did we ever write it that way
at all? grrrr).
> I agree with gavin that chances are we don't have that high a fraction
> of our extensions using this function, if only because extensions tend
> to not be very good at cleaning up their prefs.
The normal way of cleaning up is
try { clearUserPref() } catch (e) {}
in any case, which also ignores any real problem that might come up,
actually, just because a non-set user pref throws.
And any cases I've seen before that thread does this with explicitely
called out pref names, which doesn't break in any case with the patch.
The only case that breaks is the one assuming that any throwing of that
function means that a loop over all prefs can be ended, which is a bad
pattern and easily wrong for multiple reasons, including that a non-set
user pref is not the only possible cause of failure, and even then,
someone unsetting one pref in the middle of a list manually breaks the
whole model.
Robert Kaiser
Boris Zbarsky
> The normal way of cleaning up is
> try { clearUserPref() } catch (e) {}
If you look at the actual patches in the bug, this is not the case.
Typically they're using userPrefExists.
> And any cases I've seen before that thread does this with explicitely
> called out pref names, which doesn't break in any case with the patch.
I'm not sure what that means.
-Boris
Mike Kaply
thing:
If nsIPrefBranch is a frozen API, should the behavior of an existing
API be changed?
I still think the answer is no.
I understand that the new way is easier (I won't say better, because I
believe that the new behavior doesn't tell you whether or not the
preference existed when clearUserPref fails, it returns silently), but
that doesn't change the core issue here.
When we freeze an API, we set an expectation to ALL consumers (not
just extensions, but ALL consumers) that things are going to work a
certain way for a long time until we explicitly tell them otherwise.
Keep in mind that we have a lot of consumers (Thunderbird, Seamonkey,
Flock, etc. etc.) and not all of them have moved to 3.6 or trunk yet.
But if people think the API change is worth the risk, then by all
means do it. But then we need to go into our documentation and change
the meaning of the word frozen.
Mike Kaply (too many Mikes here)
gavin
> If nsIPrefBranch is a frozen API, should the behavior of an existing
> API be changed?
>
> I still think the answer is no.
I don't think that was really up for debate. The debate was about what
should be done about the change now (keep it, or back it out). Looks
like we decided to stick with backing it out...
> But if people think the API change is worth the risk, then by all
> means do it. But then we need to go into our documentation and change
> the meaning of the word frozen.
I don't think that follows. One exception doesn't mean that we need to
redefine the term "frozen", nor does it really impact future decisions
about whether APIs should be broken.
Gavin
Mike Beltzner
> On Oct 29, 10:16 am, Mike Kaply <mka...@gmail.com> wrote:
>> If nsIPrefBranch is a frozen API, should the behavior of an existing
>> API be changed?
>>
>> I still think the answer is no.
>
> I don't think that was really up for debate. The debate was about what
> should be done about the change now (keep it, or back it out). Looks
> like we decided to stick with backing it out...
Indeed. That patch has now landed on all development branches.
cheers,
mike