[Webkit-unassigned] [Bug 53003] Web Inspector: [JSC] implement setting breakpoints by line:column

[bugzilla-daemon at webkit.org]

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





--- Comment #19 from Peter Wang <peter.wang at torchmobile.com.cn>  2012-06-04 03:32:43 PST ---
(In reply to comment #18)
> (From update of attachment 144524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144524&action=review
> 
> > Source/WebCore/bindings/js/ScriptDebugServer.cpp:95
> > +    unsigned i;
> 
> Please move it to the for() initialization section:
>   for (size_t i = 0; i < breaksCount; i++) {
> 
> > Source/WebCore/bindings/js/ScriptDebugServer.cpp:135
> > +        if (breaksVector.at(i).columnNumber == (int)columnNumber) {
> 
> We should change types of ScriptBreakpoint.{lolumn,line}Number from int to unsigned if they are always >= 0. Also please use static_cast<int> instead of c-syle cast.
> 
> > Source/WebCore/bindings/js/ScriptDebugServer.cpp:163
> > +    unsigned j;
> 
> Please declare the variables in the corresponding for() sections where they are initialized.
> 
> > Source/WebCore/bindings/js/ScriptDebugServer.cpp:438
> > +    const TextPosition position = m_currentCallFrame->position();
> 
> What's the point in extracting this into a local variable given that it is used only once in the method?

sorry, it's a legacy of my old patch, I just neglected it. it will be modified.

> > Source/WebCore/bindings/js/ScriptDebugServer.cpp:446
> > +    m_currentCallFrame->updatePosition(hitPosition);
> 
> You shouldn't need to update call frame position from the ScriptDebugServer, VM should provide correct location.
> 
Actually, the VM of JSC can not provide correct enough location. It can only provide the line number. If there are several statements in one line, ScriptDebugServer has to "guest" the column and set it in currentCallFrame to make sure send a right position to frontend.

> > Source/WebCore/bindings/js/ScriptDebugServer.h:137
> > +    typedef HashMap<Page*, ListenerSet*> PageListenersMap;
> 
> Revert this line.

Sorry, ok.

> > Source/WebCore/bindings/js/ScriptDebugServer.h:153
> > +    mutable BreakpointsInLine m_lastHitScriptBreakpoints;
> 
> Why do you need these fields to be mutable?
Because I think it's most appropriate to change it in "hasBreakpoint", but "hasBreakpoint" is a const function. I just don't want to break the original design.

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