[webkit-reviews] review denied: [Bug 83716] Show flakiness dashboard data in garden-o-matic : [Attachment 136811] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 83716: Show flakiness dashboard data in garden-o-matic
https://bugs.webkit.org/show_bug.cgi?id=83716

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=136811&action=review


I'm marking this as R-.  I'm hoping we can find a way to add this feature that
doesn't involve as much fighting the platform.

>>>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/ui/results.js:212
>>> +	     window.addEventListener('scroll',
this._syncIframeToPlaceHolder.bind(this));
>> 
>> We really need a scroll listener?  That seems very unfortunate.
> 
> Why unfortunate?

Scroll listeners make scrolling slow and janky.  They're also as sign that
we're doing things wrong and fighting the platform rather than working with it.


>>>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/ui/results.js:306
>>> +	     var deadline = Date.now() + 5000;
>> 
>> This is crazy!  5 seconds just seems like a random amount of time.
> 
> In practice, this polling is cheap (uses <1% of my cpu). So we can just pick
a time that's long enough to work >99.9% of the time. So, I picked 5 seconds as
that seemed like more than enough.
> 
> I listed in the FIXME above what we'd need to do in order to get rid of this.
In practice, this turns out to be good enough I think. Maybe jQuery gives a way
to determine when the animation is done, but I can't find it.

Yeah, you can get a callback when the animation is done.  If we can't get that
working, I'd rather just remove the animation than having a 100ms poll loop.