Maciej and I discussed this in IRC. We agree that it's important to discuss the changes more broadly within the project, but we disagree about the best format for that discussion.
If you're interested in these sorts of changes, please feel free to add yourself to the CC list of this bug. Mark and I will be writing tests for all of the behavior changes and blocking those bugs against this one: https://bugs.webkit.org/show_bug.cgi?id=65787 The patches containing the tests will provide a forum for discussing whether the behavior change is beneficial. If you don't think it's beneficial, please either let us know or comment on the appropriate bug as it goes past. Maciej is worried about the existing changes having "incumbency," so we'll try to avoid treating the current behavior as incumbent in those discussions. Adam On Fri, Aug 5, 2011 at 2:16 PM, Maciej Stachowiak <m...@apple.com> wrote: > > On Aug 5, 2011, at 1:39 PM, Adam Barth wrote: > >> >>> My proposal is to revert all the patches that changed behavior without >>> incorporating tests, and we can decide how to proceed from there. If it's >>> hard to even tell which those are, then we should just revert the whole >>> patch set. Then we can do this right. >> >> I'm not sure that's necessary. We can look over the list of changed >> APIs just as easily without reverting the patches. Are there any in >> the list that Mark set out that you'd like to discuss further? > > I can't actually tell what the behavior changes were from the patches. > > For example, Mark said that the following patch changes > HTMLAnchorElement.getParameter: > > https://bugs.webkit.org/attachment.cgi?id=102840&action=review > > How am I supposed to know from that diff that HTMLAnchorElement.getParameter > changed, or how it changed? How am I supposed to verify that Mark's list of > methods whose behavior changed is complete and correct? > > It's basically impossible to meaningfully discuss the changes. > > > That is why, for behavior-changing patches, the test should be reviewed > *with* the behavior-changing code change, especially when the behavior change > is very subtle in the code. Adding tests after the fact does not give people > a meaningful opportunity to comment on the changes. > > > In fact, I am skeptical that you meaningfully reviewed the behavior changes > caused by the above patch. I don't see any mention of the functions that > changed behavior in the ChangeLog, in the patch, in the bug, or in your > review comments. Even though the changes are called intentional, they give > every appearance of being accidental and give no appearance of having been > carefully considered and reviewed. > > > > By contrast, here is an example of a much better patch: > https://bugs.webkit.org/attachment.cgi?id=98024&action=review > > It has a test and mentions the behavior change in ChangeLog and in the bug. > There is at least an implied justification of "match the spec" My one > complaint about this one is that it also changes behavior of .getKey() as > well as .get(), but the ChangeLog does not mention this (though it is covered > by the test). > > > > I don't think it's good process to say that changes which were never > justified or reviewed are now our new baseline, and anyone objecting to one > of these changes has to come up with a reason. Even though it's impossible to > tell from the diffs what the changes are. Our default should be to have *no* > behavior changes, and any behavior change should be explicitly justified. > > > Therefore I still think revert the correct action, for patches that came with > no test or explanation and justification of the changes.. These changes > should be accompanied with a test and a justification before they are > reviewed. This goes double for interfaces that are in wide use. I don't agree > that just because these changes slipped in, the burden of proof is on others > to rebut them, rather than on advocates of those changes to justify them. > > > Regards, > Maciej > > _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
