[webkit-dev] Current legacy argument refactoring -- behavior changes?
Hi Mark. 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. One example was arguments in canvas rendering context functions. 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”. I have two questions: - Did any of the recent refactoring patches change behavior? - Do you know if have test coverage for the optional arguments? -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Current legacy argument refactoring -- behavior changes?
On Fri, Aug 5, 2011 at 11:12 AM, Darin Adler da...@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@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Current legacy argument refactoring -- behavior changes?
Here is the complete list of intentional behavior changes in patches I've submitted in the past few months, along with bug URLs to show the discussion / rationale behind each change: https://bugs.webkit.org/show_bug.cgi?id=63065 IDBFactory.open() https://bugs.webkit.org/show_bug.cgi?id=63079 IDBIndex.get() IDBIndex.getKey() https://bugs.webkit.org/show_bug.cgi?id=63085 IDBKeyRange.only() IDBKeyRange.lowerBound() IDBKeyRange.upperBound() IDBKeyRange.bound() https://bugs.webkit.org/show_bug.cgi?id=63087 IDBObjectStore.put() IDBObjectStore.add() IDBObjectStore.delete() IDBObjectStore.get() IDBObjectStore.createIndex() IDBObjectStore.index() IDBObjectStore.deleteIndex() https://bugs.webkit.org/show_bug.cgi?id=63140 IDBDatabase.createObjectStore() IDBDatabase.deleteObjectStore() IDBDatabase.setVersion() IDBDatabase.transaction() https://bugs.webkit.org/show_bug.cgi?id=64539 all arguments in File API-related methods not already marked [Optional] https://bugs.webkit.org/show_bug.cgi?id=64549 all arguments in WebGL-related methods not already marked [Optional] https://bugs.webkit.org/show_bug.cgi?id=65338 DOMTokenList.item() DOMTokenList.contains() DOMTokenList.add() DOMTokenList.remove() DOMTokenList.toggle() DOMURL.createObjectURL() DOMURL.revokeObjectURL() HTMLAnchorElement.getParameter() HTMLButtonElement.setCustomValidity() HTMLFieldSetElement.setCustomValidity() HTMLInputElement.setCustomValidity() HTMLKeygenElement.setCustomValidity() HTMLMediaElement.canPlayType() TimeRanges.start() TimeRanges.end() HTMLAllCollection.namedItem() HTMLAllCollection.tags() https://bugs.webkit.org/show_bug.cgi?id=65353 DOMWindow.webkitRequestFileSystem() DOMWindow.webkitResolveLocalFileSystemURL() DOMWindow.webkitRequestAnimationFrame() (reverted in https://bugs.webkit.org/show_bug.cgi?id=65698 ) DOMWindow.webkitCancelRequestAnimationFrame() (reverted in https://bugs.webkit.org/show_bug.cgi?id=65698 ) https://bugs.webkit.org/show_bug.cgi?id=65355 Geolocation.clearWatch() PositionCallback.handleEvent() PositionErrorCallback.handleEvent() https://bugs.webkit.org/show_bug.cgi?id=65370 Navigator.registerProtocolHandler() https://bugs.webkit.org/show_bug.cgi?id=65570 SpeechInputResultList.item() https://bugs.webkit.org/show_bug.cgi?id=65571 WebKitAnimationList.item() https://bugs.webkit.org/show_bug.cgi?id=65715 MediaStreamList.item() MediaStreamTrackList.item() TouchList.item() https://bugs.webkit.org/show_bug.cgi?id=65750 AudioBufferSourceNode.noteOn() AudioBufferSourceNode.noteGrainOn() AudioBufferSourceNode.noteOff() ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Current legacy argument refactoring -- behavior changes?
Mark, Adam, thanks for your replies. It seems obviously OK to do this for new APIs that have not been around long, especially ones that never shipped in a production browser, for the same reason that we decided to do this for new functions from this point forward. I have no quibble with that. The way we checked in these changes makes it a challenge to figure out what changed. The list you mailed is easier to understand than the changes themselves, but it’s a pretty long list. Normally we’d want to triage, quickly land completely non-controversial changes, and discuss the more interesting cases at greater length. Best practices for WebKit would be to make sure there are patches where we can see easily the functions affected in each patch. We could accomplish this by first checking in with the [Optional] in a refactoring patch, then removing [Optional] in a behavior change patch along with test cases and, as appropriate, a bit of further discussion discussion. The reason these are best practices is that they help others involved in the project understand the changes and even participate in decision making. The way we’ve been doing it up until now makes it hard for anyone to follow along. The reason I have reviewed so few of these patches is that it’s very hard for me to figure out which ones include behavior changes. What about my test coverage question? Normally we require tests when changing WebKit behavior. Is there a reason these changes are an exception? It seems that in most of these cases creating a test would be straightforward and it would be useful for the project to have such tests. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Current legacy argument refactoring -- behavior changes?
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 da...@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
Re: [webkit-dev] Current legacy argument refactoring -- behavior changes?
On Fri, Aug 5, 2011 at 1:06 PM, Darin Adler da...@apple.com wrote: Mark, Adam, thanks for your replies. It seems obviously OK to do this for new APIs that have not been around long, especially ones that never shipped in a production browser, for the same reason that we decided to do this for new functions from this point forward. I have no quibble with that. The way we checked in these changes makes it a challenge to figure out what changed. The list you mailed is easier to understand than the changes themselves, but it’s a pretty long list. Normally we’d want to triage, quickly land completely non-controversial changes, and discuss the more interesting cases at greater length. Best practices for WebKit would be to make sure there are patches where we can see easily the functions affected in each patch. We could accomplish this by first checking in with the [Optional] in a refactoring patch, then removing [Optional] in a behavior change patch along with test cases and, as appropriate, a bit of further discussion discussion. The reason these are best practices is that they help others involved in the project understand the changes and even participate in decision making. The way we’ve been doing it up until now makes it hard for anyone to follow along. The reason I have reviewed so few of these patches is that it’s very hard for me to figure out which ones include behavior changes. Yeah, we should have more clearly separated phase (3) and phase (4). Ideally, we should have sent out a plan to webkit-dev and then used meta bugs to separate out the different phases so that folks could CC themselves on the bugs related to whichever phase they were most interested in. For the major features that moved to the newer behavior, we discussed each personally with at one of the folks who is a major contributor to the feature. Not all those discussions are recorded in the bug tracker because some of them happened in person or on #webkit. What about my test coverage question? Normally we require tests when changing WebKit behavior. Is there a reason these changes are an exception? It seems that in most of these cases creating a test would be straightforward and it would be useful for the project to have such tests. I've filed https://bugs.webkit.org/show_bug.cgi?id=65787 about adding the missing tests. Thanks, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Current legacy argument refactoring -- behavior changes?
Hi Maciej, On Fri, Aug 5, 2011 at 1:16 PM, Maciej Stachowiak m...@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 da...@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
Re: [webkit-dev] Current legacy argument refactoring -- behavior changes?
This list doesn't seem to include the changes in behavior to addEventListener and removeEventListerner (from patches like http://trac.webkit.org/changeset/92469 http://trac.webkit.org/changeset/92433). Are those intentionally left out? -Sam On Aug 5, 2011, at 12:44 PM, Mark Pilgrim wrote: Here is the complete list of intentional behavior changes in patches I've submitted in the past few months, along with bug URLs to show the discussion / rationale behind each change: https://bugs.webkit.org/show_bug.cgi?id=63065 IDBFactory.open() https://bugs.webkit.org/show_bug.cgi?id=63079 IDBIndex.get() IDBIndex.getKey() https://bugs.webkit.org/show_bug.cgi?id=63085 IDBKeyRange.only() IDBKeyRange.lowerBound() IDBKeyRange.upperBound() IDBKeyRange.bound() https://bugs.webkit.org/show_bug.cgi?id=63087 IDBObjectStore.put() IDBObjectStore.add() IDBObjectStore.delete() IDBObjectStore.get() IDBObjectStore.createIndex() IDBObjectStore.index() IDBObjectStore.deleteIndex() https://bugs.webkit.org/show_bug.cgi?id=63140 IDBDatabase.createObjectStore() IDBDatabase.deleteObjectStore() IDBDatabase.setVersion() IDBDatabase.transaction() https://bugs.webkit.org/show_bug.cgi?id=64539 all arguments in File API-related methods not already marked [Optional] https://bugs.webkit.org/show_bug.cgi?id=64549 all arguments in WebGL-related methods not already marked [Optional] https://bugs.webkit.org/show_bug.cgi?id=65338 DOMTokenList.item() DOMTokenList.contains() DOMTokenList.add() DOMTokenList.remove() DOMTokenList.toggle() DOMURL.createObjectURL() DOMURL.revokeObjectURL() HTMLAnchorElement.getParameter() HTMLButtonElement.setCustomValidity() HTMLFieldSetElement.setCustomValidity() HTMLInputElement.setCustomValidity() HTMLKeygenElement.setCustomValidity() HTMLMediaElement.canPlayType() TimeRanges.start() TimeRanges.end() HTMLAllCollection.namedItem() HTMLAllCollection.tags() https://bugs.webkit.org/show_bug.cgi?id=65353 DOMWindow.webkitRequestFileSystem() DOMWindow.webkitResolveLocalFileSystemURL() DOMWindow.webkitRequestAnimationFrame() (reverted in https://bugs.webkit.org/show_bug.cgi?id=65698 ) DOMWindow.webkitCancelRequestAnimationFrame() (reverted in https://bugs.webkit.org/show_bug.cgi?id=65698 ) https://bugs.webkit.org/show_bug.cgi?id=65355 Geolocation.clearWatch() PositionCallback.handleEvent() PositionErrorCallback.handleEvent() https://bugs.webkit.org/show_bug.cgi?id=65370 Navigator.registerProtocolHandler() https://bugs.webkit.org/show_bug.cgi?id=65570 SpeechInputResultList.item() https://bugs.webkit.org/show_bug.cgi?id=65571 WebKitAnimationList.item() https://bugs.webkit.org/show_bug.cgi?id=65715 MediaStreamList.item() MediaStreamTrackList.item() TouchList.item() https://bugs.webkit.org/show_bug.cgi?id=65750 AudioBufferSourceNode.noteOn() AudioBufferSourceNode.noteGrainOn() AudioBufferSourceNode.noteOff() ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Current legacy argument refactoring -- behavior changes?
I don't fully understand what's going on with those APIs. My understanding prior to this morning is that they were special cased by the code generator. It's on my list of things to investigate today. Adam On Aug 5, 2011 1:48 PM, Sam Weinig wei...@apple.com wrote: This list doesn't seem to include the changes in behavior to addEventListener and removeEventListerner (from patches like http://trac.webkit.org/changeset/92469 http://trac.webkit.org/changeset/92433). Are those intentionally left out? -Sam On Aug 5, 2011, at 12:44 PM, Mark Pilgrim wrote: Here is the complete list of intentional behavior changes in patches I've submitted in the past few months, along with bug URLs to show the discussion / rationale behind each change: https://bugs.webkit.org/show_bug.cgi?id=63065 IDBFactory.open() https://bugs.webkit.org/show_bug.cgi?id=63079 IDBIndex.get() IDBIndex.getKey() https://bugs.webkit.org/show_bug.cgi?id=63085 IDBKeyRange.only() IDBKeyRange.lowerBound() IDBKeyRange.upperBound() IDBKeyRange.bound() https://bugs.webkit.org/show_bug.cgi?id=63087 IDBObjectStore.put() IDBObjectStore.add() IDBObjectStore.delete() IDBObjectStore.get() IDBObjectStore.createIndex() IDBObjectStore.index() IDBObjectStore.deleteIndex() https://bugs.webkit.org/show_bug.cgi?id=63140 IDBDatabase.createObjectStore() IDBDatabase.deleteObjectStore() IDBDatabase.setVersion() IDBDatabase.transaction() https://bugs.webkit.org/show_bug.cgi?id=64539 all arguments in File API-related methods not already marked [Optional] https://bugs.webkit.org/show_bug.cgi?id=64549 all arguments in WebGL-related methods not already marked [Optional] https://bugs.webkit.org/show_bug.cgi?id=65338 DOMTokenList.item() DOMTokenList.contains() DOMTokenList.add() DOMTokenList.remove() DOMTokenList.toggle() DOMURL.createObjectURL() DOMURL.revokeObjectURL() HTMLAnchorElement.getParameter() HTMLButtonElement.setCustomValidity() HTMLFieldSetElement.setCustomValidity() HTMLInputElement.setCustomValidity() HTMLKeygenElement.setCustomValidity() HTMLMediaElement.canPlayType() TimeRanges.start() TimeRanges.end() HTMLAllCollection.namedItem() HTMLAllCollection.tags() https://bugs.webkit.org/show_bug.cgi?id=65353 DOMWindow.webkitRequestFileSystem() DOMWindow.webkitResolveLocalFileSystemURL() DOMWindow.webkitRequestAnimationFrame() (reverted in https://bugs.webkit.org/show_bug.cgi?id=65698 ) DOMWindow.webkitCancelRequestAnimationFrame() (reverted in https://bugs.webkit.org/show_bug.cgi?id=65698 ) https://bugs.webkit.org/show_bug.cgi?id=65355 Geolocation.clearWatch() PositionCallback.handleEvent() PositionErrorCallback.handleEvent() https://bugs.webkit.org/show_bug.cgi?id=65370 Navigator.registerProtocolHandler() https://bugs.webkit.org/show_bug.cgi?id=65570 SpeechInputResultList.item() https://bugs.webkit.org/show_bug.cgi?id=65571 WebKitAnimationList.item() https://bugs.webkit.org/show_bug.cgi?id=65715 MediaStreamList.item() MediaStreamTrackList.item() TouchList.item() https://bugs.webkit.org/show_bug.cgi?id=65750 AudioBufferSourceNode.noteOn() AudioBufferSourceNode.noteGrainOn() AudioBufferSourceNode.noteOff() ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Current legacy argument refactoring -- behavior changes?
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=102840action=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=98024action=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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Current legacy argument refactoring -- behavior changes?
On Fri, Aug 5, 2011 at 5:16 PM, Maciej Stachowiak m...@apple.com wrote: For example, Mark said that the following patch changes HTMLAnchorElement.getParameter: https://bugs.webkit.org/attachment.cgi?id=102840action=review [...] 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. https://bugs.webkit.org/show_bug.cgi?id=65338#c2 You can skip this one. getParameter is very new. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Current legacy argument refactoring -- behavior changes?
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 m...@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=102840action=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=98024action=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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Current legacy argument refactoring -- behavior changes?
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 aba...@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 m...@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=102840action=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=98024action=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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
