[webkit-reviews] review granted: [Bug 190641] Web Inspector: display low-power enter/exit events in Timelines and Network node waterfalls : [Attachment 353549] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 190641: Web Inspector: display low-power enter/exit events in Timelines and
Network node waterfalls
https://bugs.webkit.org/show_bug.cgi?id=190641

Attachment 353549: Patch

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




--- Comment #10 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 353549
  --> https://bugs.webkit.org/attachment.cgi?id=353549
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/DOMEventsBreakdownView.css:82
> +    background-color: var(--value-changed-highlight);

Should we give this another variable name? Might be weird if we rely on this
being green and we suddenly change the value-changed-highlight color and this
suddenly ends up not being green.

How did this look in Dark mode? I suspect pretty good!

> Source/WebInspectorUI/UserInterface/Views/DOMEventsBreakdownView.js:84
> +	   if (this.layoutReason !== WI.View.LayoutReason.Dirty)
> +	       return;

It isn't obvious to me why we are bailing here.

> Source/WebInspectorUI/UserInterface/Views/DOMEventsBreakdownView.js:134
> +		       if (fullscreenRange.originator)
> +			   fullscreenArea.title = WI.UIString("Fullscreen from
â%sâ").format(fullscreenRange.originator.displayName);

I think we want to avoid unicode characters in our source code.

I believe having non-ASCII characters requires JavaScriptCore use a
non-8-bit-string for the source code and parsing. And since this is almost all
combined into a single Main.js I think that means a single file treated as
UTF-16 instead of ASCII. Obviously this isn't the first place we've used
non-ASCII but we may want to try eliminating it and seeing if there is a
measurable performance win. I'd think in memory alone we should see some
savings.