http://trac.webkit.org/changeset/94902 broke the build: Disagrees with general direction for WebKit, and makes refactoring harder (Requested by ap on #webkit). This is an automatic bug report generated by the sheriff-bot. If this bug report was created because of a flaky test, please file a bug for the flaky test (if we don't already have one on file) and dup this bug against that bug so that we can track how often these flaky tests case pain. "Only you can prevent forest fires." -- Smokey the Bear
Created attachment 121753 [details] ROLLOUT of r94902 Any committer can land this patch automatically by marking it commit-queue+. The commit-queue will build and test the patch before landing to ensure that the rollout will be successful. This process takes approximately 15 minutes. If you would like to land the rollout faster, you can use the following command: webkit-patch land-attachment ATTACHMENT_ID where ATTACHMENT_ID is the ID of this attachment.
Comment on attachment 121753 [details] ROLLOUT of r94902 Can we discuss this before landing this patch?
I'm doing some refactoring, and the need to watch platform #ifdefs is making it harder than it needs to be. I've also discussed and confirmed that this is the behavior we want in foreseeable future, so phasing it out is not desirable. Not flipping cq+ just yet, although it seems that we already had a comprehensive discussion in bug 67882.
Comment on attachment 121753 [details] ROLLOUT of r94902 Adam suggested that this should go through review, so marking as such.
Also renaming the bug per Adam's request.
Comment on attachment 121753 [details] ROLLOUT of r94902 I'm trying to understand the motivation for reverting this patch. Here's my understanding of the situation: 1) This code is untested. 2) This code only benefits the AppleMac and AppleWin ports. 3) This code violates the RFCs. 4) This code has been known to crash in the past. My understanding of your point of view is that you prefer to have as few ifdefs in cross-platform code as possible. I don't think non-Apple ports should be forced to compile, execute, and possibly crash in code with these properties. I support the idea of having fewer ifdefs is cross-platform code. Perhaps a better solution is to refactor this logic so that only Apple port bear the costs of this non-standard, potentially crashing, code. We've managed to achieve such things in other situations without ifdefs. We might be able to achieve that here as well.
Comment on attachment 121753 [details] ROLLOUT of r94902 These ifdefs do not have a material effect on any port's runtime behavior, and make WebKit hacking harder. We do not want behavior affecting ifdefs in WebCore if at all possible, and we certainly don't want a port that's used by commit-queue to ifdef out important functionality that has been known to crash in the past.
Basically, the rollout is because the change being rolled out was never agreed upon, was not improving any material aspect of WebKit, and made WebKit development slightly harder.
(I'll respond again tomorrow. I don't think my responding now would be productive to this discussion.)
Comment on attachment 121753 [details] ROLLOUT of r94902 Rolling this out seems better for the project overall at this time. If we could find a way to remove the code entirely in the future that would be nice.
I'm sad that you've made up your mind before I had a change to reply today. I wanted to wait a day to reply so that I could gather my thoughts with a cool head. IMHO, we should just remove this code. Removing the code has the following benefits: 1) Standards compliance. There was a discussion in the HTTP working group about whether the requesting context should be a factor in determining the character set used to decode the Content-Disposition header. The working group decided that we shouldn't use that information for several reasons: a) Varying the interpretation of an HTTP response based on the context of the request is contrary to the stateless nature of HTTP. The fact that HTTP request/response pairs can be understood irrespective of context is core to the design of HTTP. b) Most user agents, including most browsers, do not have this behavior. That means the compatibility risk for dropping the behavior in other user agents (e.g., Safari) is relatively low, especially for a feature likes this one that is not widely used on the mobile web. 2) Stability. This code has crashed in the past and will crash in the future (I can dig up the crash reports for the previous crashes, if you want to see them). While we could invest effort in fixing these crashes, that seems like we'd be better off removing this code and spending that effort improving stability elsewhere. In Bug 67882, Alexey objected to removing this code because he disagreed with the working group's consensus and wanted to retain this non-compliant behavior in Safari. That's ok with me, but I don't think all the other ports should need to pay the stability cost of executing this code. Having the code behind an ifdef seems like a reasonable compromise. In summary, I don't think we should land this patch because it shifts the cost for supporting this non-compliant behavior away from the one port that desires it and onto all the other ports.
I would prefer to first reach consensus between Aleksey and Adam. It seems wrong that we would act (or have acted in the past) without resolving this disagreement. It's not healthy for the community and people involved. We want to be encouraging development of code, not ulcers.
Committed <http://trac.webkit.org/changeset/104723>. Reaching consensus is something that was needed in bug 67882, which got landed in a manner clearly designed to avoid discussion. I do not feel that rolling that out should be more difficult than it already was (the rollout even got a review!)
(In reply to comment #13) > Committed <http://trac.webkit.org/changeset/104723>. > > Reaching consensus is something that was needed in bug 67882, which got landed in a manner clearly designed to avoid discussion. I do not feel that rolling that out should be more difficult than it already was (the rollout even got a review!) Even though this bug is closed, the issue of reaching consensus is still unresolved.