Bug 122093

Summary: Web Inspector: highlight newly added console messages in the Activity Viewer
Product: WebKit Reporter: Antoine Quint <graouts>
Component: Web InspectorAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, graouts, joepeck, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Screen recording showing the pulsing in action
none
Patch for landing none

Description Antoine Quint 2013-09-30 02:30:20 PDT
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.
Comment 1 Radar WebKit Bug Importer 2013-09-30 02:30:56 PDT
<rdar://problem/15109711>
Comment 2 Antoine Quint 2013-09-30 02:31:33 PDT
<rdar://problem/14502312>
Comment 3 Antoine Quint 2013-09-30 02:37:12 PDT
Created attachment 212966 [details]
Patch
Comment 4 Antoine Quint 2013-10-01 03:06:14 PDT
Created attachment 213070 [details]
Screen recording showing the pulsing in action
Comment 5 Joseph Pecoraro 2013-10-01 10:25:20 PDT
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> …
Comment 6 Antoine Quint 2013-10-01 12:07:01 PDT
(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.
Comment 7 Antoine Quint 2013-10-02 01:57:09 PDT
Created attachment 213156 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2013-10-02 02:31:21 PDT
Comment on attachment 213156 [details]
Patch for landing

Clearing flags on attachment: 213156

Committed r156765: <http://trac.webkit.org/changeset/156765>
Comment 9 WebKit Commit Bot 2013-10-02 02:31:25 PDT
All reviewed patches have been landed.  Closing bug.