[DevTools] Extracted EventListenersTreeOutline (issue 1144953005 by kozyatinskiy@chromium.org)

0 views
Skip to first unread message

kozyat...@chromium.org

unread,
May 21, 2015, 9:32:44 AM5/21/15
to pfel...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, yurys...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, sergey...@chromium.org, kozyatins...@chromium.org
Reviewers: pfeldman_slow,

Message:
ptal

Description:
[DevTools] Extracted EventListenersTreeOutline

EventListenersTreeOutline is extracted from EventListenersSidebarPane.js. It
will be reused in Sources Panel for global objects event listeners.
Event listeners from window object are shown in EventListenersSidebar for
window
in current node's execution context if filter is switched to "All Nodes".

BUG=469159
R=pfel...@chromium.org

Please review this at https://codereview.chromium.org/1144953005/

Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Affected files (+418, -240 lines):
M LayoutTests/http/tests/inspector/elements-test.js
M LayoutTests/inspector/elements/event-listener-sidebar-expected.txt
M LayoutTests/inspector/elements/event-listeners-about-blank-expected.txt
M Source/devtools/devtools.gypi
A Source/devtools/front_end/components/EventListenersTreeOutline.js
A Source/devtools/front_end/components/eventListenersTreeOutline.css
M Source/devtools/front_end/components/module.json
M Source/devtools/front_end/elements/EventListenersSidebarPane.js
M Source/devtools/front_end/elements/elementsPanel.css


pfel...@chromium.org

unread,
May 21, 2015, 5:22:40 PM5/21/15
to kozyat...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, yurys...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, sergey...@chromium.org, kozyatins...@chromium.org
Could you refactor it in two steps so that I could review the changes using
side-by-side diff?

https://codereview.chromium.org/1144953005/

kozyat...@chromium.org

unread,
May 22, 2015, 6:54:11 AM5/22/15
to pfel...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, yurys...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, sergey...@chromium.org, kozyatins...@chromium.org
On 2015/05/21 21:22:40, pfeldman_slow wrote:
> Could you refactor it in two steps so that I could review the changes
> using
> side-by-side diff?

I've split it into two CLs - this one and
https://codereview.chromium.org/1154703005/ .
Please take a look.

https://codereview.chromium.org/1144953005/

pfel...@chromium.org

unread,
May 22, 2015, 12:11:23 PM5/22/15
to kozyat...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, yurys...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, sergey...@chromium.org, kozyatins...@chromium.org
Thanks!


