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

Fri Aug 5 13:16:39 PDT 2011

Hi Adam,

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:

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.

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.

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

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.

Regards,
Maciej

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