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

[Adam Barth]

To clarify:

This message isn't meant to say "if you don't like any of these, speak
up" (although you should feel free to speak up, as always).  It's
meant to encourage folks to review the forthcomming patches with the
following standard for the underlying behavior change (i.e., not
"just" as patches that add some tests):

othermaciej: in other words, for each change, there should be a good
reason to do it, not lack of a good reason not to do it
othermaciej: so if anyone does care to review the matter further, they
should be looking at the justifications, not just the behavior
changes, and thinking "is this a good reason to change", not just "do
I know of some other problem caused by this"

Thanks,
Adam


On Fri, Aug 5, 2011 at 3:04 PM, Adam Barth <abarth at webkit.org> wrote:
> 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
>>
>>
>