[DevTools] Added inline breakpoints

[DevTools] Added inline breakpoints

If line contains at least one breakpoint and more then one possible breakpoint location then text editor will show decorations for all breakpoints and possible breakpoint locations.

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

Committed: https://crrev.com/698c4714e8ad986ee5e12cde0026cb9e1b70e6cc
Cr-Commit-Position: refs/heads/master@{#435589}

[kozy, 2016-11-23 19:39:00]

Andrey, please take a look!

[lushnikov, 2016-11-23 21:54:39]

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

https://codereview.chromium.org/2526013002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1212:
Sources.JavaScriptSourceFrame.BreakpointDecoration = class {
What is BreakpointDecoration? It used to be a gutter thing, but now it blends
with inline bookmark?

Can we distinguish between the two? E.g.:
- Sources.JSSF.BreakpointDecoration holds inline breakpoint
- Sources.JSSF updates text editor gutter decorations wrt BreakpointDecorations

https://codereview.chromium.org/2526013002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1219:
constructor(handle, condition, enabled, breakpoint) {
let's make decoration texteditor-aware:

constructor(textEditor, lineNumber, columnNumber, condition, enabled,
breakpoint) {
  this._textEditor = textEditor;
  this._handle = editor.createHandle(lineNumber, columnNumber);
  ...
  
}

[kozy, 2016-11-23 23:08:47]

Done and answered.

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

https://codereview.chromium.org/2526013002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1212:
Sources.JavaScriptSourceFrame.BreakpointDecoration = class {
On 2016/11/23 21:54:39, lushnikov wrote:
> What is BreakpointDecoration? It used to be a gutter thing, but now it blends
> with inline bookmark?
> 
> Can we distinguish between the two? E.g.:
> - Sources.JSSF.BreakpointDecoration holds inline breakpoint
> - Sources.JSSF updates text editor gutter decorations wrt
BreakpointDecorations

BreakpointDecoration represents decoration for breakpoint or possible breakpoint
location in JavaScriptSourceFrame. gutter decoration is meta decoration that is
calcauled base on existing decorations for breakpoints.

https://codereview.chromium.org/2526013002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1219:
constructor(handle, condition, enabled, breakpoint) {
On 2016/11/23 21:54:39, lushnikov wrote:
> let's make decoration texteditor-aware:
> 
> constructor(textEditor, lineNumber, columnNumber, condition, enabled,
> breakpoint) {
>   this._textEditor = textEditor;
>   this._handle = editor.createHandle(lineNumber, columnNumber);
>   ...
>   
> }

Done.

[dgozman, 2016-11-23 23:46:56]

looks pretty straightfoward!

https://codereview.chromium.org/2526013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/text_editor/cmdevtools.css
(right):

https://codereview.chromium.org/2526013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/cmdevtools.css:129:
clip-path: polygon(0% 0%, 60% 0%, 100% 50%, 60% 100%, 0% 100%);
Maybe a mask or an image? UI.Icon ftw!

[kozy, 2016-11-24 01:24:18]

icons! please take a look.

https://codereview.chromium.org/2526013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/text_editor/cmdevtools.css
(right):

https://codereview.chromium.org/2526013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/cmdevtools.css:129:
clip-path: polygon(0% 0%, 60% 0%, 100% 50%, 60% 100%, 0% 100%);
On 2016/11/23 23:46:56, dgozman wrote:
> Maybe a mask or an image? UI.Icon ftw!

Done.

[lushnikov, 2016-11-28 20:35:19]

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/sources/debugger/source-frame-inline-breakpoint-decorations.html
(right):

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/debugger/source-frame-inline-breakpoint-decorations.html:49:
function testAddRemoveBreakpointInLineWithOneBreakpoint(next)
WithOneLocation

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/debugger/source-frame-inline-breakpoint-decorations.html:98:
function clickBySecondBreakpointAgein()
Again

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/Images/src/optimize_png.hashes
(right):

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/Images/src/optimize_png.hashes:12:
"breakpointInlineConditional.svg": "562f934c14be41a1250fc0b624eda5f5"
what is this?

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/Images/src/svg2png.hashes
(right):

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/Images/src/svg2png.hashes:12:
"breakpointInlineConditional.svg": "562f934c14be41a1250fc0b624eda5f5"
let's drop this as well

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js
(right):

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:218:
// This row doesn't have a breakpoint: We want to show Add Breakpoint and Add
and Edit Breakpoint.
The comment is outdated: there's no "Add and Edit Breakpoint"

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:354:
decoration.hide();
don't you want to hide breakpoints in textEditor operation?

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:776:
if (!decorations.some(decoration => !!decoration.breakpoint)) {
"I can move this to breakpointRemoved" (Alexey)

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:799:
Promise.all(promises).then(() => this._breakpointDecorationsUpdatedForTest());
don't you want to always do this?

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:941:
decoration.hide();
why do you need this call? Isn't it handled in _updateBreakpointDecorations?

[kozy, 2016-11-29 02:12:36]

all done, please take a look!

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/sources/debugger/source-frame-inline-breakpoint-decorations.html
(right):

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/debugger/source-frame-inline-breakpoint-decorations.html:49:
function testAddRemoveBreakpointInLineWithOneBreakpoint(next)
On 2016/11/28 20:35:19, lushnikov wrote:
> WithOneLocation

Done.

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/debugger/source-frame-inline-breakpoint-decorations.html:98:
function clickBySecondBreakpointAgein()
On 2016/11/28 20:35:19, lushnikov wrote:
> Again

Done.

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/Images/src/optimize_png.hashes
(right):

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/Images/src/optimize_png.hashes:12:
"breakpointInlineConditional.svg": "562f934c14be41a1250fc0b624eda5f5"
On 2016/11/28 20:35:19, lushnikov wrote:
> what is this?

done.

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/Images/src/svg2png.hashes
(right):

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/Images/src/svg2png.hashes:12:
"breakpointInlineConditional.svg": "562f934c14be41a1250fc0b624eda5f5"
On 2016/11/28 20:35:19, lushnikov wrote:
> let's drop this as well

Done.

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js
(right):

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:218:
// This row doesn't have a breakpoint: We want to show Add Breakpoint and Add
and Edit Breakpoint.
On 2016/11/28 20:35:19, lushnikov wrote:
> The comment is outdated: there's no "Add and Edit Breakpoint"

Done.

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:354:
decoration.hide();
On 2016/11/28 20:35:19, lushnikov wrote:
> don't you want to hide breakpoints in textEditor operation?

Done.

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:776:
if (!decorations.some(decoration => !!decoration.breakpoint)) {
On 2016/11/28 20:35:19, lushnikov wrote:
> "I can move this to breakpointRemoved" (Alexey)

Done.

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:799:
Promise.all(promises).then(() => this._breakpointDecorationsUpdatedForTest());
On 2016/11/28 20:35:19, lushnikov wrote:
> don't you want to always do this?

Done.

https://codereview.chromium.org/2526013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:941:
decoration.hide();
On 2016/11/28 20:35:19, lushnikov wrote:
> why do you need this call? Isn't it handled in _updateBreakpointDecorations?

Done.

[lushnikov, 2016-11-29 03:17:11]

https://codereview.chromium.org/2526013002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js
(right):

https://codereview.chromium.org/2526013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:809:
if (decorations.length > 1) {
let's hide all decorations when we're editing file

https://codereview.chromium.org/2526013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:914:
this._waitForInlineDecorationsForTest++;
it would be nice to not introduce test's state in the production code

https://codereview.chromium.org/2526013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:929:
if (!decorations.some(decoration => !!decoration.breakpoint) ||
possibleLocations.length <= 1) {
Not needed: possibleLocations.length <= 1

https://codereview.chromium.org/2526013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1268:
this.element = createElementWithClass('div', 'cm-inline-breakpoint');
you don't need a wrapper element - icon is HTMLElement!

https://codereview.chromium.org/2526013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1287:
update() {
update -> updateAndShow

https://codereview.chromium.org/2526013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1293:
this.element.appendChild(UI.Icon.create('smallicon-inline-breakpoint'));
this._icon.setIconType(...)

[kozy, 2016-11-29 19:53:26]

all done. please take another look.

https://codereview.chromium.org/2526013002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js
(right):

https://codereview.chromium.org/2526013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:809:
if (decorations.length > 1) {
On 2016/11/29 03:17:11, lushnikov wrote:
> let's hide all decorations when we're editing file

Done.

https://codereview.chromium.org/2526013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:914:
this._waitForInlineDecorationsForTest++;
On 2016/11/29 03:17:11, lushnikov wrote:
> it would be nice to not introduce test's state in the production code

Done.

https://codereview.chromium.org/2526013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:929:
if (!decorations.some(decoration => !!decoration.breakpoint) ||
possibleLocations.length <= 1) {
On 2016/11/29 03:17:11, lushnikov wrote:
> Not needed: possibleLocations.length <= 1

Done.

https://codereview.chromium.org/2526013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1268:
this.element = createElementWithClass('div', 'cm-inline-breakpoint');
On 2016/11/29 03:17:11, lushnikov wrote:
> you don't need a wrapper element - icon is HTMLElement!

Done.

https://codereview.chromium.org/2526013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1287:
update() {
On 2016/11/29 03:17:11, lushnikov wrote:
> update -> updateAndShow

Done.

https://codereview.chromium.org/2526013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1293:
this.element.appendChild(UI.Icon.create('smallicon-inline-breakpoint'));
On 2016/11/29 03:17:11, lushnikov wrote:
> this._icon.setIconType(...)

Done.

[lushnikov, 2016-11-29 21:24:29]

lgtm

https://codereview.chromium.org/2526013002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/inspector/debugger-test.js
(right):

https://codereview.chromium.org/2526013002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/debugger-test.js:632:
InspectorTest.prepareJavaScriptSourceFrame = function(sourceFrame)
prepareSourceFrameForBreakpointTest =

https://codereview.chromium.org/2526013002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/debugger-test.js:647: return
waitUpdate().then(checkIfReady);
let's that sourceFrame is prepared!

if (..) {
   log
   InspectorTest.completeTest();
   return;
}

https://codereview.chromium.org/2526013002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/debugger-test.js:655: return
waitUpdate().then(checkIfReady);
this will potentially result in an infinite promise chain

[kozy, 2016-11-29 22:30:10]

thanks!
all done.

https://codereview.chromium.org/2526013002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/inspector/debugger-test.js
(right):

https://codereview.chromium.org/2526013002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/debugger-test.js:632:
InspectorTest.prepareJavaScriptSourceFrame = function(sourceFrame)
On 2016/11/29 21:24:29, lushnikov wrote:
> prepareSourceFrameForBreakpointTest =

Done.

https://codereview.chromium.org/2526013002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/debugger-test.js:647: return
waitUpdate().then(checkIfReady);
On 2016/11/29 21:24:29, lushnikov wrote:
> let's that sourceFrame is prepared!
> 
> if (..) {
>    log
>    InspectorTest.completeTest();
>    return;
> }

Done.

https://codereview.chromium.org/2526013002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/debugger-test.js:655: return
waitUpdate().then(checkIfReady);
On 2016/11/29 21:24:29, lushnikov wrote:
> this will potentially result in an infinite promise chain

Yes, but all previous promises will be collected after resolving since there is
no other reference to promise object and there are no any memory leaks.

[kozy, 2016-11-29 22:30:14]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-11-29 22:30:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
lushnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/2526013002/#ps120001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-11-29 22:30:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kozy, 2016-11-29 22:39:56]

The CQ bit was unchecked by kozyatinskiy@chromium.org

[kozy, 2016-11-29 22:47:28]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-11-29 22:47:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
lushnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/2526013002/#ps140001
(title: "rebased test")

[commit-bot: I haz the power, 2016-11-29 22:48:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 00:51:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 00:51:39]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[kozy, 2016-11-30 07:04:01]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-11-30 07:04:08]

The CQ bit was unchecked by kozyatinskiy@chromium.org

[kozy, 2016-11-30 07:04:17]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 07:05:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 08:15:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 08:15:33]

Dry run: This issue passed the CQ dry run.

[kozy, 2016-12-01 07:39:32]

Patchset #9 (id:160001) has been deleted

[kozy, 2016-12-01 07:39:43]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-12-01 07:39:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
lushnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/2526013002/#ps140001
(title: "rebased test")

[commit-bot: I haz the power, 2016-12-01 07:39:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 09:46:35]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480577983326320,
"parent_rev": "14e6ff957426b23ba58d78b6f785564cce802c03", "commit_rev":
"940824d77542c7a4f447c7c7351c27af2a76cc48"}

[commit-bot: I haz the power, 2016-12-01 09:47:02]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-12-01 09:49:14]

Description was changed from

==========
[DevTools] Added inline breakpoints

If line contains at least one breakpoint and more then one possible breakpoint
location then text editor will show decorations for all breakpoints and possible
breakpoint locations.

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

to

==========
[DevTools] Added inline breakpoints

If line contains at least one breakpoint and more then one possible breakpoint
location then text editor will show decorations for all breakpoints and possible
breakpoint locations.

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

Committed: https://crrev.com/698c4714e8ad986ee5e12cde0026cb9e1b70e6cc
Cr-Commit-Position: refs/heads/master@{#435589}
==========

[commit-bot: I haz the power, 2016-12-01 09:49:16]

Patchset 8 (id:??) landed as
https://crrev.com/698c4714e8ad986ee5e12cde0026cb9e1b70e6cc
Cr-Commit-Position: refs/heads/master@{#435589}