[Webkit-unassigned] [Bug 40300] Web Inspector: [JSC] implement script source editing

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=40300





--- Comment #40 from Peter Wang <peter.wang at torchmobile.com.cn>  2012-07-04 20:07:30 PST ---
(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 at 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.