[DevTools] make breakpoints better

[DevTools] make breakpoints better

BUG=chromium:566801
R=lushnikov@chromium.org,dgozman@chromium.org

[lushnikov, 2016-11-11 19:56:01]

https://codereview.chromium.org/2500493003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js
(left):

https://codereview.chromium.org/2500493003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:331:
if (this._supportsEnabledBreakpointsWhileEditing() ||
this._scriptFileForTarget.size)
where did this go?

https://codereview.chromium.org/2500493003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js
(right):

https://codereview.chromium.org/2500493003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:273:
if (breakpoints.length === 1) {
nit: can we make it non-nested?

https://codereview.chromium.org/2500493003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:278:
WebInspector.UIString('Edit breakpoint…'),
nit: use \u.. for unicode

https://codereview.chromium.org/2500493003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:377:
for (var lineNumber = 0; lineNumber < this._textEditor.linesCount; ++lineNumber)
that's unfortunate. Maybe it's time to teach editor to return its decorations?

https://codereview.chromium.org/2500493003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:835:
breakpoints.sort((breakpoint1, breakpoint2) => breakpoint1.columnNumber() -
breakpoint2.columnNumber());
why do you need to sort them?

https://codereview.chromium.org/2500493003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1057:
.then(locations => locations.length ? locations : null);
while we're here - we probably can just drop this line to return
!Promise<!Array<..>>

[kozy, 2016-11-14 17:57:25]

Uploaded another approach, ready for review.
Please take a look.

[kozy, 2016-11-15 17:27:02]

Dmitry and Andrey, please take a look!
It's definitely ready for review.
Screenshots are in corresponding issue:
https://bugs.chromium.org/p/chromium/issues/detail?id=566801#c15

[lushnikov, 2016-11-15 22:42:33]

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js
(right):

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:563:
_updateBreakpointDecoration(lineNumber) {
_updateBreakpointDecorations

[dgozman, 2016-11-15 23:35:13]

Exciting! Let's add a lot of tests :-)

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js
(right):

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:256:
breakpoints = [].concat.apply([], lineBreakpoints.valuesArray());
breakpoints = lineBreakpoints.valuesArray() ?

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js
(right):

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:274:
Common.UIString(disableTitle), () => breakpoints.map(breakpoint =>
breakpoint.setEnabled(false)));
Always wrap a literal in Common.UIString() instead of doing it later.

[lushnikov, 2016-11-17 04:31:50]

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js
(right):

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:365:
breakpontLocationsForLineNumber(uiSourceCode, lineNumber) {
typo: breakpointLocationsForLineNumber

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/source_frame/SourcesTextEditor.js
(left):

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/source_frame/SourcesTextEditor.js:172:
addBreakpoint(lineNumber, disabled, conditional) {
nice to get rid of these! thanks

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js
(right):

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:602:
.then(addMissingBreakpoints.bind(this, lineNumber));
what if during this asynchronous code the breakpoint gets removed? (fast
double-click in gutter)

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:626:
function addMissingBreakpoints(lineNumber, locations) {
addInlineBreakpoints

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:658:
Common.UIString('Add conditional breakpoint…'),
this._editBreakpointCondition.bind(this, breakpoint.lineNumber(), breakpoint));
nit: let's use \u... for the ellipsis

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:661:
() => { breakpoint.setCondition('false'); breakpoint.setEnabled(true); });
It would be easier for me to read if the function is extracted

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:664:
Common.UIString('Edit breakpoint…'),
let's use \u... for the ellipsis

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1159:
var maximumBreakpointsAmount =
Runtime.experiments.isEnabled('inlineBreakpoints') ? locations.length : 1;
let's make the experiment check simpler:

if (Runtime.experiments.isEnabled('inlineBreakpoints')) {
   for (...)
} else {
   ...
}

This way it's easier to drop/release experiment.

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/text_editor/cmdevtools.css
(right):

https://codereview.chromium.org/2500493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/text_editor/cmdevtools.css:108:
clip-path: polygon(0% 0%, 60% 0%, 100% 50%, 60% 100%, 0% 100%);
FYI: it's easy to insert UI.Icon instances in editor in case you need more
flexibility