Summary: | [V8] Improve variable resolution order on window | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Erik Arvidsson <arv> | ||||||||||||
Component: | DOM | Assignee: | Erik Arvidsson <arv> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, haraken, japhet, jochen, ojan, webkit.review.bot | ||||||||||||
Priority: | P2 | Keywords: | WebExposed | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 88664 | ||||||||||||||
Bug Blocks: | 75575 | ||||||||||||||
Attachments: |
|
Description
Erik Arvidsson
2012-04-18 09:26:34 PDT
Created attachment 145631 [details]
Patch
(In reply to comment #0) > This is the V8 part of 75575 > > V8 now does the right thing but we need to update the DOM bindings too. Actually, no changes are needed to the bindings. We decided to turn this feature on in webkit to allow smother transition. Once this has landed and stabilized V8 will then change their default value for this flag and we can remove the code in webcore. (In reply to comment #2) > (In reply to comment #0) > > This is the V8 part of 75575 > > > > V8 now does the right thing but we need to update the DOM bindings too. > > Actually, no changes are needed to the bindings. > > We decided to turn this feature on in webkit to allow smother transition. Once this has landed and stabilized V8 will then change their default value for this flag and we can remove the code in webcore. I'm not sure I understand the reason for turning this on in WebKit as opposed to turning it on in V8 directly? (In reply to comment #3) > (In reply to comment #2) > > (In reply to comment #0) > > > This is the V8 part of 75575 > > > > > > V8 now does the right thing but we need to update the DOM bindings too. > > > > Actually, no changes are needed to the bindings. > > > > We decided to turn this feature on in webkit to allow smother transition. Once this has landed and stabilized V8 will then change their default value for this flag and we can remove the code in webcore. > > I'm not sure I understand the reason for turning this on in WebKit as opposed to turning it on in V8 directly? The V8 flag changed twice before and the problem is that when the V8 roll happens a few hours later no V8 people are awake and the v8 roll keeps getting rolled back. The first time this happened was due to the tests not being updated and the second time it was irrelevant. Based on discussions with the V8 team we are doing it this way so that we can do reverts of a webkit only change if needed. Thanks for the clarification. that sounds reasonable Comment on attachment 145631 [details] Patch Clearing flags on attachment: 145631 Committed r119514: <http://trac.webkit.org/changeset/119514> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 88664 Created attachment 147380 [details]
Another layout test
This tests that "var Element;" etc does not reset Element back to undefined which was the reason for the rollback.
This test exposes some other bugs in V8 (window.hasOwnProperty('elementId') should be true)
Created attachment 148667 [details]
Patch
Comment on attachment 148667 [details] Patch Attachment 148667 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13013103 New failing tests: fast/dom/Window/window-property-shadowing-name.html fast/dom/Window/es52-globals.html Created attachment 148717 [details]
Archive of layout-test-results from ec2-cr-linux-01
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 148667 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148667&action=review > Source/WebCore/ChangeLog:10 > + This is the second (third?) try. Last time there was a bug in the V8 code related to the split split window. s/split split/split The V8 change has not yet been picked up by the DEPS. Once it has I'll land-safely. Created attachment 149794 [details]
Patch for landing
Comment on attachment 149794 [details] Patch for landing Clearing flags on attachment: 149794 Committed r121376: <http://trac.webkit.org/changeset/121376> All reviewed patches have been landed. Closing bug. |