Summary: | There should be a way to reuse isolated worlds | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aaron Boodman <aa> | ||||||||
Component: | Platform | Assignee: | Aaron Boodman <aa> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Aaron Boodman
2009-10-06 18:11:29 PDT
Created attachment 40754 [details]
patchity patch
See also Chromium patch: http://codereview.chromium.org/262002 Comment on attachment 40754 [details]
patchity patch
+ To destroy it, call deleteSoon().
This method doesn't seem to exist. Also, delete() isn't the right name. detach() is probably better.
+ V8Proxy::~V8Proxy()
I wish we had a small scoped object to do this work instead of doing it explicitly in the destructor.
You seem to only clear the worldID map during destruction of V8Proxy, but doesn't V8Proxy have Frame-lifetime? You need to clean up the map on every navigation. You probably want to do that work during |clearForNavigation|.
This design also doesn't account for the PageCache, but we don't support PageCache anyway...
Tests??? Feel free to modify layoutTestController to make this stuff testable. You can see examples in LayoutTests/http/tests/security/isolatedWorld
Created attachment 40813 [details]
now with even more testing goodness
I renamed the method destroy(). I think this is accurate as, it basically queues the object to be deleted as soon as possible. Since the destructor is private, I don't think there's any chance for confusion. But if you feel strongly about 'weak' or 'release' or whatever, just say so. Regarding a scoped wrapper for V8Proxy. That sounds fine, but using smart pointers with collections always sketches me out a bit because it's not clear what the behavior will be when the collection is resized. Let me know if there is a webkity way to do this I can look to. Regarding clearing the map of worlds. I added a call to clear in navigation and close. It seems to work ,but I am not that familiar with the details of V8Proxy. Please double-check. (In reply to comment #4) > Created an attachment (id=40813) [details] > now with even more testing goodness The 'even more' here is in reference to the testing I already added for this downstream :) Comment on attachment 40813 [details]
now with even more testing goodness
Great. Thanks Aaron.
I woud probably have picked a different name than |destroy|, but that's not essential. Also, you don't need to mark the HTML files as executable.
(In reply to comment #5) > If you feel > strongly about 'weak' or 'release' or whatever, just say so. I don't feel strongly about it. > Regarding a scoped wrapper for V8Proxy. That sounds fine, but using smart > pointers with collections always sketches me out a bit because it's not clear > what the behavior will be when the collection is resized. Let me know if there > is a webkity way to do this I can look to. If we see the pattern recurring it might be worth investing in machinery to make it easier. We'll have to keep an eye out. > Regarding clearing the map of worlds. I added a call to clear in navigation and > close. It seems to work ,but I am not that familiar with the details of > V8Proxy. Please double-check. I believe that is correct for the non-PageCache case, which we don't support. Comment on attachment 40813 [details] now with even more testing goodness Rejecting patch 40813 from commit-queue. aa@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py. Comment on attachment 40813 [details]
now with even more testing goodness
Rejecting patch 40813 from commit-queue.
Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11405 test cases.
http/tests/security/isolatedWorld/iframe.html -> new (results generated in /Users/eseidel/Projects/CommitQueue/LayoutTests/platform/mac/http/tests/security/isolatedWorld)
Exiting early after 1 failures. 8829 tests run.
248.44s total testing time
8828 test cases (99%) succeeded
1 test case (<1%) was new
5 test cases (<1%) had stderr output
Created attachment 40827 [details]
Forgot to update Skipped files
Comment on attachment 40827 [details]
Forgot to update Skipped files
New patch just adds the Skipped entries.
Comment on attachment 40827 [details]
Forgot to update Skipped files
Assuming previous review holds, r+
Comment on attachment 40827 [details]
Forgot to update Skipped files
Let commit bot land it
Comment on attachment 40827 [details] Forgot to update Skipped files Rejecting patch 40827 from commit-queue. Patch https://bugs.webkit.org/attachment.cgi?id=40827 from bug 30145 failed to download and apply. This was already checked in. |