Summary: | Web Inspector: highlight newly added console messages in the Activity Viewer | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||
Component: | Web Inspector | Assignee: | 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
Antoine Quint
2013-09-30 02:30:20 PDT
Created attachment 212966 [details]
Patch
Created attachment 213070 [details]
Screen recording showing the pulsing in action
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> … (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. Created attachment 213156 [details]
Patch for landing
Comment on attachment 213156 [details] Patch for landing Clearing flags on attachment: 213156 Committed r156765: <http://trac.webkit.org/changeset/156765> All reviewed patches have been landed. Closing bug. |