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

[Adam Barth]

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 <mjs at 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
>
>