[Webkit-unassigned] [Bug 167524] Add support for Trac instances that host multiple projects.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=167524

--- Comment #3 from Kocsen Chung <kocsen_chung at apple.com> ---
(In reply to comment #2)
> Comment on attachment 299953 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299953&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:120
> > -        console.assert(fromDate <= toDate);
> > +        if (fromDate > toDate)
> > +            throw RangeError("Parameter fromDate should be before or on toDate.");
> 
> Why this change? This is logically an assertion, so it's cleaner to express
> it as such.
> 
> Just like in C/C++, adding a unit test that violates an assertion is not
> worth it.

It seemed too lenient to allow a range violation of this kind get away with an assertion. Additionally, it would allow for testing this code path. The way I interpret it, an assertion is an error and should be treated as such; especially in production code.
However, I do see your point that it may not be worth it to test this assert violation. Moreover I concur that it would be cleaner to express it as such. I will revert this change.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:131
> > +            "&from=" + encodeURIComponent(toDay.toISOString().slice(0, 10)) +
> > +            "&daysback=" + encodeURIComponent((toDay - fromDay) / 1000 / 60 / 60 / 24);
> 
> Why add encodeURIComponent here if it's not needed?

Mostly as a security mechanism to not trust date input and as a result avoid unexpected or erroneous requests to the server. In my performance profiling, this had minimal effect and can provide security benefits.

Thanks for the review, Alexey.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170130/d48a6504/attachment.html>