Clean up tests in preparation for ES5.2 global scope fix
Created attachment 139063 [details] Patch
The following code pattern is broken. var ident = window.ident || expr; because it is semantically the same as var ident; ident = window.ident || expr; which is the same as var ident = undefined; ident = window.ident || expr; which leads to ident being set to expr unless window.ident is read only.
Are we sure that this is a web-safe change? I'm not, I'm fairly sure I've seen this code in the wild.
(In reply to comment #3) > Are we sure that this is a web-safe change? I'm not, I'm fairly sure I've seen this code in the wild. Web safe. Yes. This is how Firefox behaves. WebKit walled garden safe? I'm not sure :'(
Anyway, fixing these tests is harmless.
One important point is that this change in behavior is only for the global scope. The following stil works: (function() { var document = window.document || new MyDocument; ... })();
> Anyway, fixing these tests is harmless. I think that we should achieve a reasonable level of agreement that we want to change behavior here. Losing test coverage for behavior we don't want to change would be detrimental.
(In reply to comment #7) > > Anyway, fixing these tests is harmless. > > I think that we should achieve a reasonable level of agreement that we want to change behavior here. Losing test coverage for behavior we don't want to change would be detrimental. How is this losing test coverage?
Somehow, it always happens so that if you have 50 tests with seemingly exactly the same code, only one of these will catch a regression one day. Just a law of nature, I guess.
(In reply to comment #9) > Somehow, it always happens so that if you have 50 tests with seemingly exactly the same code, only one of these will catch a regression one day. Just a law of nature, I guess. All but one of these 12 failures are in relatively new tests, so that gives me some hope that using this pattern in the global scope is not common in the real world. The spec has changed to require this behavior and I think it's a good thing if we can get away with it. Mainly this allows code to reason about what the value of a variable will be. With WebKit's current implementation, things like JS minifiers need to know about all the IDs on the page to avoid reusing those variable names. Over the years, I've witnessed this cause a number of live production bugs in Google products that were really difficult to track down and to fix. With the spec-required behavior, at least the crazy behavior is isolated to your one line of code, and only if your one line is in the global scope. You don't have a completely different part of your codebase magically breaking your code. We'd like to try this in Chromium. If we ship it and there isn't a significant compat problem, then other ports can follow suit. In the meantime, this patch makes it so these tests keep testing what they intend to test for all ports. Sound ok?
Comment on attachment 139063 [details] Patch This change is the right thing to do. When we make changes to correctly support ES5.2, we should add test cases to defend the new behavior. If ES5.2 isn't compatible with the web, and we need to revert, then we can retain the new tests but change them to defend the desired web-compatible behavior.
Comment on attachment 139063 [details] Patch Attachment 139063 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12548131 New failing tests: fast/images/gif-large-checkerboard.html
Created attachment 139142 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 139063 [details] Patch Clearing flags on attachment: 139063 Committed r115470: <http://trac.webkit.org/changeset/115470>
All reviewed patches have been landed. Closing bug.