[webkit-dev] Current legacy argument refactoring -- behavior changes?

[Maciej Stachowiak]

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