Bug 122093 – Web Inspector: highlight newly added console messages in the Activity Viewer

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Antoine Quint]

I think we could do a better job to inform the developer that a new message was logged to the console by highlighting the relevant message count in the Activity Viewer such that the developer could get a sense a new message was added without having to lose its context too much.

[Radar WebKit Bug Importer]

<rdar://problem/15109711>

[Antoine Quint]

<rdar://problem/14502312>

[Antoine Quint]

Created attachment 212966 [details]
Patch

[Antoine Quint]

Created attachment 213070 [details]
Screen recording showing the pulsing in action

[Joseph Pecoraro]

Comment on attachment 212966 [details]
Patch

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> …

[Antoine Quint]

(In reply to comment #5)
> (From update of attachment 212966 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212966&action=review
> 
> > 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?

It removes the class, invalidates styles, therefore returning to the start value, and then resets the pulsing class so that it animates again. Visually, the animation restarts every time the counter is incremented.

> > 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.

Yes, we could add a method with an adequate name that would do this… but it would still be this call (or a call to getComputedStyle() on this element).
 
> Your comment indicates we need to force a style invalidation. Is this really a WebCore issue? I'd rather see a FIXME: <bugzilla bug> …

It's just the way things work, I don't think there's a bug here, just a suckiness of CSS.

[Antoine Quint]

Created attachment 213156 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 213156 [details]
Patch for landing

Clearing flags on attachment: 213156

Committed r156765: <http://trac.webkit.org/changeset/156765>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.