[webkit-reviews] review granted: [Bug 122093] Web Inspector: highlight newly added console messages in the Activity Viewer : [Attachment 212966] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Antoine Quint
<graouts at apple.com>'s request for review:
Bug 122093: Web Inspector: highlight newly added console messages in the
Activity Viewer
https://bugs.webkit.org/show_bug.cgi?id=122093

Attachment 212966: Patch
https://bugs.webkit.org/attachment.cgi?id=212966&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=212966&action=review


r=me

> Source/WebInspectorUI/UserInterface/DashboardView.js:224
> +	   function animationEnded(event)
> +	   {
> +	       if (event.target === container) {
> +		   container.classList.remove("pulsing");
> +		   container.removeEventListener("webkitAnimationEnd",
animationEnded);
> +	       }
> +	   }

Hmmm. How does this behave if you have a couple console.logs in less then the
animation time frame (0.75s).

    js> console.log("one"); setTimeout(function() { console.log("two"); }, 50);


It seems like you're avoiding the messiness, so I'm very happy. But what
exactly happens, does the remove("pulsing") trigger webkitAnimationEnd or skip
it entirely?

> Source/WebInspectorUI/UserInterface/DashboardView.js:230
> +	       container.offsetWidth;

Is this really necessary? Maybe we should instead create a
Element.prototype.forceLayout or forceStyleRecalc in Utilities.js. I don't like
just seeing the "offsetWidth" here.

Your comment indicates we need to force a style invalidation. Is this really a
WebCore issue? I'd rather see a FIXME: <bugzilla bug> …