DevTools: Fix Elements tab event listener removal

DevTools: Fix Elements tab event listener removal

Upon removal of the last event listener of a given type,
the entire tree for that type would be removed from the
DOM and never readded. It's not clear why it was
originally being removed in the first place since the
hidden check will take care of this case already.

Updated the behavior to simply collapse instead of remove.

BUG=672867
Committed: https://crrev.com/3c1e09c2062cfd0a37a47837eba5d2654d7a7198
Cr-Commit-Position: refs/heads/master@{#439547}

[phulce, 2016-12-13 00:52:03]

Description was changed from

==========
DevTools: Fix Elements tab event listener removal

BUG=
==========

to

==========
DevTools: Fix Elements tab event listener removal

Upon removal of the last event listener of a given type,
the entire tree for that type would be removed from the
DOM and never readded. It's not clear why it was
originally being removed in the first place since the
hidden check will take care of this case already.

Updated the behavior to simply collapse instead of remove.

BUG=672867
==========

[phulce, 2016-12-13 00:52:03]

phulce@chromium.org changed reviewers:
+ lushnikov@chromium.org

[lushnikov, 2016-12-14 01:40:05]

https://codereview.chromium.org/2574603002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/components/EventListenersView.js
(left):

https://codereview.chromium.org/2574603002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/components/EventListenersView.js:330:
parent.parent.removeChild(parent);
The old behavior is better! We shouldn't show any category if there are no
events in there.

Let's rather clean the _treeItemMap here

[pfeldman, 2016-12-14 01:42:16]

Time to write a test?

[phulce, 2016-12-14 17:14:10]

On 2016/12/14 01:40:05, lushnikov wrote:
>
https://codereview.chromium.org/2574603002/diff/1/third_party/WebKit/Source/d...
> File
> third_party/WebKit/Source/devtools/front_end/components/EventListenersView.js
> (left):
> 
>
https://codereview.chromium.org/2574603002/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/components/EventListenersView.js:330:
> parent.parent.removeChild(parent);
> The old behavior is better! We shouldn't show any category if there are no
> events in there.
> 
> Let's rather clean the _treeItemMap here

@lushnikov to be clear, the category wasn't ever being shown again (including
after refresh) and this still does not show it to the user (via the hidden
attribute below) until a new event listener of that type appears. The collapse
vs. remove is just reseting the collapsed state instead of removing the element
from the tree when every other piece of this class seems to assume that will
never happen.

[lushnikov, 2016-12-14 18:44:04]

Ah, I see now, thanks

Let's add a test to verify the behavior - and it's good to go.

[phulce, 2016-12-14 19:10:28]

On 2016/12/14 18:44:04, lushnikov wrote:
> Ah, I see now, thanks
> 
> Let's add a test to verify the behavior - and it's good to go.

sweet, done!

[lushnikov, 2016-12-15 22:33:16]

lgtm

https://codereview.chromium.org/2574603002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/inspector/elements-test.js
(right):

https://codereview.chromium.org/2574603002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/elements-test.js:453: const
treeOutline =
InspectorTest.eventListenersWidget()._eventListenersView._treeOutline;
style: const -> var. We don't use consts so far

https://codereview.chromium.org/2574603002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/inspector/inspector-test.js
(right):

https://codereview.chromium.org/2574603002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/inspector-test.js:356: if
(force)
let's rather do:

  if (force)
    lstenersArrived();
  else
    InspectorTest.addSniffer(Components.EventListenersView.prototype,
"_eventListenersArrivedForTest", listenersArrived);

Because someone else might be sniffing the _eventListenersArrivedForTest
somewhere..

https://codereview.chromium.org/2574603002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/elements/event-listener-sidebar-remove.html
(right):

https://codereview.chromium.org/2574603002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/elements/event-listener-sidebar-remove.html:6:
// window.debugTest = true;
let's kill this

[phulce, 2016-12-16 18:41:56]

https://codereview.chromium.org/2574603002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/inspector/elements-test.js
(right):

https://codereview.chromium.org/2574603002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/elements-test.js:453: const
treeOutline =
InspectorTest.eventListenersWidget()._eventListenersView._treeOutline;
On 2016/12/15 22:33:16, lushnikov wrote:
> style: const -> var. We don't use consts so far

Done.

https://codereview.chromium.org/2574603002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/inspector/inspector-test.js
(right):

https://codereview.chromium.org/2574603002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/inspector-test.js:356: if
(force)
On 2016/12/15 22:33:16, lushnikov wrote:
> let's rather do:
> 
>   if (force)
>     lstenersArrived();
>   else
>     InspectorTest.addSniffer(Components.EventListenersView.prototype,
> "_eventListenersArrivedForTest", listenersArrived);
> 
> Because someone else might be sniffing the _eventListenersArrivedForTest
> somewhere..

Went back and forth and that actually, good point

https://codereview.chromium.org/2574603002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/elements/event-listener-sidebar-remove.html
(right):

https://codereview.chromium.org/2574603002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/elements/event-listener-sidebar-remove.html:6:
// window.debugTest = true;
On 2016/12/15 22:33:16, lushnikov wrote:
> let's kill this

whoops! done

[phulce, 2016-12-19 18:39:26]

The CQ bit was checked by phulce@chromium.org

[phulce, 2016-12-19 18:39:26]

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/2574603002/#ps40001
(title: "address feedback")

[commit-bot: I haz the power, 2016-12-19 18:39:52]

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-19 20:49:14]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482172766220230,
"parent_rev": "419822ee19dc8c628ab033b91d320decdeb0c3c1", "commit_rev":
"f686479a23f3745c7ba4e90778e69fdfb2835145"}

[commit-bot: I haz the power, 2016-12-19 20:49:38]

Description was changed from

==========
DevTools: Fix Elements tab event listener removal

Upon removal of the last event listener of a given type,
the entire tree for that type would be removed from the
DOM and never readded. It's not clear why it was
originally being removed in the first place since the
hidden check will take care of this case already.

Updated the behavior to simply collapse instead of remove.

BUG=672867
==========

to

==========
DevTools: Fix Elements tab event listener removal

Upon removal of the last event listener of a given type,
the entire tree for that type would be removed from the
DOM and never readded. It's not clear why it was
originally being removed in the first place since the
hidden check will take care of this case already.

Updated the behavior to simply collapse instead of remove.

BUG=672867

Review-Url: https://codereview.chromium.org/2574603002
==========

[commit-bot: I haz the power, 2016-12-19 20:49:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-19 20:52:50]

Description was changed from

==========
DevTools: Fix Elements tab event listener removal

Upon removal of the last event listener of a given type,
the entire tree for that type would be removed from the
DOM and never readded. It's not clear why it was
originally being removed in the first place since the
hidden check will take care of this case already.

Updated the behavior to simply collapse instead of remove.

BUG=672867

Review-Url: https://codereview.chromium.org/2574603002
==========

to

==========
DevTools: Fix Elements tab event listener removal

Upon removal of the last event listener of a given type,
the entire tree for that type would be removed from the
DOM and never readded. It's not clear why it was
originally being removed in the first place since the
hidden check will take care of this case already.

Updated the behavior to simply collapse instead of remove.

BUG=672867

Committed: https://crrev.com/3c1e09c2062cfd0a37a47837eba5d2654d7a7198
Cr-Commit-Position: refs/heads/master@{#439547}
==========

[commit-bot: I haz the power, 2016-12-19 20:52:51]

Patchset 3 (id:??) landed as
https://crrev.com/3c1e09c2062cfd0a37a47837eba5d2654d7a7198
Cr-Commit-Position: refs/heads/master@{#439547}