DevTools: introduce dom markers, decorate hidden, forced state and breakpoint elements using marker…

DevTools: introduce dom markers, decorate hidden, forced state and breakpoint elements using markers.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200273

[pfeldman, 2015-08-07 00:08:04]

pfeldman@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2015-08-07 23:35:05]

lgtm with comments

https://codereview.chromium.org/1273313002/diff/1/LayoutTests/inspector/eleme...
File LayoutTests/inspector/elements/hide-shortcut.html (right):

https://codereview.chromium.org/1273313002/diff/1/LayoutTests/inspector/eleme...
LayoutTests/inspector/elements/hide-shortcut.html:13: function
pseudoIframeVisibility(resolve, reject, pseudo)
You never pass |pseudo| here. Remove?

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/c...
File Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js (right):

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/c...
Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:62:
WebInspector.DOMBreakpointsSidebarPane.Marker = "breakpoint";
When I see constant, I assume that only this constant will be used everywhere.
This is not the case ("breakpoint" in module.json), so I'd rather inline instead
of constant.

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/c...
File Source/devtools/front_end/components/DOMPresentationUtils.js (right):

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/c...
Source/devtools/front_end/components/DOMPresentationUtils.js:614: PseudoState:
"pseudo-state",
These are defined here, but only used in elements module. Move there?

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/c...
File Source/devtools/front_end/components/module.json (right):

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/c...
Source/devtools/front_end/components/module.json:15: "marker": "breakpoint"
I'd name markers like "breakpoint-marker" just for the sake of search in code.

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/e...
File Source/devtools/front_end/elements/ElementsPanel.js (left):

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/e...
Source/devtools/front_end/elements/ElementsPanel.js:361:
this._treeOutlineForNode(node).updateOpenCloseTags(node);
This call disappeared. I don't really understand it, but I suspect regression
here.

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/e...
File Source/devtools/front_end/elements/ElementsTreeElement.js (right):

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/e...
Source/devtools/front_end/elements/ElementsTreeElement.js:1113: function
decorate(n, decorator)
This is more of |collectDecoration| rather than |decorate|.

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/s...
File Source/devtools/front_end/sdk/DOMModel.js (right):

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/sdk/DOMModel.js:653:
this._domModel.dispatchEventToListeners(WebInspector.DOMModel.Events.MarkersChanged,
this);
if (node._subtreeMarkerCount) ...

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/sdk/DOMModel.js:801:
this._domModel.dispatchEventToListeners(WebInspector.DOMModel.Events.MarkersChanged,
node);
This call is done below.

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/sdk/DOMModel.js:804:
this._domModel.dispatchEventToListeners(WebInspector.DOMModel.Events.MarkersChanged,
node);
This will make it O(n^2), since every event handler will traverse all child
nodes again. We can cache something or only issue this event for |this| and make
clients understand this implies changes on all parent nodes.

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/sdk/DOMModel.js:824: return (this._markers &&
this._markers[name]) || null;
|this._markers| is always an object.

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/sdk/DOMModel.js:830: markers: function()
Is this one used?

[pfeldman, 2015-08-10 21:25:16]

https://codereview.chromium.org/1273313002/diff/1/LayoutTests/inspector/eleme...
File LayoutTests/inspector/elements/hide-shortcut.html (right):

https://codereview.chromium.org/1273313002/diff/1/LayoutTests/inspector/eleme...
LayoutTests/inspector/elements/hide-shortcut.html:13: function
pseudoIframeVisibility(resolve, reject, pseudo)
On 2015/08/07 23:35:05, dgozman wrote:
> You never pass |pseudo| here. Remove?

Done.

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/c...
File Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js (right):

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/c...
Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:62:
WebInspector.DOMBreakpointsSidebarPane.Marker = "breakpoint";
On 2015/08/07 23:35:05, dgozman wrote:
> When I see constant, I assume that only this constant will be used everywhere.
> This is not the case ("breakpoint" in module.json), so I'd rather inline
instead
> of constant.

I'd rather leave it as is. It has multiple usages.

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/c...
File Source/devtools/front_end/components/DOMPresentationUtils.js (right):

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/c...
Source/devtools/front_end/components/DOMPresentationUtils.js:614: PseudoState:
"pseudo-state",
On 2015/08/07 23:35:05, dgozman wrote:
> These are defined here, but only used in elements module. Move there?

Done.

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/c...
File Source/devtools/front_end/components/module.json (right):

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/c...
Source/devtools/front_end/components/module.json:15: "marker": "breakpoint"
On 2015/08/07 23:35:05, dgozman wrote:
> I'd name markers like "breakpoint-marker" just for the sake of search in code.

Done.

[pfeldman, 2015-08-10 21:25:19]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2015-08-10 21:25:20]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1273313002/#ps20001
(title: "for landing")

[commit-bot: I haz the power, 2015-08-10 21:25:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273313002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273313002/20001

[pfeldman, 2015-08-10 21:28:05]

The CQ bit was unchecked by pfeldman@chromium.org

[pfeldman, 2015-08-10 21:35:17]

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/e...
File Source/devtools/front_end/elements/ElementsPanel.js (left):

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/e...
Source/devtools/front_end/elements/ElementsPanel.js:361:
this._treeOutlineForNode(node).updateOpenCloseTags(node);
On 2015/08/07 23:35:05, dgozman wrote:
> This call disappeared. I don't really understand it, but I suspect regression
> here.

This was indirectly triggering decorators which are now triggered from the
elementreeelement.

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/e...
File Source/devtools/front_end/elements/ElementsTreeElement.js (right):

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/e...
Source/devtools/front_end/elements/ElementsTreeElement.js:1113: function
decorate(n, decorator)
On 2015/08/07 23:35:05, dgozman wrote:
> This is more of |collectDecoration| rather than |decorate|.

Done.

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/s...
File Source/devtools/front_end/sdk/DOMModel.js (right):

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/sdk/DOMModel.js:653:
this._domModel.dispatchEventToListeners(WebInspector.DOMModel.Events.MarkersChanged,
this);
On 2015/08/07 23:35:05, dgozman wrote:
> if (node._subtreeMarkerCount) ...

Done.

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/sdk/DOMModel.js:801:
this._domModel.dispatchEventToListeners(WebInspector.DOMModel.Events.MarkersChanged,
node);
On 2015/08/07 23:35:05, dgozman wrote:
> This call is done below.

Done.

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/sdk/DOMModel.js:804:
this._domModel.dispatchEventToListeners(WebInspector.DOMModel.Events.MarkersChanged,
node);
On 2015/08/07 23:35:05, dgozman wrote:
> This will make it O(n^2), since every event handler will traverse all child
> nodes again. We can cache something or only issue this event for |this| and
make
> clients understand this implies changes on all parent nodes.

This will only be n^2 for the nodes that have markers set. We can improve this
when/if we have a lot of markers.

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/sdk/DOMModel.js:824: return (this._markers &&
this._markers[name]) || null;
On 2015/08/07 23:35:05, dgozman wrote:
> |this._markers| is always an object.

Done.

https://codereview.chromium.org/1273313002/diff/1/Source/devtools/front_end/s...
Source/devtools/front_end/sdk/DOMModel.js:830: markers: function()
On 2015/08/07 23:35:05, dgozman wrote:
> Is this one used?

It is.

[pfeldman, 2015-08-10 21:46:48]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2015-08-10 21:46:49]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1273313002/#ps40001
(title: "for landing")

[commit-bot: I haz the power, 2015-08-10 21:47:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273313002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273313002/40001

[commit-bot: I haz the power, 2015-08-10 22:52:31]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200273