https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js
File Source/devtools/front_end/components/EventListenersTreeOutline.js
(right):

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode10
Source/devtools/front_end/components/EventListenersTreeOutline.js:10:
WebInspector.EventListenersTreeOutline = function(element, objectGroup)
The name is unfortunate since this is not a tree outline.

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode14
Source/devtools/front_end/components/EventListenersTreeOutline.js:14:
this._treeOutline = new TreeOutline(true);
Please use TreeOutlineInShadow

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode16
Source/devtools/front_end/components/EventListenersTreeOutline.js:16:
this._treeOutline.element.classList.add("event-listener-tree",
"outline-disclosure", "monospace");
You would not need the outline-disclosure

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode18
Source/devtools/front_end/components/EventListenersTreeOutline.js:18:
this._treeItemMap = new Map();
Annotate the type please.

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode20
Source/devtools/front_end/components/EventListenersTreeOutline.js:20:
this._lastRuntimeAgents = new Set();
Ditto. Also, is this a leak?

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode42
Source/devtools/front_end/components/EventListenersTreeOutline.js:42:
_getTreeElementForType: function(type)
no get prefixes in Blink, but in this case getOrCreate..

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode49
Source/devtools/front_end/components/EventListenersTreeOutline.js:49: if
(this._isEmpty) {
You could get child count from root node instead, but you don't need it.

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode52
Source/devtools/front_end/components/EventListenersTreeOutline.js:52:
this._element.appendChild(this._treeOutline.element);
No need to do this lazily, just remove the placeholder unconditionally.
It is noop if it is removed.

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode93
Source/devtools/front_end/components/EventListenersTreeOutline.js:93:
node.resolveToObject(this._objectGroup, objectCallback.bind(this));
Reverse the order of functions and calls so that they went top -> bottom

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode117
Source/devtools/front_end/components/EventListenersTreeOutline.js:117:
WebInspector.EventListenersTreeElement = function(type, linkifier)
You don't seem to override anything, don't inherit.

https://codereview.chromium.org/1144953005/

kozyat...@chromium.org

unread,
May 22, 2015, 1:43:43 PM5/22/15
to pfel...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, yurys...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, sergey...@chromium.org, kozyatins...@chromium.org
All done. Please take a look!


https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js
File Source/devtools/front_end/components/EventListenersTreeOutline.js
(right):

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode10
Source/devtools/front_end/components/EventListenersTreeOutline.js:10:
WebInspector.EventListenersTreeOutline = function(element, objectGroup)
On 2015/05/22 16:11:23, pfeldman_slow wrote:
> The name is unfortunate since this is not a tree outline.

Renamed to EventListenersView

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode14
Source/devtools/front_end/components/EventListenersTreeOutline.js:14:
this._treeOutline = new TreeOutline(true);
On 2015/05/22 16:11:23, pfeldman_slow wrote:
> Please use TreeOutlineInShadow

Done.

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode16
Source/devtools/front_end/components/EventListenersTreeOutline.js:16:
this._treeOutline.element.classList.add("event-listener-tree",
"outline-disclosure", "monospace");
On 2015/05/22 16:11:23, pfeldman_slow wrote:
> You would not need the outline-disclosure

Removed.

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode18
Source/devtools/front_end/components/EventListenersTreeOutline.js:18:
this._treeItemMap = new Map();
On 2015/05/22 16:11:23, pfeldman_slow wrote:
> Annotate the type please.

Done.

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode20
Source/devtools/front_end/components/EventListenersTreeOutline.js:20:
this._lastRuntimeAgents = new Set();
On 2015/05/22 16:11:22, pfeldman_slow wrote:
> Ditto. Also, is this a leak?

releaseObjectGroup logic is shifted to EventListenerssSidebarPane.

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode42
Source/devtools/front_end/components/EventListenersTreeOutline.js:42:
_getTreeElementForType: function(type)
On 2015/05/22 16:11:23, pfeldman_slow wrote:
> no get prefixes in Blink, but in this case getOrCreate..

Done.

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode49
Source/devtools/front_end/components/EventListenersTreeOutline.js:49: if
(this._isEmpty) {
On 2015/05/22 16:11:23, pfeldman_slow wrote:
> You could get child count from root node instead, but you don't need
it.

Done.

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode52
Source/devtools/front_end/components/EventListenersTreeOutline.js:52:
this._element.appendChild(this._treeOutline.element);
On 2015/05/22 16:11:22, pfeldman_slow wrote:
> No need to do this lazily, just remove the placeholder
unconditionally. It is
> noop if it is removed.

Done.

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode93
Source/devtools/front_end/components/EventListenersTreeOutline.js:93:
node.resolveToObject(this._objectGroup, objectCallback.bind(this));
On 2015/05/22 16:11:23, pfeldman_slow wrote:
> Reverse the order of functions and calls so that they went top ->
bottom

Done.

https://codereview.chromium.org/1144953005/diff/20001/Source/devtools/front_end/components/EventListenersTreeOutline.js#newcode117
Source/devtools/front_end/components/EventListenersTreeOutline.js:117:
WebInspector.EventListenersTreeElement = function(type, linkifier)
On 2015/05/22 16:11:22, pfeldman_slow wrote:
> You don't seem to override anything, don't inherit.

I use this object in line 55: this._treeOutline.appendChild(treeItem);
appendChild function signature contains TreeElement.

https://codereview.chromium.org/1144953005/

pfel...@chromium.org

unread,
May 25, 2015, 8:08:13 AM5/25/15
to kozyat...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, yurys...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, sergey...@chromium.org, kozyatins...@chromium.org
lgtm



https://codereview.chromium.org/1144953005/

'I haz the power - commit-bot' via codereview.chromium.org

unread,
May 25, 2015, 8:09:08 AM5/25/15
to kozyat...@chromium.org, pfel...@chromium.org, commi...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, yurys...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, sergey...@chromium.org, kozyatins...@chromium.org
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1144953005/40001


https://codereview.chromium.org/1144953005/

'I haz the power - commit-bot' via codereview.chromium.org

unread,
May 25, 2015, 9:22:47 AM5/25/15
to kozyat...@chromium.org, pfel...@chromium.org, commi...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, yurys...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, sergey...@chromium.org, kozyatins...@chromium.org
Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195862

https://codereview.chromium.org/1144953005/
Reply all
Reply to author
Forward
0 new messages