WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
30145
There should be a way to reuse isolated worlds
https://bugs.webkit.org/show_bug.cgi?id=30145
Summary
There should be a way to reuse isolated worlds
Aaron Boodman
Reported
2009-10-06 18:11:29 PDT
There should be a way for WebKit clients to reuse isolated worlds, to execute different chunks of code in the same world.
Attachments
patchity patch
(12.38 KB, patch)
2009-10-06 18:22 PDT
,
Aaron Boodman
abarth
: review-
Details
Formatted Diff
Diff
now with even more testing goodness
(18.44 KB, patch)
2009-10-07 12:50 PDT
,
Aaron Boodman
abarth
: review+
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
Forgot to update Skipped files
(20.36 KB, patch)
2009-10-07 15:41 PDT
,
Aaron Boodman
staikos
: review+
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Aaron Boodman
Comment 1
2009-10-06 18:22:53 PDT
Created
attachment 40754
[details]
patchity patch
Aaron Boodman
Comment 2
2009-10-06 18:24:02 PDT
See also Chromium patch:
http://codereview.chromium.org/262002
Adam Barth
Comment 3
2009-10-06 18:49:03 PDT
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
Aaron Boodman
Comment 4
2009-10-07 12:50:11 PDT
Created
attachment 40813
[details]
now with even more testing goodness
Aaron Boodman
Comment 5
2009-10-07 12:54:11 PDT
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.
Aaron Boodman
Comment 6
2009-10-07 12:55:54 PDT
(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 :)
Adam Barth
Comment 7
2009-10-07 13:02:32 PDT
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.
Adam Barth
Comment 8
2009-10-07 13:09:38 PDT
(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.
WebKit Commit Bot
Comment 9
2009-10-07 14:05:40 PDT
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
.
WebKit Commit Bot
Comment 10
2009-10-07 15:29:31 PDT
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
Aaron Boodman
Comment 11
2009-10-07 15:41:06 PDT
Created
attachment 40827
[details]
Forgot to update Skipped files
Aaron Boodman
Comment 12
2009-10-07 15:43:31 PDT
Comment on
attachment 40827
[details]
Forgot to update Skipped files New patch just adds the Skipped entries.
George Staikos
Comment 13
2009-10-07 15:48:37 PDT
Comment on
attachment 40827
[details]
Forgot to update Skipped files Assuming previous review holds, r+
Yong Li
Comment 14
2009-10-19 08:57:53 PDT
Comment on
attachment 40827
[details]
Forgot to update Skipped files Let commit bot land it
WebKit Commit Bot
Comment 15
2009-10-19 09:09:49 PDT
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.
Aaron Boodman
Comment 16
2009-10-19 10:55:31 PDT
This was already checked in.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug