Bug 80329 - [V8] Fix object scope for inline event attribute handlers
Summary: [V8] Fix object scope for inline event attribute handlers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Erik Arvidsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-05 14:45 PST by Erik Arvidsson
Modified: 2012-03-09 12:13 PST (History)
6 users (show)

See Also:


Attachments
Patch (13.21 KB, patch)
2012-03-05 14:55 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (27.09 KB, patch)
2012-03-08 19:25 PST, Erik Arvidsson
ojan: review+
webkit.review.bot: commit-queue-
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-03-05 14:45:46 PST
[V8] Use an interceptor to do the lookup of references in inline event attribute handlers
Comment 1 Erik Arvidsson 2012-03-05 14:55:47 PST
Created attachment 130208 [details]
Patch
Comment 2 Ojan Vafai 2012-03-05 15:25:31 PST
Comment on attachment 130208 [details]
Patch

The logic all looks correct to me. Someone more familiar with the V8 code should probably r+ this though.
Comment 3 Adam Barth 2012-03-05 15:30:38 PST
Comment on attachment 130208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130208&action=review

> Source/WebCore/ChangeLog:9
> +        With this change we introduce an interceptor object in a single with statement instead of using three
> +        nested with statements. This interceptor knows how to correctly lookup the right variable reference.

Is this faster or more correct?  I'm not sure I understand why we're making this change.  (I don't doubt that there's a good reason---I'd just like to understand.)
Comment 4 Ojan Vafai 2012-03-05 15:39:36 PST
It's more correct. Also, more importantly, it will allow us to start gathering metrics of how sites use this in order to identify what changes we can safely make to the platform. We want to minimize this "feature" as much as possible since it hampers our ability to add new methods to HTMLElement and Document.
Comment 5 Erik Arvidsson 2012-03-05 15:47:45 PST
Yes this is more correct. Sorry I should have pointed to the Chrome bugs:

http://code.google.com/p/chromium/issues/detail?id=80911
http://code.google.com/p/chromium/issues/detail?id=80912
Comment 6 WebKit Review Bot 2012-03-05 16:17:15 PST
Comment on attachment 130208 [details]
Patch

Attachment 130208 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11839034

New failing tests:
inspector/debugger/debugger-scripts.html
tables/hittesting/filltable-rtl.html
tables/hittesting/filltable-outline.html
tables/hittesting/filltable-emptycells.html
fast/forms/form-action.html
tables/hittesting/filltable-levels.html
tables/hittesting/filltable-stress.html
fast/events/no-blur-on-page-leave.html
fast/forms/lazy-event-listener-scope-chain.html
fast/harness/results.html
Comment 7 Adam Barth 2012-03-05 16:24:48 PST
Comment on attachment 130208 [details]
Patch

Looks reasonable.  It looks like you have some test failures to work through.
Comment 8 Erik Arvidsson 2012-03-05 17:34:28 PST
Yeah, I have to add one more level of indirection to get reassignment to work.
Comment 9 Erik Arvidsson 2012-03-08 19:10:52 PST
An interceptor does not work because we get the wrong this pointer.
Comment 10 Erik Arvidsson 2012-03-08 19:25:32 PST
Created attachment 130955 [details]
Patch
Comment 11 WebKit Review Bot 2012-03-08 20:52:38 PST
Comment on attachment 130955 [details]
Patch

Attachment 130955 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11903345

New failing tests:
inspector/debugger/debugger-scripts.html
Comment 12 Erik Arvidsson 2012-03-09 12:12:44 PST
Committed r110315: <http://trac.webkit.org/changeset/110315>
Comment 13 Erik Arvidsson 2012-03-09 12:13:00 PST
This will need a rebaseline.