[DevTools] reworked JavaScriptBreakpointsSidebarPane.js

[DevTools] reworked JavaScriptBreakpointsSidebarPane.js

- sidebar shows one entry for all breakpoints in the line,
- sidebar updates changed lines at the end of JS task after all breakpoint removed and breakpoint added events.

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

Committed: https://crrev.com/3655eb5ce80540b03a07b8fba1d3c8d6f5ae806a
Cr-Commit-Position: refs/heads/master@{#432004}

[kozy, 2016-11-10 19:17:26]

Dmitry and Andrey, please take a look.

[kozy, 2016-11-10 19:17:52]

Description was changed from

==========
[DevTools] rework JavaScriptBreakpointsSidebarPane.js

- sidebar shows one entry for all breakpoints in the line,
- sidebar updates changed lines at the end of JS task after all breakpoint
removed and breakpoint added events.

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

to

==========
[DevTools] reworked JavaScriptBreakpointsSidebarPane.js

- sidebar shows one entry for all breakpoints in the line,
- sidebar updates changed lines at the end of JS task after all breakpoint
removed and breakpoint added events.

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

[lushnikov, 2016-11-11 04:24:54]

(as per offline discussion) Let's try to render this every time from scratch so
that the Sidebar pane itself will be stateless.

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

https://codereview.chromium.org/2491983002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:14:
this._locationSymbol = Symbol('location');
let's make this static

[kozy, 2016-11-11 17:21:13]

I tried to make this state-less but found a problem.

This component uses hover to highlight breakpoint item. When user clicks by
check box, we schedule sidebar update and create all elements from scratch. At
the end of update function I replace current content element with new one. Blink
paints it and then send hover event. Hover event forces additional repaint and
we get annoying blinking.

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

https://codereview.chromium.org/2491983002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:14:
this._locationSymbol = Symbol('location');
On 2016/11/11 04:24:53, lushnikov wrote:
> let's make this static

Done.

[kozy, 2016-11-12 02:17:56]

Dmitry and Andrey, please take another look!

[lushnikov, 2016-11-12 02:57:07]

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

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:8:
WebInspector.JavaScriptBreakpointsSidebarPane = class extends WebInspector.VBox
{
Use WebInspector.ThrottledWidget!!!

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:41:
if (!this._emptyElement) {
let's create it right away in the constructor! You don't need the
this._emptyElement btw!

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:44:
emptyElement.addEventListener('contextmenu',
this._emptyElementContextMenu.bind(this), true);
do we need it??

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:65:
/** @type {!Map<string, !Array<!{breakpoint:
!WebInspector.BreakpointManager.Breakpoint, uiLocation:
!WebInspector.UILocation}>} */
Multimap!

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:87:
var isSelected = selectedUILocation && locations.some(location =>
location.uiLocation.id() === selectedUILocation.id());
if (isSelected)
    shouldShowView = true;

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:103:
_getOrCreateContainerForEntry(index) {
"I don't need this!" (Alexey)

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:113:
_resetEntry(element, locations) {
jsdoc!!

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:113:
_resetEntry(element, locations) {
element, uiLocation, hasEnabled, hasDisabled, isSelected

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:127:
checkboxLabel.checkboxElement.indeterminate = hasEnabled && hasDisabled;
hasEnabled && hasDisabled -- crazy!!

[kozy, 2016-11-12 03:39:05]

All done, please take another look!

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

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:8:
WebInspector.JavaScriptBreakpointsSidebarPane = class extends WebInspector.VBox
{
On 2016/11/12 02:57:06, lushnikov wrote:
> Use WebInspector.ThrottledWidget!!!

Done.

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:41:
if (!this._emptyElement) {
On 2016/11/12 02:57:07, lushnikov wrote:
> let's create it right away in the constructor! You don't need the
> this._emptyElement btw!

Done.

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:44:
emptyElement.addEventListener('contextmenu',
this._emptyElementContextMenu.bind(this), true);
On 2016/11/12 02:57:06, lushnikov wrote:
> do we need it??

Removed.

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:87:
var isSelected = selectedUILocation && locations.some(location =>
location.uiLocation.id() === selectedUILocation.id());
On 2016/11/12 02:57:06, lushnikov wrote:
> if (isSelected)
>     shouldShowView = true;

Done.

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:103:
_getOrCreateContainerForEntry(index) {
On 2016/11/12 02:57:06, lushnikov wrote:
> "I don't need this!" (Alexey)

Done.

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:113:
_resetEntry(element, locations) {
On 2016/11/12 02:57:06, lushnikov wrote:
> jsdoc!!

Done.

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:113:
_resetEntry(element, locations) {
On 2016/11/12 02:57:06, lushnikov wrote:
> element, uiLocation, hasEnabled, hasDisabled, isSelected

Done.

https://codereview.chromium.org/2491983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:127:
checkboxLabel.checkboxElement.indeterminate = hasEnabled && hasDisabled;
On 2016/11/12 02:57:07, lushnikov wrote:
> hasEnabled && hasDisabled -- crazy!!

It's new inline breakpoints world.. :)

[kozy, 2016-11-13 06:23:31]

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

[commit-bot: I haz the power, 2016-11-13 06:23:36]

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

[kozy, 2016-11-13 06:27:23]

Ready for review, please take a look.

- JavaScriptSidebarPane resuses entry elements as much as possible to avoid
removeChildren call which produces repaint.
- better tests.

[commit-bot: I haz the power, 2016-11-13 08:07:43]

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

[commit-bot: I haz the power, 2016-11-13 08:07:44]

Dry run: This issue passed the CQ dry run.

[lushnikov, 2016-11-14 18:56:15]

lgtm

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

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/debugger-test.js:428:
InspectorTest.waitBreakpointSidebarPane = function()
nit: you can avoid exposing breakpointsSidebarPaneContent via adding a flag to
the waitBreakpointsSIdebarPane:

... = function(nonEmpty) {

  ... 
}

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/persistence/persistence-move-breakpoints.html
(left):

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/persistence/persistence-move-breakpoints.html:67:
var sidebarPane =
self.runtime.sharedInstance(Sources.JavaScriptBreakpointsSidebarPane)
can we retain this using your approach from JSSP?

var node = event.target.enclosingNodeOrSelfWithClass('breakpoint-entry');

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/sources/debugger-breakpoints/debugger-reload-breakpoints-with-source-maps-expected.txt
(right):

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/sources/debugger-breakpoints/debugger-reload-breakpoints-with-source-maps-expected.txt:10:
source1.js:17}
It's strange that it's here only after reload; it used to be here from the
beginning.

A guess: maybe source map is yet to load in one of the cases?

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js
(right):

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:38:
this.didUpdateForTest();
nit: let's make all ..ForTest() private while we are here

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:66:
for (var descriptor of locationForEntry.keysArray()) {
let's use valuesArray()

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:90:
var entriesToRemove = this._listElement.children.length - locationForEntry.size;
while (entry) {
   var next = entry.nextSibling;
   entry.remove();
   entry = next;
}

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:138:
* @param {!Event} event
@return

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:140:
_locationFromEvent(event) {
"_uiLocationFromEvent" (Alex)

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:219:
didUpdateForTest() {
nit: _didUpdateForTest

[kozy, 2016-11-14 19:45:26]

thanks! all done.

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

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/debugger-test.js:428:
InspectorTest.waitBreakpointSidebarPane = function()
On 2016/11/14 18:56:15, lushnikov wrote:
> nit: you can avoid exposing breakpointsSidebarPaneContent via adding a flag to
> the waitBreakpointsSIdebarPane:
> 
> ... = function(nonEmpty) {
> 
>   ... 
> }

Acknowledged.

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/persistence/persistence-move-breakpoints.html
(left):

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/persistence/persistence-move-breakpoints.html:67:
var sidebarPane =
self.runtime.sharedInstance(Sources.JavaScriptBreakpointsSidebarPane)
On 2016/11/14 18:56:15, lushnikov wrote:
> can we retain this using your approach from JSSP?
> 
> var node = event.target.enclosingNodeOrSelfWithClass('breakpoint-entry');

Done.

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/sources/debugger-breakpoints/debugger-reload-breakpoints-with-source-maps-expected.txt
(right):

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/sources/debugger-breakpoints/debugger-reload-breakpoints-with-source-maps-expected.txt:10:
source1.js:17}
On 2016/11/14 18:56:15, lushnikov wrote:
> It's strange that it's here only after reload; it used to be here from the
> beginning.
> 
> A guess: maybe source map is yet to load in one of the cases?

Fixed.

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js
(right):

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:38:
this.didUpdateForTest();
On 2016/11/14 18:56:15, lushnikov wrote:
> nit: let's make all ..ForTest() private while we are here

Done.

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:66:
for (var descriptor of locationForEntry.keysArray()) {
On 2016/11/14 18:56:15, lushnikov wrote:
> let's use valuesArray()

Multimap.valuesArray returns flatten array of values instead of array of sets.

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:90:
var entriesToRemove = this._listElement.children.length - locationForEntry.size;
On 2016/11/14 18:56:15, lushnikov wrote:
> while (entry) {
>    var next = entry.nextSibling;
>    entry.remove();
>    entry = next;
> }

Done.

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:138:
* @param {!Event} event
On 2016/11/14 18:56:15, lushnikov wrote:
> @return

Done.

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:140:
_locationFromEvent(event) {
On 2016/11/14 18:56:15, lushnikov wrote:
> "_uiLocationFromEvent" (Alex)

Done.

https://codereview.chromium.org/2491983002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:219:
didUpdateForTest() {
On 2016/11/14 18:56:15, lushnikov wrote:
> nit: _didUpdateForTest

Done.

[kozy, 2016-11-14 19:45:52]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-11-14 19:45:52]

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/2491983002/#ps140001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-11-14 19:47:03]

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

[kozy, 2016-11-14 20:20:47]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-11-14 20:20:47]

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/2491983002/#ps160001
(title: "labal :(")

[commit-bot: I haz the power, 2016-11-14 20:21:27]

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-15 00:12:22]

Description was changed from

==========
[DevTools] reworked JavaScriptBreakpointsSidebarPane.js

- sidebar shows one entry for all breakpoints in the line,
- sidebar updates changed lines at the end of JS task after all breakpoint
removed and breakpoint added events.

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

to

==========
[DevTools] reworked JavaScriptBreakpointsSidebarPane.js

- sidebar shows one entry for all breakpoints in the line,
- sidebar updates changed lines at the end of JS task after all breakpoint
removed and breakpoint added events.

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

[commit-bot: I haz the power, 2016-11-15 00:12:23]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-11-15 00:14:49]

Description was changed from

==========
[DevTools] reworked JavaScriptBreakpointsSidebarPane.js

- sidebar shows one entry for all breakpoints in the line,
- sidebar updates changed lines at the end of JS task after all breakpoint
removed and breakpoint added events.

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

to

==========
[DevTools] reworked JavaScriptBreakpointsSidebarPane.js

- sidebar shows one entry for all breakpoints in the line,
- sidebar updates changed lines at the end of JS task after all breakpoint
removed and breakpoint added events.

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

Committed: https://crrev.com/3655eb5ce80540b03a07b8fba1d3c8d6f5ae806a
Cr-Commit-Position: refs/heads/master@{#432004}
==========

[commit-bot: I haz the power, 2016-11-15 00:14:50]

Patchset 9 (id:??) landed as
https://crrev.com/3655eb5ce80540b03a07b8fba1d3c8d6f5ae806a
Cr-Commit-Position: refs/heads/master@{#432004}