[webkit-reviews] review granted: [Bug 194972] Web Inspector: CPU Usage Timeline - Main Thread Indicator : [Attachment 362964] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 194972: Web Inspector: CPU Usage Timeline - Main Thread Indicator
https://bugs.webkit.org/show_bug.cgi?id=194972

Attachment 362964: [PATCH] Proposed Fix

https://bugs.webkit.org/attachment.cgi?id=362964&action=review




--- Comment #8 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 362964
  --> https://bugs.webkit.org/attachment.cgi?id=362964
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=362964&action=review

r=me

I think this might need a rebase, as bugzilla wasn't able to apply it during
the review :(

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:221
> +Object.defineProperty(Node.prototype, "enclosingNodeOrSelfWithLocalName",

I agree with @Nikita.  I think we should fix the existing version, rather than
create a new one.

Considering that we `toUpperCase` the `nodeNames`, we should be able to just
`toLowerCase` the `node.nodeName` instead.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:208
> +	  
this._mainThreadWorkIndicatorView.chart.element.addEventListener("click",
this._handleIndicatorClick.bind(this));

This could use a more "specific" name, like
`_handleMainThreadWorkIndicatorClick`.