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

Fri Aug 5 13:39:35 PDT 2011

Hi Maciej,

On Fri, Aug 5, 2011 at 1:16 PM, Maciej Stachowiak <mjs at apple.com> wrote:
> I have a hard time reading this wall of text, but it seems to me there were at least three things wrong with what happened:

I didn't mean to write a wall of test.  I wanted to give Darin
complete answers to his questions because it seems like the main issue
here is under communication.

> 1) Patches mixing refactoring and behavior changes. This is bad. We should aim whenever possible to separate behavior changes from refactoring. Reviewers should point this out and ask for behavior change to be separated with refactoring.

Yes, I agree that I should have asked Mark to separate all the
behavior-changing patches from the non-behavior changing parts.
Originally I had thought that proceeding by IDL file would have that
effect, but that wasn't the case for 100% of the IDL files because
some, like DOMWindow, include APIs from a number of different
features.

> 2) It seems like this change was not discussed enough for such a large change. You say that "everyone was on board", but from the fact that people are surprised and alarmed by this change, that was clearly not the case.

Yes, I misunderstood the extent to which folks were on board.  For
example, some important parts of the discussion occurred on
<https://bugs.webkit.org/show_bug.cgi?id=21257> in 2009 and 2010.  We
should have re-confirmed with more of the relevant folks more
recently.

> 3) Patches changed behavior without having test cases, and in some cases in the process broke existing tests. This is a clear violation of project policies should have been an automatic r-.

I agree that some of the patches should have included more tests.
Thankfully, that's easy to fix after the fact.

As for breaking existing tests, that's something we should do better
on both for these patches and in the project more generally.

> 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?

(As for adding the missing tests, we can also do that without
reverting and re-applying the IDL changes.)

Adam

> On Aug 5, 2011, at 12:05 PM, Adam Barth wrote:
>
>> On Fri, Aug 5, 2011 at 11:12 AM, Darin Adler <darin at apple.com> wrote:
>>> I assume that the legacy argument refactoring you are doing right now has a goal of no behavior changes. But I’ve noticed some cases where arguments are not marked optional in a few of the patches. Possibly I misunderstood the patch.
>>
>> The project has proceeded in 4 phases:
>>
>> 1) Change the default behavior of the code generator and tag every IDL
>> file with LegacyDefaultOptionalArguments (no behavior change).
>> 2) Remove LegacyDefaultOptionalArguments from IDL files where it
>> doesn't affect the generated code (no behavior change).
>> 3) Remove LegacyDefaultOptionalArguments from "newer" APIs where we
>> wish to have the stricter behavior (behavior change, with tests).
>> 4) Remove LegacyDefaultOptionalArguments and tag each individual
>> optional argument with the [Optional=CallWithDefaultArgument]
>> attribute (no behavior change).
>>
>> Here's an example of a patch from phase (3):
>>
>> http://trac.webkit.org/changeset/89377
>>
>> My understanding was that everyone was on board with tightening up our
>> behavior for newer APIs.  We discussed this on some W3C mailing lists
>> and got Jonas to agree to loosen Firefox in some areas so that we can
>> converge in behavior.
>>
>> My view is that it's important for the long-term health of the web
>> that browser vendors converge on these sorts of behaviors, but it's
>> not overly important whether they converge to strictness or looseness.
>> We picked "strict" for the default to match WebIDL, which aligns with
>> out long-term goal of having WebKit IDL be as close to WebIDL as
>> possible.
>>
>>> One example was arguments in canvas rendering context functions.
>>
>> I believe we changed WebGL to be strict (i.e., matching Firefox) but
>> left 2D canvas loose (as there's a lof of WebKit-specific code that
>> uses canvas).  Is there a specific patch you have in mind?
>>
>> Here's are the recent phase (4) patches in the
>> Source/WebCore/html/canvas directory:
>>
>> http://trac.webkit.org/changeset/92443/trunk/Source/WebCore/html/canvas
>> http://trac.webkit.org/changeset/91617/trunk/Source/WebCore/html/canvas
>>
>> Neither of those are related to canvas rendering context functions,
>> and all of them should preserve existing behavior.
>>
>>> If you we want to make behavior changes, I think it would be done in a patch without other refactoring. I’m sure we’ll want to change at least some, but not mixed in with the refactoring under “accidental cover of darkness”.
>>
>> There has been some patches that combined phase (3) and phase (4)
>> changes in a single patch.  For example, there are some IDL files that
>> include both old APIs (where we want to preserve behavior) and new
>> APIs (e.g., FileSystem, where we want the strict behavior).  We've
>> been proceeding by IDL files, but it's certainly possible I should
>> have asked Mark to separate those changes into separate patches.  I
>> probably also should have been more pedantic about asking Mark to
>> write more tests as we go through these.
>>
>>> I have two questions:
>>> - Did any of the recent refactoring patches change behavior?
>>
>> Yes.  For example,
>> http://trac.webkit.org/changeset/92313/trunk/Source/WebCore/page/DOMWindow.idl
>> mixes some phase (3) and phase (4) changes.  The vast majority of the
>> APIs were left unchanged, but we did make the following APIs stricter:
>>
>> 1) webkitRequestAnimationFrame
>> 2) webkitCancelRequestAnimationFrame
>> 3) webkitRequestFileSystem
>> 4) webkitResolveLocalFileSystemURL
>>
>> My reasoning in asking Mark to do make these changes is that these
>> APIs are relatively new (therefore the compat risk of making them
>> match other browsers is low), and they're vendor-prefixed (which puts
>> developers on notice that we might update them as the spec evolves).
>>
>> That change did break something.  It broke some callers of
>> webkitRequestAnimationFrame.  That issue was fixed in
>> <http://trac.webkit.org/changeset/92392>, which also didn't contain
>> tests.  This time I did ask for tests
>> <https://bugs.webkit.org/show_bug.cgi?id=65698#c7>, which will happen
>> in <https://bugs.webkit.org/show_bug.cgi?id=65710>.
>>
>>> - Do you know if have test coverage for the optional arguments?
>>
>> We've added more test coverage in this area, but I doubt our coverage
>> is 100%, even in the places where we've changed behavior.  I agree
>> that we probably should have added more tests for the phase (3)
>> stragglers that overlapped with phase (4).  That's my fault, as a
>> reviewer, for not asking for these tests.
>>
>> Adam
>> _______________________________________________
>> webkit-dev mailing list
>> webkit-dev at lists.webkit.org
>> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
>