Bug 90208 - [V8] HTMLCollection wrappers are not retained
Summary: [V8] HTMLCollection wrappers are not retained
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:
Depends on:
Blocks:
 
Reported: 2012-06-28 14:27 PDT by Erik Arvidsson
Modified: 2012-06-29 20:32 PDT (History)
6 users (show)

See Also:


Attachments
Patch (10.59 KB, patch)
2012-06-29 11:23 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch for landing (10.54 KB, patch)
2012-06-29 16:43 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-06-28 14:27:09 PDT
The following fails with the V8 bindings because we are not keeping the wrapper for the HTMLFormCollection alive

var form= ....
form.elements.custom = 42;
gc();
shouldBe('form.elements.custom', '42');

This causes a test as part of bug 90194 to fail in V8.
Comment 1 Erik Arvidsson 2012-06-29 11:23:42 PDT
Created attachment 150221 [details]
Patch
Comment 2 Adam Barth 2012-06-29 13:55:08 PDT
Comment on attachment 150221 [details]
Patch

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

> Source/WebCore/bindings/scripts/IDLAttributes.txt:55
> -GenerateIsReachable=|Impl|ImplContext|ImplDocument|ImplElementRoot|ImplFrame
> +GenerateIsReachable=|Impl|ImplContext|ImplDocument|ImplElementRoot|ImplFrame|ImplBaseRoot

Would you be willing to update the documentation about this property?  I don't really understand what these various values mean:
https://trac.webkit.org/wiki/WebKitIDL#JSGenerateToJSObject
Comment 3 Erik Arvidsson 2012-06-29 14:38:35 PDT
(In reply to comment #2)
> Would you be willing to update the documentation about this property?

Sure thing.
Comment 4 Adam Barth 2012-06-29 14:44:48 PDT
Comment on attachment 150221 [details]
Patch

This LGTM, with the caveat that I don't know whether ImplBaseRoot is a sensible name, but it seems in line with the names of the other values this property can take.
Comment 5 Erik Arvidsson 2012-06-29 15:03:38 PDT
(In reply to comment #4)
> (From update of attachment 150221 [details])
> This LGTM, with the caveat that I don't know whether ImplBaseRoot is a sensible name, but it seems in line with the names of the other values this property can take.

The names comes from ...::root(impl()->base()). For V8 we don't do the root part but other than that it is the same. I'll try to explain that in the wiki. The values for these make almost no sense without actually looking at the code gen and/or the .h files.
Comment 6 Erik Arvidsson 2012-06-29 15:47:29 PDT
I filed bug 90317 to look into using WebCore::root for V8 too.
Comment 7 Erik Arvidsson 2012-06-29 16:43:35 PDT
Created attachment 150272 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-06-29 20:32:21 PDT
Comment on attachment 150272 [details]
Patch for landing

Clearing flags on attachment: 150272

Committed r121615: <http://trac.webkit.org/changeset/121615>
Comment 9 WebKit Review Bot 2012-06-29 20:32:26 PDT
All reviewed patches have been landed.  Closing bug.