Bug 29330 – [v8] Don't keep clean wrappers artificially alive

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Misc.

[Christian Plesner Hansen]

We currently keep all DOM node wrappers alive, even when there are no more references to them from JS, in case they have properties that we need to keep around if new JS references are created.  This changes the policy to only keep wrappers artificially alive if they have changed since they were created.  Empty wrappers are discarded and recreated as needed.

[Christian Plesner Hansen]

Created attachment 39686 [details]
initial

[Christian Plesner Hansen]

Important note: this change can not be committed until v8 has been updated in chromium.  Should I wait until then to get this reviewed?

[Adam Barth]

Comment on attachment 39686 [details]
initial

Super cool change.  Feel free to ping this bug when you want it landed.  You can also set the commit-queue flag to ? if you'd like the commit bot to land the patch.

[Christian Plesner Hansen]

Thanks, that was quick!

So if I set the commit flag to ? someone with the right permissions will notice and set it to +?

[Adam Barth]

(In reply to comment #4)
> Thanks, that was quick!

Well, the patch is obviously correct by magic (because all the hard work is going on in V8).  :)

> So if I set the commit flag to ? someone with the right permissions will notice
> and set it to +?

Yes.  I've CC'ed Eric explicitly because he's good at setting the commit-queue flag.

[Eric Seidel (no email)]

(In reply to comment #5)
> Yes.  I've CC'ed Eric explicitly because he's good at setting the commit-queue
> flag.

Huh?

[Adam Barth]

> Huh?

Meaning if you see an email with commit-queue?, you're likely to go to the bug and set commit-queue+.  :)

[Eric Seidel (no email)]

(In reply to comment #7)
> > Huh?
> 
> Meaning if you see an email with commit-queue?, you're likely to go to the bug
> and set commit-queue+.  :)

Except here it's set to cq-...

[Adam Barth]

(In reply to comment #8)
> Except here it's set to cq-...

See comment #2.  :)

[WebKit Commit Bot]

Comment on attachment 39686 [details]
initial

Rejecting patch 39686 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Committing to http://svn.webkit.org/repository/webkit/trunk ...
Merge conflict during commit: Your file or directory 'WebCore/ChangeLog' is probably out-of-date: resource out of date; try updating at /usr/local/libexec/git-core//git-svn line 469

[Adam Barth]

Comment on attachment 39686 [details]
initial

I don't think the commit queue should reject patches when it gets that error.

[Eric Seidel (no email)]

(In reply to comment #11)
> (From update of attachment 39686 [details])
> I don't think the commit queue should reject patches when it gets that error.

I totally agree!  bug 28316.  I'll see if I can post a patch to fix it today. :(

[Eric Seidel (no email)]

Builders have been red for the last while: http://webkit-commit-queue.appspot.com/  But once they go green again this will land.

[WebKit Commit Bot]

Comment on attachment 39686 [details]
initial

Clearing flags on attachment: 39686

Committed r48639: <http://trac.webkit.org/changeset/48639>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

This patch seems to have introduced all kinds of weird, seemingly random layout test failures. I am rolling this out until we hash this out.

[Dimitri Glazkov (Google)]

Rolled out in http://trac.webkit.org/changeset/48657.

Awesome scripting, Eric!

[Anders Carlsson]

V8 is gone.