Should be possible to edit script sources on the fly.
This is now implemented, isn't it?
Not for JSC.
Created attachment 148524 [details] Patch
Attachment 148524 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 148524 [details] Patch Attachment 148524 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12995175
Comment on attachment 148524 [details] Patch Attachment 148524 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13003142
Comment on attachment 148524 [details] Patch Attachment 148524 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12995174
Comment on attachment 148524 [details] Patch Attachment 148524 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13009130
Comment on attachment 148524 [details] Patch Attachment 148524 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12997162
Comment on attachment 148524 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148524&action=review > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:91 > + const ScriptValue& value = frame->script()->executeScript(newContent); This will just re-evaluate the script as if you were running it in the inspector console while setScriptSource command should allow you to modify a function that may have already been run and leave other stuff untouched if its source doesn't change. E.g. changing "var count = 1; function foo() { count++; }" to "var count = 1; function foo() { count--; }" shouldn't reset value of the count variable to 1.
(In reply to comment #10) > (From update of attachment 148524 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148524&action=review > > > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:91 > > + const ScriptValue& value = frame->script()->executeScript(newContent); > > This will just re-evaluate the script as if you were running it in the inspector console while setScriptSource command should allow you to modify a function that may have already been run and leave other stuff untouched if its source doesn't change. E.g. changing > > "var count = 1; > function foo() > { > count++; > }" > > to > > "var count = 1; > function foo() > { > count--; > }" > > shouldn't reset value of the count variable to 1. I see... Thx.
Created attachment 148768 [details] Patch
Comment on attachment 148768 [details] Patch Attachment 148768 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13026083
Comment on attachment 148768 [details] Patch Attachment 148768 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13025089
Created attachment 148778 [details] Patch
Comment on attachment 148778 [details] Patch Attachment 148778 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13038077
Comment on attachment 148778 [details] Patch Attachment 148778 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13033086
Comment on attachment 148778 [details] Patch Attachment 148778 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13028122
Comment on attachment 148778 [details] Patch Attachment 148778 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13035089
Comment on attachment 148778 [details] Patch Attachment 148778 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13027109
Comment on attachment 148778 [details] Patch Attachment 148778 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13025119
Comment on attachment 148778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148778&action=review > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:85 > +{ You should change the script with given sourceID live, you are evaluating newContent here, basically creating a new script instead of changing the old one.
(In reply to comment #22) > (From update of attachment 148778 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148778&action=review > > > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:85 > > +{ > > You should change the script with given sourceID live, you are evaluating newContent here, basically creating a new script instead of changing the old one. I agree that your description is the ideal solution. But it seems impossible for JSC, since neither JSC nor Frame will cache the JS code they executed. JSC just read the string of JS code, "compile" it into Identifiers and executable code blocks, save these things in Global Data structure, and execute. My solution is taking an advantage of JSC: when JSC is "compiling" JS code, if there is an Identifier with an existent name in Global Data, JSC will replace the old with the new. So I did a little modification of "Interpreter::execute": if we just need to recompile, we send a flag to it to let it just replace the old Identifiers and executable code blocks but don't really execute them. By that way we implement the effect of "editing live js code".
Created attachment 149225 [details] Patch
Comment on attachment 149225 [details] Patch Attachment 149225 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13026992
Comment on attachment 149225 [details] Patch Attachment 149225 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13094006
Comment on attachment 149225 [details] Patch Attachment 149225 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13090023
Comment on attachment 149225 [details] Patch Attachment 149225 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13087071
Comment on attachment 149225 [details] Patch Attachment 149225 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13089037
Created attachment 149240 [details] Patch
Comment on attachment 149240 [details] Patch Attachment 149240 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13098014
Created attachment 149259 [details] Patch
Comment on attachment 149259 [details] Patch Attachment 149259 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13090081
Created attachment 149263 [details] Patch
Someone who knows JSC better should take a look on this.
<rdar://problem/11741197>
The main modifications of this patch are: (1) Add a flag to JSC::Interpret::execute, if this flag is true that means we want to compile the code only. (2) In PageScriptDebugServer, record the map between frame and sourceID of JS to ensure the modified JS code will be recompiled in same world. (3) Implement WebCore::ScriptDebugServer::setScriptSource by calling JSC::Interpret::execute with a flag. This solution takes the advantage that when JSC compile the same script it will replace the old Identifier with the new one.
Comment on attachment 149263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149263&action=review > Source/JavaScriptCore/ChangeLog:8 > +2012-06-21 Peter Wang <peter.wang@torchmobile.com.cn> > + > + [JSC] Web Inspector: implement script source editing > + https://bugs.webkit.org/show_bug.cgi?id=40300 > + > + Reviewed by NOBODY (OOPS!). > + > + * debugger/DebuggerCallFrame.cpp: Can you add more details in changelog? > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094 > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool justRecompile) I believe "fooOnly" is more popular pattern in webkit. > Source/JavaScriptCore/runtime/Completion.h:37 > + JS_EXPORT_PRIVATE JSValue evaluate(ExecState*, ScopeChainNode*, const SourceCode&, bool, JSValue thisValue = JSValue(), JSValue* exception = 0); > } // namespace JSC Why isn't justRecompile the last argument here? > Source/WebCore/bindings/js/PageScriptDebugServer.h:58 > + typedef HashMap<String, Frame*> SourceIdToGlobalObjectMap; should be SourceIDToGlobalObjectMap? Are we sure using Frame* is always safe? I.E. no frame can be freed during the life time of this map? Also, is it possible that one script (or one script ID) can be shared by multiple frames? > Source/WebCore/bindings/js/ScriptController.cpp:130 > + > + RefPtr<Frame> protect(m_frame); why do we need this? I don't see how recompiling a script can free the Frame object. > Source/WebCore/bindings/js/ScriptController.h:89 > + JSC::ExecState* getScriptState(); WebKit doesn't use "getFoo" to name such a getter that passes result only through return value. it could be mainThreadNormalWorldState() -- probably too long? :) > Source/WebCore/bindings/js/ScriptDebugServer.cpp:201 > bool ScriptDebugServer::canSetScriptSource() > { > - return false; > + return true; > } why return true here? > Source/WebCore/bindings/js/ScriptDebugServer.cpp:206 > bool ScriptDebugServer::setScriptSource(const String&, const String&, bool, String*, ScriptValue*, ScriptObject*) > { > - // FIXME(40300): implement this. > - return false; > + return true; > } why return true here but nothing is done?
Comment on attachment 149263 [details] Patch r- for lack of detailed descriptions in changelog. Also, can you provide a test case (even manual one) or mention how others can test it?
(In reply to comment #38) > (From update of attachment 149263 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149263&action=review > > > Source/JavaScriptCore/ChangeLog:8 > > +2012-06-21 Peter Wang <peter.wang@torchmobile.com.cn> > > + > > + [JSC] Web Inspector: implement script source editing > > + https://bugs.webkit.org/show_bug.cgi?id=40300 > > + > > + Reviewed by NOBODY (OOPS!). > > + > > + * debugger/DebuggerCallFrame.cpp: > > Can you add more details in changelog? ok. > > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094 > > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool justRecompile) > > I believe "fooOnly" is more popular pattern in webkit. ok, thx. > > Source/JavaScriptCore/runtime/Completion.h:37 > > + JS_EXPORT_PRIVATE JSValue evaluate(ExecState*, ScopeChainNode*, const SourceCode&, bool, JSValue thisValue = JSValue(), JSValue* exception = 0); > > } // namespace JSC > > Why isn't justRecompile the last argument here? Because the last two parameters have default value, if we pull justRecompile in last position we have to set a default value otherwise it causes a compiling error. > > Source/WebCore/bindings/js/PageScriptDebugServer.h:58 > > + typedef HashMap<String, Frame*> SourceIdToGlobalObjectMap; > > should be SourceIDToGlobalObjectMap? sure, thx. > Are we sure using Frame* is always safe? I.E. no frame can be freed during the life time of this map? Yes, if the frame is freed the inspector will refresh. So if you can see the source code in inspector, the related frame must exists. > Also, is it possible that one script (or one script ID) can be shared by multiple frames? Never. Actually the ID here is the address of SourceProvider of a JS code block. We take it as a index to record from which frame this piece of JS code is executed. > > Source/WebCore/bindings/js/ScriptController.cpp:130 > > + > > + RefPtr<Frame> protect(m_frame); > > why do we need this? I don't see how recompiling a script can free the Frame object. Yes, thx. It's the code from my old patch. It's useless now and should be removed. > > Source/WebCore/bindings/js/ScriptController.h:89 > > + JSC::ExecState* getScriptState(); > > WebKit doesn't use "getFoo" to name such a getter that passes result only through return value. it could be mainThreadNormalWorldState() -- probably too long? :) ok. > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:201 > > bool ScriptDebugServer::canSetScriptSource() > > { > > - return false; > > + return true; > > } > > why return true here? It's a legacy code, since JSC doesn't support this feature but V8 support. Inpsector uses this interface to probe it. If this bug is fixed, this function plus related JS code of front-end will be removed in a dependent bug report. > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:206 > > bool ScriptDebugServer::setScriptSource(const String&, const String&, bool, String*, ScriptValue*, ScriptObject*) > > { > > - // FIXME(40300): implement this. > > - return false; > > + return true; > > } > > why return true here but nothing is done? This function is never be invoked. Its override in PageScriptDebugServer will play the rule. For JSC we cannot implement the feature at "ScriptDebugServer" level as V8 does, since V8 seems have a extra shell layer to record the origin of JS source code.
Created attachment 150884 [details] Patch
Comment on attachment 150884 [details] Patch Attachment 150884 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13145031
Created attachment 150887 [details] Patch
(In reply to comment #40) > (In reply to comment #38) > > Why isn't justRecompile the last argument here? > Because the last two parameters have default value, if we pull justRecompile in last position we have to set a default value otherwise it causes a compiling error. Then we can give a default value? or move arg to the similar position for other functions? It is better to see consistent order. > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:201 > > > bool ScriptDebugServer::canSetScriptSource() > > > { > > > - return false; > > > + return true; > > > } > > > > why return true here? > It's a legacy code, since JSC doesn't support this feature but V8 support. Inpsector uses this interface to probe it. If this bug is fixed, this function plus related JS code of front-end will be removed in a dependent bug report. I see we are using PageScriptDebugServer. Can we make it virtual and leave the base class to return false? > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:206 > > > bool ScriptDebugServer::setScriptSource(const String&, const String&, bool, String*, ScriptValue*, ScriptObject*) > > > { > > > - // FIXME(40300): implement this. > > > - return false; > > > + return true; > > > } > > > > why return true here but nothing is done? > This function is never be invoked. Its override in PageScriptDebugServer will play the rule. For JSC we cannot implement the feature at "ScriptDebugServer" level as V8 does, since V8 seems have a extra shell layer to record the origin of JS source code. If it is never invoked, why do we change it? A pure virtual function or ASSERT_NOT_REACHED may be better. > > > > should be SourceIDToGlobalObjectMap? > sure, thx. m_sourceIdToGlobalObjectMap is still there :)
(In reply to comment #44) > (In reply to comment #40) > > (In reply to comment #38) > > > Why isn't justRecompile the last argument here? > > Because the last two parameters have default value, if we pull justRecompile in last position we have to set a default value otherwise it causes a compiling error. > > Then we can give a default value? or move arg to the similar position for other functions? It is better to see consistent order. Yes, but there is a trivial reason causes we cannot do that. Firstly, we have to keep the old interface untouched: evaluate(ExecState*, ScopeChainNode*, const SourceCode&, JSValue thisValue = JSValue(), JSValue* exception = 0); Otherwise we will break the windows porting since it's a interface of DLL. Then if we put "compileOnly" in last position with a default value, looks like this: evaluate(ExecState*, ScopeChainNode*, const SourceCode&, JSValue thisValue = JSValue(), JSValue* exception = 0, bool compileOnly = false); The compiler will confused it with the old interface and complain. > > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:201 > > > > bool ScriptDebugServer::canSetScriptSource() > > > > { > > > > - return false; > > > > + return true; > > > > } > > > > > > why return true here? > > It's a legacy code, since JSC doesn't support this feature but V8 support. Inpsector uses this interface to probe it. If this bug is fixed, this function plus related JS code of front-end will be removed in a dependent bug report. > > I see we are using PageScriptDebugServer. Can we make it virtual and leave the base class to return false? Don't worry this function too much. It will be removed soon after this bug fixed, like what we did in bug#88759 > > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:206 > > > > bool ScriptDebugServer::setScriptSource(const String&, const String&, bool, String*, ScriptValue*, ScriptObject*) > > > > { > > > > - // FIXME(40300): implement this. > > > > - return false; > > > > + return true; > > > > } > > > > > > why return true here but nothing is done? > > This function is never be invoked. Its override in PageScriptDebugServer will play the rule. For JSC we cannot implement the feature at "ScriptDebugServer" level as V8 does, since V8 seems have a extra shell layer to record the origin of JS source code. > > If it is never invoked, why do we change it? A pure virtual function or ASSERT_NOT_REACHED may be better. Agree with ASSERT_NOT_REACHED, it tells design intension more clearly. Thx. > > > > > > should be SourceIDToGlobalObjectMap? > > sure, thx. > > m_sourceIdToGlobalObjectMap is still there :) I'm so sorry. It'll be corrected.
Created attachment 151015 [details] Patch
(In reply to comment #45) > (In reply to comment #44) > > (In reply to comment #40) > > > (In reply to comment #38) > > > > Why isn't justRecompile the last argument here? > > > Because the last two parameters have default value, if we pull justRecompile in last position we have to set a default value otherwise it causes a compiling error. > > > > Then we can give a default value? or move arg to the similar position for other functions? It is better to see consistent order. > > Yes, but there is a trivial reason causes we cannot do that. Firstly, we have to keep the old interface untouched: > evaluate(ExecState*, ScopeChainNode*, const SourceCode&, JSValue thisValue = JSValue(), JSValue* exception = 0); > Otherwise we will break the windows porting since it's a interface of DLL. > Then if we put "compileOnly" in last position with a default value, looks like this: > evaluate(ExecState*, ScopeChainNode*, const SourceCode&, JSValue thisValue = JSValue(), JSValue* exception = 0, bool compileOnly = false); > The compiler will confused it with the old interface and complain. Can't we change the DLL interface or ask Apple gentlemen for help? > > > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:201 > > > > > bool ScriptDebugServer::canSetScriptSource() > > > > > { > > > > > - return false; > > > > > + return true; > > > > > } > > > > > > > > why return true here? > > > It's a legacy code, since JSC doesn't support this feature but V8 support. Inpsector uses this interface to probe it. If this bug is fixed, this function plus related JS code of front-end will be removed in a dependent bug report. > > > > I see we are using PageScriptDebugServer. Can we make it virtual and leave the base class to return false? > Don't worry this function too much. It will be removed soon after this bug fixed, like what we did in bug#88759 We shouldn't push unfinished thing into the repo. Logically ScriptDebugServer doesn't support setScriptSource() by itself.
Comment on attachment 151015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151015&action=review > Source/WebCore/ChangeLog:12 > + The related test cases already exist. Do the tests need to be enalbed on JSC builds? ie unskipped?
Comment on attachment 151015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151015&action=review > Source/WebCore/bindings/js/ScriptDebugServer.cpp:207 > bool ScriptDebugServer::setScriptSource(const String&, const String&, bool, String*, ScriptValue*, ScriptObject*) > { > - // FIXME(40300): implement this. > + ASSERT_NOT_REACHED(); > return false; > } As pointed by Konrad, ASSERT_NOT_REACHED() looks weird here because pure virtual function makes more sense. sorry for my old suggestion.
Can JSC experts put some comments? Is it the right approach?
(In reply to comment #48) > (From update of attachment 151015 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151015&action=review > > > Source/WebCore/ChangeLog:12 > > + The related test cases already exist. > > Do the tests need to be enalbed on JSC builds? ie unskipped? Frankly speaking, I don't plan to unskip related test case here, since at least Qt porting still has some problems to support some inspector test, like bug#89056. I'm planning to enable it in an independent bug report to make it more clear.
(In reply to comment #49) > (From update of attachment 151015 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151015&action=review > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:207 > > bool ScriptDebugServer::setScriptSource(const String&, const String&, bool, String*, ScriptValue*, ScriptObject*) > > { > > - // FIXME(40300): implement this. > > + ASSERT_NOT_REACHED(); > > return false; > > } > > As pointed by Konrad, ASSERT_NOT_REACHED() looks weird here because pure virtual function makes more sense. sorry for my old suggestion. I agree with Konrad that pure virtual function is better, but some porting derived their own new class form "ScriptDebugServer", pure virtual function will break their own code :(.
Created attachment 151849 [details] Patch
(In reply to comment #53) > Created an attachment (id=151849) [details] > Patch Since the interface "setScriptSource" for JSC is implemented in "PageScriptDebugServer", so set it as pure virtual function to avoid misuse and make the meaning of code more clear.
So, we don't worry about this any more? "some porting derived their own new class form "ScriptDebugServer" Also ScriptDebugServer::canSetScriptSource() should be virtual. It is weird that the base class forces all derived classes to support it. If so, why do we even call this method? Back to the approach you are doing, have you tested the case that the script has been referenced by other scripts? I am afraid some JIT assemblies can still be using the old executable, or even worse, linking to freed memory.
Comment on attachment 151849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151849&action=review > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094 > -JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj) > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool compileOnly) It would be better if we created a compile() helper, preferably outside of the Interpreter class if it's easy. That way instead of saying execute(blah, true) you'd say compile(). > Source/JavaScriptCore/runtime/Completion.cpp:-56 > - JSLockHolder lock(exec); Did you put in some locking elsewhere to justify the removal of this lock acquisition?
(In reply to comment #55) > So, we don't worry about this any more? "some porting derived their own new class form "ScriptDebugServer" > > Also ScriptDebugServer::canSetScriptSource() should be virtual. It is weird that the base class forces all derived classes to support it. If so, why do we even call this method? Like bug#88759, this bug removed an interface ScriptDebugServer::supportsNativeBreakpoints(), this interface existed just because JSC didn't support a feature that V8 supports. As JSC started to support it, this interface became useless and was removed. We'll do the same thing to canSetScriptSource(). These interfaces are just transitions, so eventually they will be removed. > Back to the approach you are doing, have you tested the case that the script has been referenced by other scripts? I am afraid some JIT assemblies can still be using the old executable, or even worse, linking to freed memory. Yes, I did. My approach is almost executing the same piece of JS again, just skip the step of creating a new call frame and reading the executable to evaluate. All the operations that can change the executable are still included, so I think it's safe.
(In reply to comment #56) > (From update of attachment 151849 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151849&action=review > > > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094 > > -JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj) > > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool compileOnly) > > It would be better if we created a compile() helper, preferably outside of the Interpreter class if it's easy. That way instead of saying execute(blah, true) you'd say compile(). I considered this way once. But Interpreter::execute implements all steps of compiling, so that if I want to write an independent compile(), I just need to copy Interpreter::execute and delete several lines, it looks ugly. It's the reason. > > > Source/JavaScriptCore/runtime/Completion.cpp:-56 > > - JSLockHolder lock(exec); > > Did you put in some locking elsewhere to justify the removal of this lock acquisition? Oops, sorry. It's supposed to be here. It's a mistake, seems imported from attachment 149263 [details]. I'll correct it.
Created attachment 152134 [details] Patch
Comment on attachment 152134 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152134&action=review > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094 > -JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj) > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool compileOnly) Split this into two methods, one that compiles, and the other that compiles and executes.
(In reply to comment #58) > (In reply to comment #56) > > (From update of attachment 151849 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=151849&action=review > > > > > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094 > > > -JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj) > > > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool compileOnly) > > > > It would be better if we created a compile() helper, preferably outside of the Interpreter class if it's easy. That way instead of saying execute(blah, true) you'd say compile(). > > I considered this way once. But Interpreter::execute implements all steps of compiling, so that if I want to write an independent compile(), I just need to copy Interpreter::execute and delete several lines, it looks ugly. > It's the reason. Are you joking? > > > > > > Source/JavaScriptCore/runtime/Completion.cpp:-56 > > > - JSLockHolder lock(exec); > > > > Did you put in some locking elsewhere to justify the removal of this lock acquisition? > > Oops, sorry. It's supposed to be here. It's a mistake, seems imported from attachment 149263 [details]. I'll correct it.
(In reply to comment #61) > (In reply to comment #58) > > (In reply to comment #56) > > > (From update of attachment 151849 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=151849&action=review > > > > > > > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094 > > > > -JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj) > > > > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool compileOnly) > > > > > > It would be better if we created a compile() helper, preferably outside of the Interpreter class if it's easy. That way instead of saying execute(blah, true) you'd say compile(). > > > > I considered this way once. But Interpreter::execute implements all steps of compiling, so that if I want to write an independent compile(), I just need to copy Interpreter::execute and delete several lines, it looks ugly. > > It's the reason. > > Are you joking? > ?? No. I'm serious. Parsing, Building AST, Generiting bytecode, creating call frame to evaluate the executable. Except the last step, Interpreter::execute does all steps of compiling.
(In reply to comment #62) > (In reply to comment #61) > > (In reply to comment #58) > > > (In reply to comment #56) > > > > (From update of attachment 151849 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=151849&action=review > > > > > > > > > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094 > > > > > -JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj) > > > > > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool compileOnly) > > > > > > > > It would be better if we created a compile() helper, preferably outside of the Interpreter class if it's easy. That way instead of saying execute(blah, true) you'd say compile(). > > > > > > I considered this way once. But Interpreter::execute implements all steps of compiling, so that if I want to write an independent compile(), I just need to copy Interpreter::execute and delete several lines, it looks ugly. > > > It's the reason. > > > > Are you joking? > > > ?? > No. I'm serious. Parsing, Building AST, Generiting bytecode, creating call frame to evaluate the executable. Except the last step, Interpreter::execute does all steps of compiling. Invoking a method called "execute" in order to compile something is bizarre.
(In reply to comment #63) > (In reply to comment #62) > > (In reply to comment #61) > > > (In reply to comment #58) > > > > (In reply to comment #56) > > > > > (From update of attachment 151849 [details] [details] [details] [details] [details]) > > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=151849&action=review > > > > > > > > > > > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094 > > > > > > -JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj) > > > > > > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool compileOnly) > > > > > > > > > > It would be better if we created a compile() helper, preferably outside of the Interpreter class if it's easy. That way instead of saying execute(blah, true) you'd say compile(). > > > > > > > > I considered this way once. But Interpreter::execute implements all steps of compiling, so that if I want to write an independent compile(), I just need to copy Interpreter::execute and delete several lines, it looks ugly. > > > > It's the reason. > > > > > > Are you joking? > > > > > ?? > > No. I'm serious. Parsing, Building AST, Generiting bytecode, creating call frame to evaluate the executable. Except the last step, Interpreter::execute does all steps of compiling. > > Invoking a method called "execute" in order to compile something is bizarre. Yes, so I'd like to discuss. Based on the current situation, by my opinion, I think there are three ways to solve it: (1) Have independent compile() and execute() with a big block of same code. (2) Have execute() with a parameter. (3) Move compiling related code to an independent function compile(), and let execute() to invoke it. I said the reason why I don't choose (1). I prefer (2) because it brings minimal modification. Do you mean you prefer (3)? I just concern there is too big modification.
(In reply to comment #64) > (In reply to comment #63) > > (In reply to comment #62) > > > (In reply to comment #61) > > > > (In reply to comment #58) > > > > > (In reply to comment #56) > > > > > > (From update of attachment 151849 [details] [details] [details] [details] [details] [details]) > > > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=151849&action=review > > > > > > > > > > > > > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094 > > > > > > > -JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj) > > > > > > > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool compileOnly) > > > > > > > > > > > > It would be better if we created a compile() helper, preferably outside of the Interpreter class if it's easy. That way instead of saying execute(blah, true) you'd say compile(). > > > > > > > > > > I considered this way once. But Interpreter::execute implements all steps of compiling, so that if I want to write an independent compile(), I just need to copy Interpreter::execute and delete several lines, it looks ugly. > > > > > It's the reason. > > > > > > > > Are you joking? > > > > > > > ?? > > > No. I'm serious. Parsing, Building AST, Generiting bytecode, creating call frame to evaluate the executable. Except the last step, Interpreter::execute does all steps of compiling. > > > > Invoking a method called "execute" in order to compile something is bizarre. > > Yes, so I'd like to discuss. Based on the current situation, by my opinion, I think there are three ways to solve it: > (1) Have independent compile() and execute() with a big block of same code. Bad. > (2) Have execute() with a parameter. Bad. > (3) Move compiling related code to an independent function compile(), and let execute() to invoke it. That's what I'm asking for. > I said the reason why I don't choose (1). I prefer (2) because it brings minimal modification. Do you mean you prefer (3)? I just concern there is too big modification. (3) is the only sensible path.
(In reply to comment #65) > (In reply to comment #64) > > (In reply to comment #63) > > > (In reply to comment #62) > > > > (In reply to comment #61) > > > > > (In reply to comment #58) > > > > > > (In reply to comment #56) > > > > > > > (From update of attachment 151849 [details] [details] [details] [details] [details] [details] [details]) > > > > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=151849&action=review > > > > > > > > > > > > > > > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094 > > (3) Move compiling related code to an independent function compile(), and let execute() to invoke it. > > That's what I'm asking for. > > > I said the reason why I don't choose (1). I prefer (2) because it brings minimal modification. Do you mean you prefer (3)? I just concern there is too big modification. > > (3) is the only sensible path. Ok, I understand what you mean now. I'll try to make a new patch.
Comment on attachment 152134 [details] Patch It looks like this patch will recompile a top-level program and then replace all global functions declared by that program using the function keyword. Is that really how this feature is supposed to work? What happens to non-global functions and functions declared using the var keyword?
(In reply to comment #67) > (From update of attachment 152134 [details]) > It looks like this patch will recompile a top-level program and then replace all global functions declared by that program using the function keyword. Is that really how this feature is supposed to work? What happens to non-global functions and functions declared using the var keyword? Sorry for reply your comments so late. I did a test again, my patch can pass the test cases you mentioned. Since the Identifier of old function is replaced, the function body will also be recompiled.
Created attachment 153900 [details] test case I believe this test case will fail, given your approach to code editing.
(In reply to comment #69) > Created an attachment (id=153900) [details] > test case > > I believe this test case will fail, given your approach to code editing. I did a test, my patch can pass this test. :)
Created attachment 155436 [details] Patch
Comment on attachment 155436 [details] Patch Attachment 155436 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13390639
Comment on attachment 155436 [details] Patch Attachment 155436 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13387652
Created attachment 155446 [details] Patch
Comment on attachment 155446 [details] Patch Attachment 155446 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13392599
Created attachment 155450 [details] Patch
Comment on attachment 155450 [details] Patch Attachment 155450 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13385801
Created attachment 155460 [details] Patch
Comment on attachment 155460 [details] Patch Attachment 155460 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13396564
Created attachment 155472 [details] Patch
> > I believe this test case will fail, given your approach to code editing. > > I did a test, my patch can pass this test. :) I don't think you ran the test right. I don't see any code in this patch that would make it work.
(In reply to comment #81) > > > I believe this test case will fail, given your approach to code editing. > > > > I did a test, my patch can pass this test. :) > > I don't think you ran the test right. I don't see any code in this patch that would make it work. Sorry, I did a test again with newest code, my patch doesn't pass this test, and V8 can pass it. :( However, the reason is not that the embedded functions aren't be recompiled. In JSC, the expression var aa = function button1Click() {...}; is compiled as there is a function declaration with name "aa" (refer to JSC::AssignResolveNode::emitBytecode). And we need "execute" these bytecodes to make "aa" to become related with the new function body. The book "JavaScript: The Definitive Guide" says, for that type of expression, like var aa = function button1Click() {...}; "aa" should work like a pointer to the function. I'm not expert of javascript syntax, maybe it's a defect of JSC implementation?
> I'm not expert of javascript syntax, maybe it's a defect of JSC implementation? No, you've just misdiagnosed the issue.
(In reply to comment #83) > > I'm not expert of javascript syntax, maybe it's a defect of JSC implementation? > > No, you've just misdiagnosed the issue. Sorry, I'm a little confused. You mean: (1) The way to implementation of "setScriptSource" is wrong, I shouldn't implement it by recompiling? (2) Or, I don't understand this feature well? I think it gives user a chance to modify some function or expression and see what will happen if these function or expression triggered. Do you think it's right?
(In reply to comment #83) > > I'm not expert of javascript syntax, maybe it's a defect of JSC implementation? > > No, you've just misdiagnosed the issue. Agree, I understand it now, thank you very much.
It requires a JSC reviewer, changing the component to remove it off the inspector dashboard.
Can someone on the JSC team take a look at the latest patch?
Comment on attachment 155472 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155472&action=review I've redone how compilation works since this patch was put up so a lot of the refactoring can probably go away :-/ > Source/JavaScriptCore/interpreter/Interpreter.cpp:1131 > - if (i == 0) { > + if (!i) { Don't change this, while it's a style violation we don't really like changes just to appease the style bot.
Any news on this?
(In reply to comment #89) > Any news on this? The patches of mine cannot cover all the cases, so just ignore them. I'm not sure if JSC supports live-editing js code right now. If not, maybe, I should spend sometime to investigate and try again.
JSC does not support live-editing right now. It is a complex problem (especially when closures are involved.)