Bug 84247 – [V8] Improve variable resolution order on window

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Erik Arvidsson]

This is the V8 part of 75575

V8 now does the right thing but we need to update the DOM bindings too.

[Erik Arvidsson]

Created attachment 145631 [details]
Patch

[Erik Arvidsson]

(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?

[Erik Arvidsson]

(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

[WebKit Review Bot]

Comment on attachment 145631 [details]
Patch

Clearing flags on attachment: 145631

Committed r119514: <http://trac.webkit.org/changeset/119514>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 88664

[Erik Arvidsson]

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)

[Erik Arvidsson]

Created attachment 148667 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Ojan Vafai]

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

[Erik Arvidsson]

The V8 change has not yet been picked up by the DEPS. Once it has I'll land-safely.

[Erik Arvidsson]

Created attachment 149794 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 149794 [details]
Patch for landing

Clearing flags on attachment: 149794

Committed r121376: <http://trac.webkit.org/changeset/121376>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.