Summary: | [V8] Use script state of the current isolated world when creating ScriptCallStack | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yury Semikhatsky <yurys> | ||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Yury Semikhatsky <yurys> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, pfeldman, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 32554, 33047 | ||||||||||||
Attachments: |
|
Description
Yury Semikhatsky
2009-12-29 05:10:34 PST
Created attachment 45594 [details]
patch
Created attachment 45595 [details]
patch
style-queue ran check-webkit-style on attachment 45595 [details] without any errors.
Comment on attachment 45595 [details]
patch
I like the premise of the patch. I don't like that we're adding extra methods to already humongous V8Proxy and V8IsolatedWorld. ScriptState is supposed to be a thin wrapper, and I certainly don't want us to carry it around in either of these instances.
Can you rework it perhaps to just add static create methods to ScriptState, and put world-aware logic there?
(In reply to comment #4) > (From update of attachment 45595 [details]) > I like the premise of the patch. I don't like that we're adding extra methods > to already humongous V8Proxy and V8IsolatedWorld. ScriptState is supposed to be > a thin wrapper, and I certainly don't want us to carry it around in either of > these instances. > > Can you rework it perhaps to just add static create methods to ScriptState, and > put world-aware logic there? ScriptState unlike other Script* objetcs is not a value type. I don't want to manage ScriptState life time and my point was to cache pointer to ScriptState on the objects that can be casted to ScriptState* This way the ScriptState instance would live as long as the original object stays alive. Otherwise if one needed to store a pointer to ScriptState from the call stack he would have to make a copy of the ScriptCallStack.state() since the original ScriptState object would be destroyed along with stack allocated ScriptCallStack object. I guess original ScriptState design assumes that clients should be able to keep pointer to ScriptState instance while original context exists. Please correct me if I'm wrong. Created attachment 45617 [details]
patch
Does this change have an observable change in behavior? If so, we'll need a test. Attachment 45617 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/ScriptCallStack.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1
(In reply to comment #7) > Does this change have an observable change in behavior? If so, we'll need a > test. With current implementation of InspectorController the change should not affect observable behavior. Created attachment 45618 [details]
patch
style-queue ran check-webkit-style on attachment 45618 [details] without any errors.
Comment on attachment 45618 [details]
patch
r=me.
Please file a bug regarding the new role of ScriptState.
(In reply to comment #12) > Please file a bug regarding the new role of ScriptState. Done: https://bugs.webkit.org/show_bug.cgi?id=33047 Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/bindings/v8/ScriptCallStack.cpp M WebCore/bindings/v8/ScriptCallStack.h M WebCore/bindings/v8/ScriptController.cpp M WebCore/bindings/v8/ScriptController.h M WebCore/bindings/v8/V8IsolatedWorld.cpp M WebCore/bindings/v8/V8IsolatedWorld.h Committed r52653 |