Bug 40781 – Web Inspector: show actual breakpoint position in UI.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Podivilov]

We should use actual breakpoint position provided by debugger when displaying breakpoint in UI.

[Pavel Podivilov]

Created attachment 59000 [details]
Proposed patch.

[Pavel Podivilov]

Created attachment 59002 [details]
Proposed patch.

[Pavel Feldman]

Comment on attachment 59002 [details]
Proposed patch.

WebCore/ChangeLog:5
 +          Web Inspector: show actual breakpoint position in UI.
Could you please provide more details on the scenario in the ChangeLog?

WebCore/ChangeLog:9
 +          No new tests. (OOPS!)
Remove this line (or add tests!)

WebCore/bindings/js/ScriptDebugServer.cpp:141
 +  bool ScriptDebugServer::setBreakpoint(const String& sourceID, ScriptBreakpoint breakpoint, int* lineNumber)
I don't like the fact that lineNumber is [inout] here. We should only use input or output parameters. I would return int instead of bool with actual line. -1 could indicate unsuccessful attempt. r- is for this.

WebCore/inspector/front-end/BreakpointManager.js:99
 +      _saveBreakpointOnBackend: function(breakpoint, callback)
Every time I look at the method name, I find it confusing. Lets rename to _setBreakpointOnBackend ?

WebCore/inspector/front-end/BreakpointManager.js:110
 +      _didSetBreakpoint: function(breakpoint, success, line)
I think this needs to be a local function in _saveBreakpointOnBackend defined as

function didSetBreakpoint()
{
...
}

I see that you don't want to call it for the one-timers, but it makes the overall logic more confusing. In fact, I would probably use same code for one-time-breakpoints and make them show up in the UI (probably using a bit different rendering).

[Pavel Podivilov]

Created attachment 59770 [details]
Proposed patch.

[Pavel Feldman]

Comment on attachment 59770 [details]
Proposed patch.

WebCore/bindings/js/ScriptDebugServer.cpp:140
 +  bool ScriptDebugServer::setBreakpoint(const String& sourceID, ScriptBreakpoint breakpoint, int lineNumber, int* actualLineNumber)
Given that you still prefer returning bool, you don't need to encode -1, so you should use unsigned values for line numbers.

WebCore/inspector/InspectorController.h:387
 +      HashMap<String, int> m_breakpointsMapping;
So why do you need this mapping after all? Why does sourceID:line not identify breakpoint properly?

Btw, if you really need unique breakpoint ids, now that setBreakpoint has a callback, you could assign them on backend (using progress counter) and send them back to the backend.

[Pavel Podivilov]

Created attachment 59898 [details]
Proposed patch.

[Pavel Podivilov]

(In reply to comment #5)
> (From update of attachment 59770 [details])
> WebCore/bindings/js/ScriptDebugServer.cpp:140
>  +  bool ScriptDebugServer::setBreakpoint(const String& sourceID, ScriptBreakpoint breakpoint, int lineNumber, int* actualLineNumber)
> Given that you still prefer returning bool, you don't need to encode -1, so you should use unsigned values for line numbers.

Done.

> WebCore/inspector/InspectorController.h:387
>  +      HashMap<String, int> m_breakpointsMapping;
> So why do you need this mapping after all? Why does sourceID:line not identify breakpoint properly?

This mapping is needed to preserve existing behavior. If breakpoint was restored to several scripts in didParseSource, and user removes that breakpoint from one of the scripts, we want it to be removed from settings. And since line numbers do not necessarily match now, we need such a mapping.

> 
> Btw, if you really need unique breakpoint ids, now that setBreakpoint has a callback, you could assign them on backend (using progress counter) and send them back to the backend.

Let me introduce it in a separate patch.

[Yury Semikhatsky]

Comment on attachment 59898 [details]
Proposed patch.

WebCore/inspector/InspectorController.cpp:1752
 +      String breakpointId = String::format("%s:%d", sourceID.utf8().data(), actualLineNumber);
Extract this code in a function please.

WebCore/inspector/InspectorController.cpp:1780
 +      it->second.remove(stickyLine);
You may want to remove "it" from the map if it->second becomes empty.

WebCore/inspector/InspectorController.h:387
 +      HashMap<String, int> m_breakpointsMapping;
Shouldn't the value type be unsigned here?

WebCore/inspector/front-end/BreakpointManager.js:122
 +          var that = this;
Use .bind(this) instead, for consistency with the rest code.

WebCore/inspector/InspectorController.cpp:1772
 +      int stickyLine = m_breakpointsMapping.take(breakpointId);
This mapping is a bit confusing. One breakpoint may result is several breakpoints with same URL and different line numbers and deleting any of them will clear the original setting while leaving intact all other breakpoints restored from that setting in other scripts. Probably we should delete all the breakpoints in that case?

[Pavel Podivilov]

Created attachment 59903 [details]
Proposed patch.

[Pavel Podivilov]

(In reply to comment #8)
> (From update of attachment 59898 [details])
> WebCore/inspector/InspectorController.cpp:1752
>  +      String breakpointId = String::format("%s:%d", sourceID.utf8().data(), actualLineNumber);
> Extract this code in a function please.
Done.

> 
> WebCore/inspector/InspectorController.cpp:1780
>  +      it->second.remove(stickyLine);
> You may want to remove "it" from the map if it->second becomes empty.
Empty SourceBreakpoints are skipped by saveBreakpoints method.

> 
> WebCore/inspector/InspectorController.h:387
>  +      HashMap<String, int> m_breakpointsMapping;
> Shouldn't the value type be unsigned here?
Done.

> 
> WebCore/inspector/front-end/BreakpointManager.js:122
>  +          var that = this;
> Use .bind(this) instead, for consistency with the rest code.
Done.

> 
> WebCore/inspector/InspectorController.cpp:1772
>  +      int stickyLine = m_breakpointsMapping.take(breakpointId);
> This mapping is a bit confusing. One breakpoint may result is several breakpoints with same URL and different line numbers and deleting any of them will clear the original setting while leaving intact all other breakpoints restored from that setting in other scripts. Probably we should delete all the breakpoints in that case?
This mapping is used to preserve current behavior. Probably we should change this in a separate patch.

[Pavel Podivilov]

Created attachment 59905 [details]
Proposed patch.

[Yury Semikhatsky]

Comment on attachment 59905 [details]
Proposed patch.

Clearing flags on attachment: 59905

Committed r62095: <http://trac.webkit.org/changeset/62095>

[Yury Semikhatsky]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/62095 might have broken Qt Windows 32-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/62093
http://trac.webkit.org/changeset/62094
http://trac.webkit.org/changeset/62095

[Pavel Feldman]

Comment on attachment 59905 [details]
Proposed patch.

WebCore/inspector/InspectorController.cpp:1758
 +      m_breakpointsMapping.set(breakpointId, actualLineNumber);
I still do not understand why you need this mapping. Again, name is confusing in either case.

[Pavel Podivilov]

(In reply to comment #15)
> (From update of attachment 59905 [details])
> WebCore/inspector/InspectorController.cpp:1758
>  +      m_breakpointsMapping.set(breakpointId, actualLineNumber);
> I still do not understand why you need this mapping. Again, name is confusing in either case.

Please look at the patch for https://bugs.webkit.org/show_bug.cgi?id=40669.
This mapping is moved to front-end, and doesn't look so weird now (I hope).