Bug 84247 - [V8] Improve variable resolution order on window
Summary: [V8] Improve variable resolution order on window
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Erik Arvidsson
URL:
Keywords: WebExposed
Depends on: 88664
Blocks: 75575
  Show dependency treegraph
 
Reported: 2012-04-18 09:26 PDT by Erik Arvidsson
Modified: 2012-06-27 15:01 PDT (History)
7 users (show)

See Also:


Attachments
Patch (8.27 KB, patch)
2012-06-04 14:52 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Another layout test (819 bytes, text/html)
2012-06-13 11:55 PDT, Erik Arvidsson
no flags Details
Patch (9.97 KB, patch)
2012-06-20 15:33 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (868.41 KB, application/zip)
2012-06-20 20:01 PDT, WebKit Review Bot
no flags Details
Patch for landing (10.01 KB, patch)
2012-06-27 14:23 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2012-04-18 09:26:34 PDT
This is the V8 part of 75575

V8 now does the right thing but we need to update the DOM bindings too.
Comment 1 Erik Arvidsson 2012-06-04 14:52:55 PDT
Created attachment 145631 [details]
Patch
Comment 2 Erik Arvidsson 2012-06-04 15:19:14 PDT
(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.
Comment 3 jochen 2012-06-04 15:29:18 PDT
(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?
Comment 4 Erik Arvidsson 2012-06-04 15:47:14 PDT
(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.
Comment 5 jochen 2012-06-04 15:51:40 PDT
Thanks for the clarification. that sounds reasonable
Comment 6 WebKit Review Bot 2012-06-05 12:23:04 PDT
Comment on attachment 145631 [details]
Patch

Clearing flags on attachment: 145631

Committed r119514: <http://trac.webkit.org/changeset/119514>
Comment 7 WebKit Review Bot 2012-06-05 12:23:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 WebKit Review Bot 2012-06-08 10:12:33 PDT
Re-opened since this is blocked by 88664
Comment 9 Erik Arvidsson 2012-06-13 11:55:56 PDT
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)
Comment 10 Erik Arvidsson 2012-06-20 15:33:34 PDT
Created attachment 148667 [details]
Patch
Comment 11 WebKit Review Bot 2012-06-20 20:01:19 PDT
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
Comment 12 WebKit Review Bot 2012-06-20 20:01:24 PDT
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 13 Ojan Vafai 2012-06-21 08:28:49 PDT
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
Comment 14 Erik Arvidsson 2012-06-21 11:58:39 PDT
The V8 change has not yet been picked up by the DEPS. Once it has I'll land-safely.
Comment 15 Erik Arvidsson 2012-06-27 14:23:31 PDT
Created attachment 149794 [details]
Patch for landing
Comment 16 WebKit Review Bot 2012-06-27 15:01:12 PDT
Comment on attachment 149794 [details]
Patch for landing

Clearing flags on attachment: 149794

Committed r121376: <http://trac.webkit.org/changeset/121376>
Comment 17 WebKit Review Bot 2012-06-27 15:01:18 PDT
All reviewed patches have been landed.  Closing bug.