[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 #38 from Yong Li <yoli at rim.com>  2012-07-04 07:35:56 PST ---
(From update of attachment 149263)
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?

> 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?

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