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

[Adam Barth]

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