[webkit-dev] Breaking other ports

Tue Jan 29 23:58:57 PST 2013

On Jan 29, 2013, at 7:17 PM, James Robinson <jamesr at google.com> wrote:

> 
> 
> On Tue, Jan 29, 2013 at 6:29 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
> On Tue, Jan 29, 2013 at 5:46 PM, Adam Barth <abarth at webkit.org> wrote:
> I understand that the new "rules of the road" for WebKit2 are that
> contributors are allowed to break non-Apple ports.  However, those new
> norms do not extend to WebCore.
> 
> In <http://trac.webkit.org/changeset/138962>, Alexey broke form
> resubmission confirmation in the Chromium port.  In
> <https://bugs.webkit.org/show_bug.cgi?id=108214>, he is refusing to
> fix the regression.  I don't view that as acceptable behavior from a
> member of the WebKit community.
> 
> I understand your frustration but I don't think he intentionally broke the feature given that neither EWS nor commit queue detected the regression.
> 
> I don't see Adam or anyone else implying that he intentionally broke a feature, but regardless when a patch to WebCore causes a regression in an upstream port the options are to investigate and fix the regression or roll the patch out.  If Alexey doesn't want to look into the regression, which is valid, then the only clear option is to roll it out until someone who is familiar with the breakage can look at it.
>  
>  As far as I checked both of those bugs, there aren't even a Chromium test failing per his patch. So his patch broke something that had not been tested before.
> 
> Also, given that the fix needs to Chromium port specific, it doesn't seem productive to ask Alexey, who presumably doesn't know much about Chromium port, to come up with a fix for it. And both of those bugs and https://code.google.com/p/chromium/issues/detail?id=172721 don't seem to contain any information as to why his patch broke the feature or what kind of changes he needs to make in order to fix it. Someone who knows how form resubmission confirmation works in Chromium needs to help him so that we can fix this regression.
> 
> If Alexey is interested in learning about this and fixing it, that's fine, but in the meantime the regression can't be left in the tree.  As long as I've been involved with the WebKit project there has been a strict no-regression policy.

I agree that the regression should be fixed. But before we discuss that...

I am puzzled by the apparent stance of "Alexey must immediately fix this himself or we must revert immediately". That's not the standard we have applied in the past to changes that appear generally correct but end up breaking the UI of a client of a specific port.

Let me give a specific example:
----------------
In <https://bugs.webkit.org/show_bug.cgi?id=45631> scroll events were changed to be asynchronous (as it happens, by a Google contributor).

This broke a UI feature of Safari, resulting in <https://bugs.webkit.org/show_bug.cgi?id=52988>. This was, as in the more recent situation, a breakage not caught by WebKit tests or even any of our internal automated testing, but did break a feature in a way that we would have considered unacceptable for a stable release.

Apple folks did not demand an immediate fix or revert in this case. Instead, we investigated what caused the breakage to happen, worked together with Google folks to discuss possible solutions, and in the end added a quirk specific to the Safari UI feature in question ourselves. The more preferred approach would have been to fix it downstream in Safari itself instead of adding a quirk to WebKit, but that turned out to be impractical. We did at one point suggest reverting when we thought the change broke a public website too, but promptly abandoned that position when it turned out that it was the site in question that was broken
----------------

To me this example seems pretty parallel to the situation <https://bugs.webkit.org/show_bug.cgi?id=108214>. Can anyone explain to me why 108214 is being approached so much more aggressively than 52988. Granted, the symptom is somewhat more severe, but I expect the action leading to it is fairly uncommon. And it seems like Alexey's change is broadly correct, in that it fixes a race condition in cross-platform code. Perhaps it's just the fact that Adam and Alexey are both somewhat prone to needlessly inflammatory remarks? If so, perhaps we can simply find other engineers to work on this issue.

It seems to me that the most positive way to resolve this particular issue would be if Chromium folks could help us understand why the change broke Chromium, and help us figure out if the right fix is a change to Chromium, or a particular change to WebKit. And also to be a little patient. I don't expect this will take the multiple months it took to resolve the scroll event issue, but a few days turnaround might be a bit much to expect. Nate suggested a possible workaround in a comment: <https://bugs.webkit.org/show_bug.cgi?id=108214#c3>. I uploaded a patch implementing this workaround to the bug. I would appreciate if any Chromium hackers could help test it and help me determine if it is correct (likewise input from anyone understanding Alexey's change in depth would be appreciated).

Cheers,
Maciej

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20130129/dfe71a71/attachment.html>